vfprintf typing problem

[Carlos O'Donell]

On Fri, Mar 30, 2012 at 4:18 AM, David Miller <davem@davemloft.net> wrote:
> I propose the following patch.
>
> At the points where the "(size_t) -1 / sizeof(CHAT_T)" checks exist
> the 'width' has been "normalized" to be positive.  If it was negative,
> that state has been recorded in the 'left' variable.
>
> So we can just directly sanity check whether width (plus the 32-bytes
> of slack) is >= INT_MAX.
>
> Therefore we have two levels of protection in this code:
>
> 1) Individual field widths are validated against INT_MAX
>
> 2) The total result (stored in 'done') is validated against
>   overflowing INT_MAX as it accumulates, via the done_add() macro.
>
> I've also adjusted bug22.c so that it tests both of those cases.
>
> 2012-03-30  David S. Miller  <davem@davemloft.net>
>
>        * stdio-common/vfprintf.c (vfprintf): Validate width against
>        overflow of INT_MAX.  Set errno to EOVERFLOW when 'done' overflows
>        INT_MAX.
>        * stdio-common/bug22.c: Adjust to test both width INT_MAX overflow
>        as well as total length INT_MAX overflow.  Check explicitly for
>        proper errno values.
>
> diff --git a/stdio-common/bug22.c b/stdio-common/bug22.c
> index 2228388..6b6bb43 100644
> --- a/stdio-common/bug22.c
> +++ b/stdio-common/bug22.c
> @@ -1,12 +1,18 @@
>  /* BZ #5424 */
>  #include <stdio.h>
> +#include <errno.h>
>
> +/* INT_MAX + 1 */
>  #define N 2147483648
>
> +/* (INT_MAX / 2) + 2 */
> +#define N2 1073741825
> +
>  #define STRINGIFY(S) #S
>  #define MAKE_STR(S) STRINGIFY(S)
>
>  #define SN MAKE_STR(N)
> +#define SN2 MAKE_STR(N2)
>
>  static int
>  do_test (void)
> @@ -20,11 +26,15 @@ do_test (void)
>       return 1;
>     }
>
> -  ret = fprintf (fp, "%" SN "d%" SN "d", 1, 1);
> +  ret = fprintf (fp, "%" SN "d", 1);
> +  printf ("ret = %d\n", ret);
> +  if (ret != -1 || errno != ERANGE)
> +         return 1;
>
> +  ret = fprintf (fp, "%" SN2 "d%" SN2 "d", 1, 1);
>   printf ("ret = %d\n", ret);
>
> -  return ret != -1;
> +  return ret != -1 || errno != EOVERFLOW;
>  }
>
>  #define TIMEOUT 30
> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index 1e90483..4875fe0 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -71,6 +71,7 @@
>                          < _val, 0))                                         \
>       {                                                                              \
>        done = -1;                                                            \
> +        __set_errno (EOVERFLOW);                                             \
>        goto all_done;                                                        \
>       }                                                                              \
>     done += _val;                                                            \
> @@ -141,12 +142,18 @@
>   do                                                                         \
>     {                                                                        \
>       assert ((size_t) done <= (size_t) INT_MAX);                            \
> -      if ((size_t) PUT (s, (String), (Len)) != (size_t) (Len)                \
> -         || (size_t) INT_MAX - (size_t) done < (size_t) (Len))               \
> +      if ((size_t) PUT (s, (String), (Len)) != (size_t) (Len))               \
>        {                                                                     \
>          done = -1;                                                          \
>          goto all_done;                                                      \
>        }                                                                     \
> +    if (__builtin_expect ((unsigned int) INT_MAX - (unsigned int) done       \
> +                         < (Len), 0))                                        \
> +      {                                                                              \
> +       done = -1;                                                            \
> +        __set_errno (EOVERFLOW);                                             \
> +       goto all_done;                                                        \
> +      }                                                                              \
>       done += (Len);                                                         \
>     }                                                                        \
>   while (0)
> @@ -1449,7 +1456,8 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
>            left = 1;
>          }
>
> -       if (__builtin_expect (width >= (size_t) -1 / sizeof (CHAR_T) - 32, 0))
> +       if (__builtin_expect ((unsigned int) width >= INT_MAX
> +                             / sizeof (CHAR_T) - 32, 0))
>          {
>            __set_errno (ERANGE);
>            done = -1;
> @@ -1481,7 +1489,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
>     LABEL (width):
>       width = read_int (&f);
>
> -      if (__builtin_expect (width >= (size_t) -1 / sizeof (CHAR_T) - 32, 0))
> +      if (__builtin_expect ((unsigned int) width >= INT_MAX / sizeof (CHAR_T) - 32, 0))
>        {
>          __set_errno (ERANGE);
>          done = -1;

Thanks for slogging through this, the patch is looking good so far.

Could you explain why we don't also need to fix the same problem with `prec'?

Cheers,
Carlos.