Re: [bug-gnulib] snprintf fixes in gnulib

[Bruno Haible]

Paul Eggert wrote:
> The first thing I noticed was this comment about two size_t variables:
> 
>     if (len > size - 1) /* equivalent to: (size > 0 && len >= size) */
> 
> The comment is not correct if size_t promotes to int

This is a border case; you can fix it by adding a cast:

      if (len > (size_t)(size - 1))

> and anyway the
> code would be clearer and typically just as fast if it were written if
> (0 < size && size <= len) since compilers typically optimize this into
> a single comparison these days.

No, they don't. Take "gcc-4.1.1 -O2" on this test file:

===================== foo.c =====================
typedef unsigned int size_t;
int single_cond_jump (size_t len, size_t size)
{
  if (len > size - 1)
    return len;
  else
    return -1;
}
int two_cond_jumps (size_t len, size_t size)
{
  if (size > 0 && len >= size)
    return len;
  else
    return -1;
}
===================== foo.s =======================
        .file   "foo.c"
        .text
        .p2align 4,,15
.globl single_cond_jump
        .type   single_cond_jump, @function
single_cond_jump:
        pushl   %ebp
        movl    $-1, %edx
        movl    %esp, %ebp
        movl    12(%ebp), %eax
        movl    8(%ebp), %ecx
        decl    %eax
        cmpl    %ecx, %eax
        jae     .L4
        movl    %ecx, %edx
.L4:
        popl    %ebp
        movl    %edx, %eax
        ret
        .size   single_cond_jump, .-single_cond_jump
        .p2align 4,,15
.globl two_cond_jumps
        .type   two_cond_jumps, @function
two_cond_jumps:
        pushl   %ebp
        movl    %esp, %ebp
        movl    12(%ebp), %edx
        testl   %edx, %edx
        je      .L10
        movl    8(%ebp), %eax
        cmpl    %eax, %edx
        ja      .L10
        popl    %ebp
        ret
        .p2align 4,,7
.L10:
        popl    %ebp
        movl    $-1, %eax
        .p2align 4,,4
        ret
        .size   two_cond_jumps, .-two_cond_jumps
        .ident  "GCC: (GNU) 4.1.1"
        .section        .note.GNU-stack,"",@progbits
============================================================

You see the that gcc turns the two comparisons into two conditional jumps.
It is well-known that jumps affect performance a lot on modern CPUs.

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.

> However, when I looked into the surrounding code I noticed that the
> logic was wrong anyway, since it didn't properly copy the generated
> string into the output buffer when the string is too long.

Right, this is a major bug. Thanks for noticing it!

> While we're on the subject, snprintf should
> check for EOVERFLOW, since it's the interface that stupidly tries to
> copy a size_t into an int, so I changed it to do that.

This is not needed. vasnprintf already does this check.

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

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.

Simon, ok to apply this?

*** snprintf.c  10 Aug 2006 19:32:38 -0000      1.8
--- snprintf.c  11 Aug 2006 13:43:12 -0000
***************
*** 1,6 ****
  /* Formatted output to strings.
     Copyright (C) 2004, 2006 Free Software Foundation, Inc.
!    Written by Simon Josefsson and Paul Eggert.
  
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
--- 1,6 ----
  /* Formatted output to strings.
     Copyright (C) 2004, 2006 Free Software Foundation, Inc.
!    Written by Simon Josefsson, Paul Eggert, Bruno Haible.
  
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
***************
*** 22,40 ****
  
  #include "snprintf.h"
  
- #include <errno.h>
- #include <limits.h>
  #include <stdarg.h>
  #include <stdlib.h>
  #include <string.h>
  
  #include "vasnprintf.h"
  
- /* Some systems, like OSF/1 4.0 and Woe32, don't have EOVERFLOW.  */
- #ifndef EOVERFLOW
- # define EOVERFLOW E2BIG
- #endif
- 
  /* Print formatted output to string STR.  Similar to sprintf, but
     additional length SIZE limit how much is written into STR.  Returns
     string length of formatted string (which may be larger than SIZE).
--- 22,33 ----
***************
*** 57,76 ****
  
    if (output != str)
      {
!       if (size)
        {
!         memcpy (str, output, size - 1);
!         str[size - 1] = '\0';
        }
  
        free (output);
      }
  
!   if (INT_MAX < len)
!     {
!       errno = EOVERFLOW;
!       return -1;
!     }
  
    return len;
  }
--- 50,71 ----
  
    if (output != str)
      {
!       /* output was malloc()ed by the vasnprintf function.  */
! 
!       if (str != NULL && size > 0)
        {
!         /* Copy contents of output to str.  */
!         size_t pruned_len = (len >= size ? size - 1 : len);
! 
!         memcpy (str, output, pruned_len);
!         str[pruned_len] = '\0';
        }
  
        free (output);
      }
  
!   /* No need to test against len > INT_MAX.  vasnprintf already handled
!      this case.  */
  
    return len;
  }