snprintf() confusion [LWN.net]

Any C coder worth his or her salt knows that encoding text into a string with sprintf() invites buffer overflows, and is thus dangerous. The proper way of doing things is with snprintf(), which takes the length of the destination string as a parameter, and will not overrun it. Callers to snprintf() generally assume that the return value is the length of what was actually encoded into the destination array. That turns out, however, to not be the case. As per the C99 standard, snprintf() returns the length the resulting string would be, assuming it all fit into the destination array. As a result of this misunderstanding, the kernel is full of snprintf() calls which use the return value incorrectly.

This mistake is rarely a problem; snprintf() almost never has to truncate its output, so the return value is what the programmer is expecting. Every miscoded use is an invitation for trouble, however, and really should be fixed. To that end, the 2.6.2-rc3-mm1 tree contains a patch by Juergen Quade which adds a couple of new functions:

    int scnprintf(char *buf, size_t size, const char *format, ...);
    int vscnprintf(char *buf, size_t size, const char *format, va_list args);

The new functions work the way many programmers expected the old ones to: they return the length of the string actually created in buf. The plan is to migrate the kernel over to the new functions; the patch fixes well over 200 snprintf() and vsnprint() calls. Unless the old functions are eventually removed, however, they are likely to be a source of programming errors well into the future.

(Log in to post comments)

snprintf() not the only way

Posted Feb 5, 2004 21:18 UTC (Thu) by iabervon (subscriber, #722) [Link]

To calculate the right buffer size, you should use code which stays tied
to the format and conversion code. Like, for example, "snprintf(NULL, 0,
format, ...)"; the kernel version of vsnprintf actually obeys a zero size
and doesn't dereference the buffer. It would actually be a neat trick to
detect this case and handle it, if possible, at compile time. That way,
people could get the compiler to size their buffers correctly
automatically.

snprintf() not the only way

Posted Feb 7, 2004 10:07 UTC (Sat) by ThePythonicCow (guest, #11308) [Link]

Utterly impossible to size at compile time -- the length of what is added to the buffer varies with runtime data. Something as simple as snprintf(buf, 999, "%d", random()); could put 1 to 10 digits in buf.

snprintf() confusion

Posted Feb 5, 2004 8:43 UTC (Thu) by ThePythonicCow (guest, #11308) [Link]

Historically, there have been two variants of snprintf, with the ISO C99 standard, taken from BSD, of returning the length of the entire formatted string, even if it didn't entirely fit.

Earlier versions of snprintf only returned a count of how many non-nul characters went into the buffer.

This earlier return convention was actually "obvious" at the time -- no one on those slow, old machines would expect a C library routine to be wasting its CPU cycles calculating the length of some formatted output that it wasn't going to output.

And my impression is that most of us old timers are surprised by the new convention.

This new convention is also not worth much in most kernel code, which doesn't do anything with the extra information anyway. If the intended output doesn't fit (it's usually a console print or something) then tough - it gets truncated. Not usually worth the bother to try to expand the buffer size (and you have to be careful, not to open the kernel up to a Denial of Service attack, if the user can somehow persuade it to attempt to print something that's surprisingly long.

Still, Linus has clearly chosen the new ISO C99 standard (someone backed the lib/vsprintf.c code off to the "old standard", and Linus restored the C99 conventions.

snprintf() confusion

Posted Feb 6, 2004 23:50 UTC (Fri) by giraffedata (subscriber, #1954) [Link]

I doubt the wasted computation entered into the decision. The reason the old behavior was obvious at the time is that the function is emulating printf(), which like all Unix I/O routines, returns the number of characters transferred.

The modern sprintf return value, though useful, is actually quite unconventional.

snprintf() confusion

Posted Feb 7, 2004 10:00 UTC (Sat) by ThePythonicCow (guest, #11308) [Link]

I doubt the wasted computation entered into the decision.

Hmmmm ... I feel we are both guessing slightly off. You're likely right it wasn't an explicit decision to save cpu cycles. But the analogy with printf is fragile as well - it's not like printf has a choice of two possible lengths to choose from. It writes whatever it has format for, and counts it all (or SEGV's trying).

Your last words:

is actually quite unconventional

might come closest to the mark. Something about the modern value just seems odd. The early snprintf creators perhaps didn't even consider that choice of return value as an alternative.

snprintf() confusion

Posted Feb 7, 2004 18:09 UTC (Sat) by giraffedata (subscriber, #1954) [Link]

it's not like printf has a choice of two possible lengths to choose from. It writes whatever it has format for, and counts it all (or SEGV's trying).

OK, I see in the printf case, a partial write isn't practical. But it doesn't really weaken the argument that printf is an I/O routine, and so it is natural to follow the pattern of write() and fwrite() and return the number of bytes written.

The leap from there to snprintf() is a lot more obvious when you include the sprintf() step. sprintf(), derived from printf(), quite naturally returns the number of characters formatted, just like printf(). And it's a very useful value, too. snprintf() was derived from sprintf(), and merely adds a constraint on the length; If you're going to drop it in to replace a dangerous sprintf(), you clearly want it to do the same as sprintf() and return the number of characters formatted.

snprintf() confusion

Posted Feb 7, 2004 18:56 UTC (Sat) by ThePythonicCow (guest, #11308) [Link]

Yeah - that makes sense.

snprintf() confusion

Posted Feb 5, 2004 8:32 UTC (Thu) by ThePythonicCow (guest, #11308) [Link]

Pre-calculating the length of the resulting string, as suggested
above, doesn't cut it.

(1) It requires adding error prone code to pre-walk the result,
duplicating the choice of what to print. Soon the pre-walk
code and the actual format will get out of sync again.
(2) Sometimes you are printing variable stuff, that is changing as
you print it (on a multiprocessor). So you don't know the
length until you know it.
(3) Sometimes you are printing several items into a buffer, while
walking a list or something. You can't afford to walk the list
twice, and besides - see (2) above.
(4) The idea of a buffer limited routine like snprintf is to help
the user avoid disastrous mistakes that only blow up rarely.
Given all the difficulties (listed above) in getting the 'right'
answer as to the resulting length, you _still_ need a buffer
limited routine, because you still risk running off the end.

snprintf() confusion

Posted Feb 5, 2004 8:52 UTC (Thu) by ThePythonicCow (guest, #11308) [Link]

The biggest problem that this snprintf patch will fix is not noted in the article above.

There were a couple hundred places in the kernel that would loop:

len += snprintf (buf+len, buflen-len, "...", ...); accumulating the number of non-nul chars written into a buffer in 'len'. Well, actually, unbeknownst to the coder, accumulating the total number of characters that could have been written, if the buffer were big enough.

The 2nd snprintf parameter (buflen-len, here) would go negative, when the total number of chars that could have been written into the buffer exceeded the buffer length.

Since this 2nd parameter is passed as a type size_t (unsigned), this meant that the next call to snprintf claimed that the available buffer size was **huge**. A license to overrun buffers. Not something you desire in your average kernel.

The patch going in will have any buffer size that is huge (high bit set) be special cased as if it was zero, putting nothing more in the buffer.