Re: [bug-gnulib] snprintf fixes in gnulib

[Paul Eggert]

Bruno Haible <address@hidden> writes:

> This is not needed. vasnprintf already does this check.

But my next bug report was going to be for vasnprintf, since
vasnprintf should not do the INT_MAX check.  vasnprintf's API does not
suffer from the INT_MAX limit, and there's no need to inflict this
arbitrary limit on vasnprintf, asprintf, etc.  Only functions like
snprintf, where the (poorly-designed, but cast-in-stone) API requires
that the result fit in int, should check for INT_MAX.

The first step to get this fixed is to change snprintf so that it does
the INT_MAX check.  (This step is done in gnulib.)  Assuming you
agree, the next step is to simplify/speedup vasnprintf by removing the
check there.

> gcc converts two comparisons like this to a single conditional jump only
> when both bounds are constant. Not when, like here, one of them is a
> variable.

Thanks, I stand corrected.  (GCC should do better, but that's a bigger task....)

> Still one bug remaining: Your new code will crash when passed an str = NULL
> argument.

No, because if STR is null, then SIZE must be zero.  C99 and POSIX
require this.  So if there is a crash it's a user error.

> Furthermore I don't like that your memcpy copies more than needed.
> The vasnprintf code needs memory in chunks, is not looking for it byte
> for byte. Therefore it can happen that output != str and still
> 0 <= len < size - and then you are copying around more memory than needed.

Thanks, I didn't know that.

I installed the following patch to fix that, since it's the part we
all agree on so far.  I'll await your opinion on the INT_MAX issue
before tackling it.

There's one other optimization that I thought of while reviewing this
change: don't take the address of LEN, so that the compiler can put
LEN into a register.  This shrinks the size of the generated machine
code by 10% on my Debian x86 host with GCC 4.1.1 -O2.  The following
patch incorporates that optimization too.

2006-08-11  Paul Eggert  <address@hidden>

        * lib/snprintf.c (snprintf): memcpy LEN bytes, not SIZE - 1, when
        LEN is smaller than SIZE.  Suggested by Bruno Haible.
        Also, help the compiler to keep LEN in a register.

--- lib/snprintf.c      10 Aug 2006 19:32:38 -0000      1.8
+++ lib/snprintf.c      11 Aug 2006 17:47:58 -0000      1.10
@@ -45,11 +45,12 @@ snprintf (char *str, size_t size, const 
 {
   char *output;
   size_t len;
+  size_t lenbuf = size;
   va_list args;
 
   va_start (args, format);
-  len = size;
-  output = vasnprintf (str, &len, format, args);
+  output = vasnprintf (str, &lenbuf, format, args);
+  len = lenbuf;
   va_end (args);
 
   if (!output)
@@ -59,8 +60,9 @@ snprintf (char *str, size_t size, const 
     {
       if (size)
        {
-         memcpy (str, output, size - 1);
-         str[size - 1] = '\0';
+         size_t pruned_len = (len < size ? len : size - 1);
+         memcpy (str, output, pruned_len);
+         str[pruned_len] = '\0';
        }
 
       free (output);