21/07/2006 @13:31:25 ^17:58:47

Behaviour of GNU libc snprintf with write buffer in argument list

Unlike sprintf (which these days must always be referred to as "the unsafe sprintf") snprintf clears the write buffer before starting any processing. This has implications for calls that use the write buffer in the format arguments (I call this "snprintf with writeback" because you're trying to write back into one of the arguments) What's worse is that I can't find this behaviour documented anywhere.

Background

PrBoom 23x has its own snprintf implementation called psnprintf ("portable" snprintf, apparently) which it inherited from their ill-advised attempt to merge in much if not all of the SMMU/Eternity codebase. For rboom I decided to try to replace psnprintf with snprintf from the underlying libc. But guess what, everything broke.

The problem turned out to be this: In a number of places, 23x uses psnprintf to lazily append to a string that already exists, like so:

psnprintf(buffer, size, "%s%s", buffer, string);

To clarify, suppose you start with "abc" then append "def" using the above code; this works fine with psnprintf and you'd end up with "abcdef" in the buffer but with libc's snprintf you'd just get "def".

Now I know it's not very efficient but it does take care of null termination for you (it would seem that snprintf never writes more than size-1 characters and always puts a zero on the end). You could argue that you should be using strncat or something. But the size you pass to strncat is the maximum number of copyable characters, not the total size of the destination buffer, so you have to keep track of the length of the string already in the buffer which increases code complexity. Also you can only append character arrays; with snprintf you can append numbers, single characters, etc as well (and 23x does)

Workarounds

So far I have three workarounds that come with various tradeoffs of code complexity, efficiency, portability and so forth. Consider the following hypothetical join function that takes an array of strings and mashes them all together:

char *join(char *buf, size_t size, int argc, char **argv)
{
  int i;

  *buf = 0; // start off with an empty buffer
  for (i = 0; i < argc; i++)
    snprintf(buf, size, "%s%s", buf, argv[i]);
  return buf;
}

Of course, the whole point is that with libc's snprintf, this won't work.

1. Stop being a pussy and keep track of the length like you know you should

This is probably what you should be doing, and, in this example, isn't very hard to add. The following example relies on snprintf returning the length of string it would have written if it hadn't run out of buffer. Some implementations don't do this, I guess you'd have to mess with strlen in that case.

char *join(char *buf, size_t size, int argc, char **argv)
{
  int i, len;

  for (i = 0, len = 0; i < argc && len < size; i++)
    len += snprintf(buf+len, size-len, "%s", argv[i]);
  return buf;
}

The format string and arguments no longer refer to the write buffer at all, so the problem doesn't occur. Note you no longer need to initialise the buffer, instead you have a variable len to keep track of how far through the buffer you are. You also break out of the loop early if the length written exceeds the size (it won't overflow the buffer of course, that's the point of snprintf, but snprintf is returning the number of characters it would have written)

So yes, as I say, this is probably the best method as it avoids having the write buffer in the argument list entirely. However, you have to do the length tracking at the same code level as the call to snprintf. You can't just write a wrapper around it that does this. That's okay here, in this example it's easy. However some of these calls in 23x are already inside fairly complex loops of their own. Adding in another variable to keep track of the written length is going to be extremely prone to errors, so I chickened out of it and decided to search for an easier solution.

2. Write to a different buffer and then copy it back

Obvious but inefficient solution. The following example exaggerates the inefficiency, and also uses C99 variable-length array declarations - if you can't do this, stunt something up with malloc (which will make it even more inefficient...)

char *join(char *buf, size_t size, int argc, char **argv)
{
  int i;
  char copy[size];

  *buf = 0; // start off with an empty buffer
  for (i = 0; i < argc; i++) {
    snprintf(copy, size, "%s%s", buf, argv[i]);
    memcpy(buf, copy, size);
  }
  return buf;
}

Here just for completeness is a version that uses malloc/realloc to keep a static buffer. Note the useful shortcut - calling realloc with a null pointer is equivalent to calling malloc. (You could also use asprintf, if available. I'll leave that as an exercise for the reader.)

char *join(char *buf, size_t size, int argc, char **argv)
{
  static char *copy = NULL;
  static size_t copysize = 0;
  int i;

  if (size > copysize)
    copy = realloc(copy, (copysize = size));

  *buf = '\0'; // start off with an empty buffer
  for (i = 0; i < argc; i++) {
    snprintf(copy, size, "%s%s", buf, argv[i]);
    memcpy(buf, copy, size);
  }
  return buf;
}

Anyway, so basically, you have a second buffer you write into, to avoid writeback. Then you copy that into the original. So the string is going back and forth and back and forth... Obviously copying the whole buffer with memcpy is going to be a total waste of processing time; using strncpy to only copy up to the first zero byte would improve that. Furthermore you could break out of the loop early if you detect that snprintf has tried to write more bytes than the buffer size:

    int len = snprintf(copy, size, "%s%s", buf, argv[i]);
    strncpy(buf, copy, size);
    if (len >= size) break;

But then you're practically doing length tracking anyway.

3. GROSS HACK ALERT

So after all this copying, I poked around a bit and it turns out that snprintf doesn't zero the whole buffer before doing any processing. In fact, it only zeroes the first element. So why not just preserve that one character?

char *join(char *buf, size_t size, int argc, char **argv)
{
  int i;

  if (argc == 0) {
    *buf = '\0'; // if no arguments, return an empty buffer
  } else {
    snprintf(buf, size, "%s", argv[0]);
    for (i = 1; i < argc; i++)
      snprintf(buf, size, "%c%s%s", *buf, buf+1, argv[i]);
  }
  return buf;
}

This makes the huge assumption that it is only the first character that gets zeroed, but who's to say? I don't like this, it's clearly a hack.

Conclusion

1. Length tracking is clearly the "best" way to do it but the hardest to write. You can't write an "append_snprintf" wrapper that appends to the buffer instead of overwriting it, and then simply replace the call to snprintf. You need to keep some state information - the current written length - between calls. You could do this by passing pointers to the buffer pointer and length and letting it update those, but they'd still need to be initialised. It's all extra complexity.
2. A wrapper that allocates a second scratch buffer and writes into that is inefficient, but is it too inefficient? Perhaps not, but I don't like the argument that code only has to be "good enough" and "these days everybody has computers whose speeds and memory sizes involve the prefix 'giga' so who cares if you waste a few cycles" and "buy new hardware if the software runs too slowly". That shit is for idiot corporations who pay bad programmers far too high a salary.
3. Making assumptions about the internal workings of library functions only leads to portability headaches. Not that I care too much about portability but I suppose it's the principle of the thing. I'd feel obliged to write complicated tests in the configure scripts and autoconf isn't exactly my strong point...

As for the apparent lack of documentation, well. At least in Debian Sarge it doesn't say anything about this on snprintf's manual page, or even in the libc info documentation. However I did some more searches and it turns out that the C99 standard itself says that "If copying takes place between objects that overlap, the behavior is undefined." That basically means, "don't do it". That's another reason to rule out the gross hack.

I think I'll just keep psnprintf for now, and maybe find or write a general string appending function later. Anyway, I hope someone found this interesting because once I got into it I quite enjoyed writing it!!