[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: snprintf() revisited, please comment



On Tue, Aug 23, 2005, Moritz Grimm wrote:

1.
> - find out overall length of n parts that shall be reassembled in some
>   way and add 1 for the terminating NUL

2.
> - if re- or malloc()'ing the required buffer size failed, then
>   - cleanup
>   - return NULL (caller errs or warns about the failure, depending on
>     what's appropriate)

3.
> - else (we have a buffer that is definitely large enough to hold what we
>   need)
>   - assemble the parts with sprintf(), format strings only contain %c
>     and %s
>   - cleanup as appropriate

4.
> - return pointer to the (new) buffer


> While I wouldn't use sprintf() in any new code of my own, this does look 
> safe to me. If my assumption of this being safe is correct, then 

It is safe as long as step 1 and 3 agree on the size. If someone
inserts/changes something at step 3 without changing step 1 you
lose (such a bug was introduced in Cyrus-SASL not too long ago,
luckily it didn't make it into a release version).

Unfortunately a lot of C code is really ugly and "assembles" some
string piece by piece which makes it hard to keep track of the size
that is still available (it took me a long time to convert some
large software to use only snprintf() "thanks" to the way it handled
buffers). This experience caused me to add some functionality to
an snprintf() replacement: similar to having strlcpy() and strlcat()
you have snprintf() and snprintf_append(). So instead of code like:

   r1 = snprintf(str, str_size, ...)
   check overflow...
   r2 = snprintf(str + r1, str_size - r1, ...)
   check overflow...

you use

   str[0] = '\0; /* clear string */
   r1 = snprintf_append(str, str_size, ...)
   check overflow...
   r2 = snprintf_append(str, str_size, ...)
   check overflow...

(that's the basic idea, the real implementation looks a bit different).