Sign in to follow this  
AaronA

sprintf, itoa, etc...

Recommended Posts

AaronA    142
I'm having a problem converting an integer to char or const char *. The following code gets the "Month Names" from a configuration file, but the program crashes right after the log says "Test 1", so its at the sprintf line. Note: I also tried using itoa, but I switched to sprintf for more of a standard function, besides, they both caused crash at the same point.
const char * LDevice::getMonth()
{
    // LTimeMonth DOES have a value
    char * LTimeMonthTMP;
    printf("Test 1\n");

    sprintf(LTimeMonthTMP, "%i", LTimeMonth); // --- CRASHES HERE ---

    printf("Test 2\n");
    CFG_SelectGroup(LTimeMonthTMP, CFG_False);
    printf("Test 3\n");
    return CFG_ReadText("Name", LTimeMonthTMP);
}
So, I was curious, why does it crash?

Share this post


Link to post
Share on other sites
Antheus    2409

You never allocate:
char * LTimeMonthTMP;


So this results in undefined behaviour:
sprintf(LTimeMonthTMP, "%i", LTimeMonth); // --- CRASHES HERE ---



/sigh

char* are evil....

Share this post


Link to post
Share on other sites
AaronA    142
Because I operate under the theory that as long as it works like I told it to, its perfectly fine. I can think of a million and one people against that and understand why, however at this particular point and on this particular project, I feel that it shouldn't concern me. :)

Share this post


Link to post
Share on other sites
ToohrVyk    1595
Quote:
Because I operate under the theory that as long as it works like I told it to, its perfectly fine.


A program always works as you tell it to. The problem is that, with char*, it's extremely easy to tell the program to crash, leak memory, or do things that you don't want it to do. In your source, the program did exactly what you asked it to: access a pointer which it didn't own. Basically, you have two options here.
  1. Write correct C-string code to manipulate strings. This implies measuring how much memory you must allocate for the result, allocate that memory, perform the operations in a safe manner (that is, snprintf, strncpy, strncat instead of sprintf, strcpy, strcat), and deallocate the memory (making sure that it is always deallocated when the function exits, including when an exception happens).
    const char * LDevice::getMonth()
    {
    const unsigned size = snprintf(0,0,"%i", LTimeMonth);
    char *strMonth = new char[size+1];

    try
    {
    snprintf(strMonth,size,"%i",LTimeMonth);
    CFG_SelectGroup(strMonth, CFG_False);
    const char *found = CFG_ReadText("Name", strMonth);
    delete[] strMonth;
    return found;
    }
    catch(...)
    {
    delete[] strMonth;
    throw;
    }
    }


  2. Use modern C++, using std::string and the corresponding helper classes and functions to write shorter and safe code.
    const char * LDevice::getMonth()
    {
    std::stringstream conversion;
    std::string strMonth = (conversion << LTimeMonth).str();
    CFG_SelectGroup( strMonth.c_str(), CFG_False );
    return CFG_ReadText( "Name", strMonth.c_str() );
    }



There is no third way that you can follow which will result in code which does what you intend. So, do you think you can write code using method one, and do you think it's worth the additional effort?

Quote:
Original post by dgreen02
Unless you know what you're doing :-D


Kinda like nitroglycerin—reason why dynamite was invented in the first place.

Share this post


Link to post
Share on other sites
AaronA    142
I can respect that, but wouldn't you feel that I reserve the right to use char * if I'd like? Even if its wrong, it'll give me a hands on learning lesson as they say :). I've already used quite a few char *'s anyway so switching now doesn't sound like my cup of tea, sorry, I'll try to include a warning if I decide to publicize the game :P.

Quote:
So, do you think you can write code using method one, and do you think it's worth the additional effort?


At this point yes (because I don't need conversion often). How about when I decide to start a new project, I'll use string instead? But right now it isn't fitting on the agenda.

Share this post


Link to post
Share on other sites
taby    1265
I personally try to avoid sprintf, etc:

int integer = 50;

ostringstream oss;
oss << integer;

char *ca = new char[oss.str().length() + 1];

// or memcpy, or copy...
for(size_t i = 0; i < oss.str().length(); i++)
ca[i] = oss.str()[i];

ca[oss.str().length()] = '\0';

...

delete [] ca;



or, avoiding char* like the plague...

ostringstream oss;
oss << integer;

string s = oss.str();



(Oops, beaten to the punch by ToohrVyk)

Share this post


Link to post
Share on other sites
taby    1265
On second thought, the direct usage of new/delete can (should?) be avoided for arrays:


int i = 50;

ostringstream oss;
oss << i;
string s = oss.str();

vector<char> vc(s.begin(), s.end());
vc.push_back('\0');

// Equivalent to char* only for vector container.
cout << &vc[0] << endl;

// The vector class automatically deletes its contained memory when the deconstructor is called.
// No potential memory leaks.

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
Original post by AaronA
Because I operate under the theory that as long as it works like I told it to, its perfectly fine. I can think of a million and one people against that and understand why, however at this particular point and on this particular project, I feel that it shouldn't concern me. :)


This is a common fallacy that leads to long term problems.

Your case is trivial - but is it?
const char * LDevice::getMonth()
{
char * LTimeMonthTMP = ... // either new or malloc
// OR
char LTimeMonthTMP[12]; // I believe 12 should cover 32-bit int
sprintf(LTimeMonthTMP, "%i", LTimeMonth);
CFG_SelectGroup(LTimeMonthTMP, CFG_False);
return CFG_ReadText("Name", LTimeMonthTMP);
}



If you allocate LTimeMonthTMP, you need to de-allocate it, and the allocation can fail. If it fails, how will it fail? With exception? With return code? What happens if CFG_SelectGroup or CFG_ReadText fail? Where will you de-allocate it?

On the other hand, allocating room on stack leads to obvious overflow scenario, since %i can display more than 32 bit integer value. One solution is to allocate more than 12 bytes, but that doesn't solve the problem, it merely leaves you open for other attacks on null-terminated strings - after all, you've given the attacker a typical stack overrun facility.

Consider something else as well. Let's say that 12 is indeed enough. The number 12 is unrelated to following sprintf function - what if, later, another programmer changes the string there to " %i". Everything will work. But putting in unusual values (month cannot be 2^32 - or can it?). Congratulations, you've just created the most typical buffer overrun scenario, and it wasn't even your fault.

The major problem of using null-terminated strings comes from their "null-terminatedness", and being a pointer in nature. First one is a problem since you have no way of testing an overrun condition, second one is a problem since you need to allocate/de-allocate them.

Compared to std::string, they aren't pointers, and they have well-known and protected length.

There's room for char *. But even then, writing your own read-only class referencing constant char * storage is much better.

For your case, explicit lookup table of pre-calculated string values would be much more suitable anyway. There's only 12 months, and anything beyond that is an error - so you even solve additional problem.

Share this post


Link to post
Share on other sites
Zahlman    1682
Wow, I'm late to the party.

But for the sake of making contributions: as well as ignoring the *real* string type that C++ provides to you, you're also ignoring its *real* boolean type, not to mention namespaces. You might want to think more about how you name things, too.


std::string Device::getMonth()
{
std::string monthAsString = (std::istringstream() << month).str();
CFG::SelectGroup(monthAsString, false);
return CFG::ReadText("Name", monthAsString);
}

// where the CFG functions are accordingly placed into a CFG namespace instead
// and accept a const std::string& instead of a const char*.


Quote:
Because I operate under the theory that as long as it works like I told it to, its perfectly fine.


An uninsulated wire does a perfectly good job of conducting electricity, so why would I ever bother insulating it, or taping the wires down, or considering the resistance of the load, or making a plug connection for the load instead of just soldering it in place, or...

Share this post


Link to post
Share on other sites
Verg    450
Quote:
Original post by AaronA
I can respect that, but wouldn't you feel that I reserve the right to use char * if I'd like? Even if its wrong, it'll give me a hands on learning lesson as they say :).


As they say, you can put all the bullets in the gun, but if you shoot yourself in the eye... it's someone else's fault [smile]

Personally I don't trust myself that much... and would sleep better by just using the right tools for the job (std::string and its counterparts).


C

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this