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?


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.

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...