Tighten up vfprintf width, precision, and total length overflow handling.
authorDavid S. Miller <davem@davemloft.net>
Mon, 2 Apr 2012 21:31:19 +0000 (14:31 -0700)
committerDavid S. Miller <davem@davemloft.net>
Mon, 2 Apr 2012 21:31:19 +0000 (14:31 -0700)
With help from Paul Eggert, Carlos O'Donell, and Roland McGrath.
* stdio-common/printf-parse.h (read_int): Change return type to
'int', return -1 on INT_MAX overflow.
* stdio-common/vfprintf.c (vfprintf): Validate width and precision
against overflow of INT_MAX.  Set errno to EOVERFLOW when 'done'
overflows INT_MAX.  Check for overflow of in-format-string precision
values properly.  Use EOVERFLOW rather than ERANGE throughout.  Use
SIZE_MAX not INT_MAX for integer overflow test.
* stdio-common/printf-parsemb.c: If read_int signals an overflow,
skip the construct in the format string but do not record anything.
* stdio-common/bug22.c: Adjust to test both width/prevision
INT_MAX overflow as well as total length INT_MAX overflow.  Check
explicitly for proper errno values.

ChangeLog
stdio-common/bug22.c
stdio-common/printf-parse.h
stdio-common/printf-parsemb.c
stdio-common/vfprintf.c

index ca85e54..8705429 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2012-04-02  David S. Miller  <davem@davemloft.net>
+
+       With help from Paul Eggert, Carlos O'Donell, and Roland McGrath.
+       * stdio-common/printf-parse.h (read_int): Change return type to
+       'int', return -1 on INT_MAX overflow.
+       * stdio-common/vfprintf.c (vfprintf): Validate width and precision
+       against overflow of INT_MAX.  Set errno to EOVERFLOW when 'done'
+       overflows INT_MAX.  Check for overflow of in-format-string precision
+       values properly.  Use EOVERFLOW rather than ERANGE throughout.  Use
+       SIZE_MAX not INT_MAX for integer overflow test.
+       * stdio-common/printf-parsemb.c: If read_int signals an overflow,
+       skip the construct in the format string but do not record anything.
+       * stdio-common/bug22.c: Adjust to test both width/prevision
+       INT_MAX overflow as well as total length INT_MAX overflow.  Check
+       explicitly for proper errno values.
+
 2012-04-02  Thomas Schwinge  <thomas@codesourcery.com>
 
        * string/test-memcmp.c [! WIDE]: #include <limits.h> for CHAR_MIN,
index 2228388..efd9501 100644 (file)
@@ -1,12 +1,22 @@
 /* BZ #5424 */
 #include <stdio.h>
+#include <errno.h>
 
+/* INT_MAX + 1 */
 #define N 2147483648
 
+/* (INT_MAX / 2) + 2 */
+#define N2 1073741825
+
+/* INT_MAX - 3 */
+#define N3 2147483644
+
 #define STRINGIFY(S) #S
 #define MAKE_STR(S) STRINGIFY(S)
 
 #define SN MAKE_STR(N)
+#define SN2 MAKE_STR(N2)
+#define SN3 MAKE_STR(N3)
 
 static int
 do_test (void)
@@ -20,11 +30,25 @@ 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 != EOVERFLOW)
+         return 1;
+
+  ret = fprintf (fp, "%." SN "d", 1);
+  printf ("ret = %d\n", ret);
+  if (ret != -1 || errno != EOVERFLOW)
+         return 1;
+
+  ret = fprintf (fp, "%." SN3 "d", 1);
+  printf ("ret = %d\n", ret);
+  if (ret != -1 || errno != EOVERFLOW)
+         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
index 72665dc..3aa0274 100644 (file)
@@ -68,16 +68,27 @@ union printf_arg
 #ifndef DONT_NEED_READ_INT
 /* Read a simple integer from a string and update the string pointer.
    It is assumed that the first character is a digit.  */
-static unsigned int
+static int
 read_int (const UCHAR_T * *pstr)
 {
-  unsigned int retval = **pstr - L_('0');
+  int retval = **pstr - L_('0');
 
   while (ISDIGIT (*++(*pstr)))
-    {
-      retval *= 10;
-      retval += **pstr - L_('0');
-    }
+    if (retval >= 0)
+      {
+       if (INT_MAX / 10 < retval)
+         retval = -1;
+       else
+         {
+           int digit = **pstr - L_('0');
+
+           retval *= 10;
+           if (INT_MAX - digit < retval)
+             retval = -1;
+           else
+             retval += digit;
+         }
+      }
 
   return retval;
 }
index 2bdb5e6..a45ac74 100644 (file)
@@ -87,12 +87,15 @@ __parse_one_specmb (const UCHAR_T *format, size_t posn,
 
       n = read_int (&format);
 
-      if (n > 0 && *format == L_('$'))
+      if (n != 0 && *format == L_('$'))
        /* Is positional parameter.  */
        {
          ++format;             /* Skip the '$'.  */
-         spec->data_arg = n - 1;
-         *max_ref_arg = MAX (*max_ref_arg, n);
+         if (n != -1)
+           {
+             spec->data_arg = n - 1;
+             *max_ref_arg = MAX (*max_ref_arg, n);
+           }
        }
       else
        /* Oops; that was actually the width and/or 0 padding flag.
@@ -160,10 +163,13 @@ __parse_one_specmb (const UCHAR_T *format, size_t posn,
          /* The width argument might be found in a positional parameter.  */
          n = read_int (&format);
 
-         if (n > 0 && *format == L_('$'))
+         if (n != 0 && *format == L_('$'))
            {
-             spec->width_arg = n - 1;
-             *max_ref_arg = MAX (*max_ref_arg, n);
+             if (n != -1)
+               {
+                 spec->width_arg = n - 1;
+                 *max_ref_arg = MAX (*max_ref_arg, n);
+               }
              ++format;         /* Skip '$'.  */
            }
        }
@@ -177,9 +183,13 @@ __parse_one_specmb (const UCHAR_T *format, size_t posn,
        }
     }
   else if (ISDIGIT (*format))
-    /* Constant width specification.  */
-    spec->info.width = read_int (&format);
+    {
+      int n = read_int (&format);
 
+      /* Constant width specification.  */
+      if (n != -1)
+       spec->info.width = n;
+    }
   /* Get the precision.  */
   spec->prec_arg = -1;
   /* -1 means none given; 0 means explicit 0.  */
@@ -196,10 +206,13 @@ __parse_one_specmb (const UCHAR_T *format, size_t posn,
            {
              n = read_int (&format);
 
-             if (n > 0 && *format == L_('$'))
+             if (n != 0 && *format == L_('$'))
                {
-                 spec->prec_arg = n - 1;
-                 *max_ref_arg = MAX (*max_ref_arg, n);
+                 if (n != -1)
+                   {
+                     spec->prec_arg = n - 1;
+                     *max_ref_arg = MAX (*max_ref_arg, n);
+                   }
                  ++format;
                }
            }
@@ -213,7 +226,12 @@ __parse_one_specmb (const UCHAR_T *format, size_t posn,
            }
        }
       else if (ISDIGIT (*format))
-       spec->info.prec = read_int (&format);
+       {
+         int n = read_int (&format);
+
+         if (n != -1)
+           spec->info.prec = n;
+       }
       else
        /* "%.?" is treated like "%.0?".  */
        spec->info.prec = 0;
index 1e90483..463f9c0 100644 (file)
   do {                                                                       \
     unsigned int _val = val;                                                 \
     assert ((unsigned int) done < (unsigned int) INT_MAX);                   \
-    if (__builtin_expect ((unsigned int) INT_MAX - (unsigned int) done       \
-                         < _val, 0))                                         \
+    if (__builtin_expect (INT_MAX - done < _val, 0))                         \
       {                                                                              \
        done = -1;                                                            \
+        __set_errno (EOVERFLOW);                                             \
        goto all_done;                                                        \
       }                                                                              \
     done += _val;                                                            \
   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 (INT_MAX - done < (Len), 0))                              \
+      {                                                                              \
+       done = -1;                                                            \
+        __set_errno (EOVERFLOW);                                             \
+       goto all_done;                                                        \
+      }                                                                              \
       done += (Len);                                                         \
     }                                                                        \
   while (0)
@@ -1435,10 +1440,21 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
        const UCHAR_T *tmp;     /* Temporary value.  */
 
        tmp = ++f;
-       if (ISDIGIT (*tmp) && read_int (&tmp) && *tmp == L_('$'))
-         /* The width comes from a positional parameter.  */
-         goto do_positional;
+       if (ISDIGIT (*tmp))
+         {
+           int pos = read_int (&tmp);
 
+           if (pos == -1)
+             {
+               __set_errno (EOVERFLOW);
+               done = -1;
+               goto all_done;
+             }
+
+           if (pos && *tmp == L_('$'))
+             /* The width comes from a positional parameter.  */
+             goto do_positional;
+         }
        width = va_arg (ap, int);
 
        /* Negative width means left justified.  */
@@ -1449,9 +1465,9 @@ 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 (width >= INT_MAX / sizeof (CHAR_T) - 32, 0))
          {
-           __set_errno (ERANGE);
+           __set_errno (EOVERFLOW);
            done = -1;
            goto all_done;
          }
@@ -1481,9 +1497,10 @@ 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 (width == -1
+                           || width >= INT_MAX / sizeof (CHAR_T) - 32, 0))
        {
-         __set_errno (ERANGE);
+         __set_errno (EOVERFLOW);
          done = -1;
          goto all_done;
        }
@@ -1518,10 +1535,21 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
          const UCHAR_T *tmp;   /* Temporary value.  */
 
          tmp = ++f;
-         if (ISDIGIT (*tmp) && read_int (&tmp) > 0 && *tmp == L_('$'))
-           /* The precision comes from a positional parameter.  */
-           goto do_positional;
+         if (ISDIGIT (*tmp))
+           {
+             int pos = read_int (&tmp);
+
+             if (pos == -1)
+               {
+                 __set_errno (EOVERFLOW);
+                 done = -1;
+                 goto all_done;
+               }
 
+             if (pos && *tmp == L_('$'))
+               /* The precision comes from a positional parameter.  */
+               goto do_positional;
+           }
          prec = va_arg (ap, int);
 
          /* If the precision is negative the precision is omitted.  */
@@ -1529,15 +1557,26 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
            prec = -1;
        }
       else if (ISDIGIT (*f))
-       prec = read_int (&f);
+       {
+         prec = read_int (&f);
+
+         /* The precision was specified in this case as an extremely
+            large positive value.  */
+         if (prec == -1)
+           {
+             __set_errno (EOVERFLOW);
+             done = -1;
+             goto all_done;
+           }
+       }
       else
        prec = 0;
       if (prec > width
          && prec > sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
        {
-         if (__builtin_expect (prec >= (size_t) -1 / sizeof (CHAR_T) - 32, 0))
+         if (__builtin_expect (prec >= INT_MAX / sizeof (CHAR_T) - 32, 0))
            {
-             __set_errno (ERANGE);
+             __set_errno (EOVERFLOW);
              done = -1;
              goto all_done;
            }
@@ -1710,9 +1749,9 @@ do_positional:
                     + sizeof (*args_type));
 
     /* Check for potential integer overflow.  */
-    if (__builtin_expect (nargs > SIZE_MAX / bytes_per_arg, 0))
+    if (__builtin_expect (nargs > INT_MAX / bytes_per_arg, 0))
       {
-        __set_errno (ERANGE);
+        __set_errno (EOVERFLOW);
         done = -1;
         goto all_done;
       }