Code Gem of the Day…


I ran across this particular “gem” today in some C code:


char * string = Foo();

if (string == NULL)
{
   string = (char *)malloc(1);
   sprintf(string, "%c", NULL);
}

The intent was to guarantee that “string” contains a C string. This implementation is amazingly stupid for two reasons.

  1. It is really inefficient. What they really wanted on the sprintf line is string[0] = 0; The sprintf line, requires a function call, requires the stack be set up with the string parameter "%c", and requires the function decode the string to determine how to interpret the optional arguments. The alternative I propose ends up being a couple of assembly instructions, no function call, and certainly no format string being generated.
  2. It overruns the 1 byte buffer they allocated. sprintf null terminates the string it creates, without regard for the size of the buffer you provide for storage, and without regard for the content of the formatting values. A safer way to do this would be to use snprintf, which allows you to specify how much storage you are providing. Or better yet, use the suggestion above.

The whole function that this code was contained in was poorly written, but this “gem” stood out.

Be Sociable, Share!
  1. #1 by daddy-o on September 21, 2006 - 3:17 pm

    I agree with almost all of your points.

(will not be published)