# sprintf, itoa, etc...

This topic is 3786 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

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");
}

So, I was curious, why does it crash?

##### Share on other sites

You never allocate:
char * LTimeMonthTMP;

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

/sigh

char* are evil....

##### Share on other sites
hehe, thanks. Indeed, normally they say to use std::string, but I like doing things the wrong way :), thanks again.

##### Share on other sites
Quote:
 Original post by AaronA...but I like doing things the wrong way :)
Why? (Just curious...)

##### Share on other sites
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 on other sites
Quote:
 Original post by Antheuschar* are evil....

Unless you know what you're doing :-D

##### Share on other sites
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 dgreen02Unless you know what you're doing :-D

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

##### Share on other sites
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 on other sites
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 on other sites
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 on other sites
Quote:
 Original post by AaronABecause 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 on other sites
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 on other sites
Quote:
 Original post by AaronAI 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 on other sites

This topic is 3786 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Create an account

Register a new account

• ### Forum Statistics

• Total Topics
628714
• Total Posts
2984353

• 23
• 11
• 10
• 13
• 14