CodeSOD: Truly Strung Out

Strings in C remain hard. They're complicated- they're complicated because of null termination, they're complicated because of memory management, they're complicated because of the behaviors of const. There's a lot going on in C strings.

But Alice had a simple case: convert a true or false value into the string "true" or "false". Let's see how her co-worker solved this:

char* __comps_num2boolstr(COMPS_Object* obj) { char *ret; char *_bool; if (((COMPS_Num*)obj)->val) { _bool = "true"; } else { _bool = "false"; } ret = malloc(sizeof(char) * (strlen(_bool)+1)); ret[0] = 0; strcat(ret, _bool); return ret;}

The __ at the top leads me to believe this is their convention for making the function "private"- it's not meant to be called outside of this specific module. Also, this has the specific job of turning a boolean value into a string, so it seems awkward and weird to pass a whole structure, only to cast the structure into a different type and access a member. What could be a handy utility method has been overspecialized.

But that's not where things get weird.

This starts by assigning a static, hard-coded string into _bool. Then it allocates a buffer for ret, and uses strcat to place the contents of _bool into ret, so that ret can be returned.

We'll come back to why the entire premise of this method is absurd, but let's take the method at face value and look at what's wrong with the implementation as it stands.

Everything about how they fill the buffer is confusing and weird. First, they use strcat when they could have used strcpy, removing the need for the ret[0] = 0 (which terminates the string at the first character).

But even if they used strcpy, that'd be weird, because they could have gotten rid of the malloc and the null terminator, and just used strdup which does all that for you.

But all of that is completely unnecessary, because you can just do this:

char* __comps_num2boolstr(COMPS_Object* obj) { if (((COMPS_Num*)obj)->val) { return "true" } else { return "false" }}

String literals like this are implicitly static const char*, which means they live for the life of the program, and thus that pointer is always valid. Attempts to modify that literal would have undefined behavior, but given that this function's job is to create strings to be fed into printf calls, that's not a problem in this case, freeing other code of having to free this memory.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!