Re: [PATCH] stdio-common/vfprintf.c: Remove magic number 32.

[Florian Weimer <fweimer at redhat dot com>]

On 05/04/2017 06:26 PM, Carlos O'Donell wrote:
> I was looking at modifying some of this code for a bug I was working
> on and it was super annoying that the 32 character buffer is hardcoded
> everywhere. This patch simply hoists this into a macro EXTSIZ.
> 
> I'll check this in within 24h if nobody objects.
> 
> 2017-05-04  Carlos O'Donell  <carlos@redhat.com>
> 
> 	* stdio-common/vfprintf.c (EXTSIZ): Define.
> 	(vfprintf): Use EXTSIZ.
> 	(printf_positional): Likewise.

I looked at this and discovered that since 1999, we no longer use the
work buffer for padding purposes, so resizing it based on field width is
no longer needed.  The attached patch removes the resizing logic.

We could probably reduce the size of the work buffer, too, and we should
have some checks that a funny locale does not produce number formatting
rules which exceed the buffer size.

(Floating point formatting does not use the work buffer.)

Thanks,
Florian

vfprintf: Do not resize work buffer based on width/precision

Since commit 3e95f6602b226e0de06aaff686dc47b282d7cc16 (Remove
limitation on size of precision for integers), padding is no
longer written to the work buffer, only the number itself
(and its transformations), so a statically sized buffer is
sufficient.

This commit does not remove existing overflow checks which
lead to early failures, but it adjusts them not to use EXTSIZE.
This causes one of the EOVERFLOW tests in stdio-comment/bug22 to
pass.  This commit adjust it so that it fails again.

2017-05-30  Florian Weimer  <fweimer@redhat.com>

	* stdio-common/vfprintf.c (EXTSIZ): Remove definition.
	(WORK_BUFFER_SIZE): Expand comment.
	(vfprintf): Remove workstart.  Turn workend into a constant.
	Remove work buffer resizing.
	(printf_positional): Likewise.
	* stdio-common/bug22.c: Convert to <support/test-driver.c>.
	(do_test): Adjust SN3 test to trigger the overflow again.

diff --git a/stdio-common/bug22.c b/stdio-common/bug22.c
index b26399a..38af20c 100644
--- a/stdio-common/bug22.c
+++ b/stdio-common/bug22.c
@@ -1,6 +1,7 @@
 /* BZ #5424 */
 #include <stdio.h>
 #include <errno.h>
+#include <support/check.h>
 
 /* INT_MAX + 1 */
 #define N 2147483648
@@ -32,25 +33,26 @@ do_test (void)
 
   ret = fprintf (fp, "%" SN "d", 1);
   printf ("ret = %d\n", ret);
-  if (ret != -1 || errno != EOVERFLOW)
-	  return 1;
+  TEST_VERIFY (ret == -1);
+  TEST_VERIFY (errno == EOVERFLOW);
 
   ret = fprintf (fp, "%." SN "d", 1);
   printf ("ret = %d\n", ret);
-  if (ret != -1 || errno != EOVERFLOW)
-	  return 1;
+  TEST_VERIFY (ret == -1);
+  TEST_VERIFY (errno == EOVERFLOW);
 
-  ret = fprintf (fp, "%." SN3 "d", 1);
+  ret = fprintf (fp, "%." SN3 "d%4d", 1, 2);
   printf ("ret = %d\n", ret);
-  if (ret != -1 || errno != EOVERFLOW)
-	  return 1;
+  TEST_VERIFY (ret == -1);
+  TEST_VERIFY (errno == EOVERFLOW);
 
   ret = fprintf (fp, "%" SN2 "d%" SN2 "d", 1, 1);
   printf ("ret = %d\n", ret);
+  TEST_VERIFY (ret == -1);
+  TEST_VERIFY (errno == EOVERFLOW);
 
-  return ret != -1 || errno != EOVERFLOW;
+  return 0;
 }
 
 #define TIMEOUT 60
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 2cf7c8a..b179b20 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -45,10 +45,6 @@
 #define va_list	_IO_va_list
 #undef BUFSIZ
 #define BUFSIZ		_IO_BUFSIZ
-/* In some cases we need extra space for all the output which is not
-   counted in the width of the string. We assume 32 characters is
-   enough.  */
-#define EXTSIZ		32
 #define ARGCHECK(S, Format) \
   do									      \
     {									      \
@@ -204,7 +200,9 @@ typedef wchar_t THOUSANDS_SEP_T;
 /* Global constants.  */
 static const CHAR_T null[] = L_("(null)");
 
-/* Size of the work_buffer variable (in characters, not bytes.  */
+/* Size of the work_buffer variable (in characters, not bytes.  This
+   must be large enough to store all formatted integers, including
+   grouping and alternative digits, but excluding padding.  */
 enum { WORK_BUFFER_SIZE = 1000 };
 
 /* This table maps a character into a number representing a class.  In
@@ -1258,8 +1256,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 
   /* Buffer intermediate results.  */
   CHAR_T work_buffer[WORK_BUFFER_SIZE];
-  CHAR_T *workstart = NULL;
-  CHAR_T *workend;
+  CHAR_T *const workend = work_buffer + WORK_BUFFER_SIZE;
 
   /* We have to save the original argument pointer.  */
   va_list ap_save;
@@ -1367,9 +1364,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       UCHAR_T pad = L_(' ');/* Padding character.  */
       CHAR_T spec;
 
-      workstart = NULL;
-      workend = work_buffer + WORK_BUFFER_SIZE;
-
       /* Get current character in format string.  */
       JUMP (*++f, step0_jumps);
 
@@ -1460,62 +1454,25 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	    left = 1;
 	  }
 
-	if (__glibc_unlikely (width >= INT_MAX / sizeof (CHAR_T) - EXTSIZ))
+	if (__glibc_unlikely (width >= INT_MAX / sizeof (CHAR_T)))
 	  {
 	    __set_errno (EOVERFLOW);
 	    done = -1;
 	    goto all_done;
 	  }
-
-	if (width >= WORK_BUFFER_SIZE - EXTSIZ)
-	  {
-	    /* We have to use a special buffer.  */
-	    size_t needed = ((size_t) width + EXTSIZ) * sizeof (CHAR_T);
-	    if (__libc_use_alloca (needed))
-	      workend = (CHAR_T *) alloca (needed) + width + EXTSIZ;
-	    else
-	      {
-		workstart = (CHAR_T *) malloc (needed);
-		if (workstart == NULL)
-		  {
-		    done = -1;
-		    goto all_done;
-		  }
-		workend = workstart + width + EXTSIZ;
-	      }
-	  }
       }
       JUMP (*f, step1_jumps);
 
       /* Given width in format string.  */
     LABEL (width):
       width = read_int (&f);
-
-      if (__glibc_unlikely (width == -1
-			    || width >= INT_MAX / sizeof (CHAR_T) - EXTSIZ))
+      if (__glibc_unlikely (width == -1 || width >= INT_MAX / sizeof (CHAR_T)))
 	{
 	  __set_errno (EOVERFLOW);
 	  done = -1;
 	  goto all_done;
 	}
 
-      if (width >= WORK_BUFFER_SIZE - EXTSIZ)
-	{
-	  /* We have to use a special buffer.  */
-	  size_t needed = ((size_t) width + EXTSIZ) * sizeof (CHAR_T);
-	  if (__libc_use_alloca (needed))
-	    workend = (CHAR_T *) alloca (needed) + width + EXTSIZ;
-	  else
-	    {
-	      workstart = (CHAR_T *) malloc (needed);
-	      if (workstart == NULL)
-		{
-		  done = -1;
-		  goto all_done;
-		}
-	      workend = workstart + width + EXTSIZ;
-	    }
-	}
       if (*f == L_('$'))
 	/* Oh, oh.  The argument comes from a positional parameter.  */
 	goto do_positional;
@@ -1564,34 +1521,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	}
       else
 	prec = 0;
-      if (prec > width && prec > WORK_BUFFER_SIZE - EXTSIZ)
-	{
-	  /* Deallocate any previously allocated buffer because it is
-	     too small.  */
-	  if (__glibc_unlikely (workstart != NULL))
-	    free (workstart);
-	  workstart = NULL;
-	  if (__glibc_unlikely (prec >= INT_MAX / sizeof (CHAR_T) - EXTSIZ))
-	    {
-	      __set_errno (EOVERFLOW);
-	      done = -1;
-	      goto all_done;
-	    }
-	  size_t needed = ((size_t) prec + EXTSIZ) * sizeof (CHAR_T);
-
-	  if (__libc_use_alloca (needed))
-	    workend = (CHAR_T *) alloca (needed) + prec + EXTSIZ;
-	  else
-	    {
-	      workstart = (CHAR_T *) malloc (needed);
-	      if (workstart == NULL)
-		{
-		  done = -1;
-		  goto all_done;
-		}
-	      workend = workstart + prec + EXTSIZ;
-	    }
-	}
       JUMP (*f, step2_jumps);
 
       /* Process 'h' modifier.  There might another 'h' following.  */
@@ -1655,10 +1584,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       /* The format is correctly handled.  */
       ++nspecs_done;
 
-      if (__glibc_unlikely (workstart != NULL))
-	free (workstart);
-      workstart = NULL;
-
       /* Look for next format specifier.  */
 #ifdef COMPILE_WPRINTF
       f = __find_specwc ((end_of_spec = ++f));
@@ -1676,18 +1601,11 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 
   /* Hand off processing for positional parameters.  */
 do_positional:
-  if (__glibc_unlikely (workstart != NULL))
-    {
-      free (workstart);
-      workstart = NULL;
-    }
   done = printf_positional (s, format, readonly_format, ap, &ap_save,
 			    done, nspecs_done, lead_str_end, work_buffer,
 			    save_errno, grouping, thousands_sep);
 
  all_done:
-  if (__glibc_unlikely (workstart != NULL))
-    free (workstart);
   /* Unlock the stream.  */
   _IO_funlockfile (s);
   _IO_cleanup_region_end (0);
@@ -1732,8 +1650,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
   /* Just a counter.  */
   size_t cnt;
 
-  CHAR_T *workstart = NULL;
-
   if (grouping == (const char *) -1)
     {
 #ifdef COMPILE_WPRINTF
@@ -1931,8 +1847,7 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
       char pad = specs[nspecs_done].info.pad;
       CHAR_T spec = specs[nspecs_done].info.spec;
 
-      workstart = NULL;
-      CHAR_T *workend = work_buffer + WORK_BUFFER_SIZE;
+      CHAR_T *const workend = work_buffer + WORK_BUFFER_SIZE;
 
       /* Fill in last information.  */
       if (specs[nspecs_done].width_arg != -1)
@@ -1965,27 +1880,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
 	  prec = specs[nspecs_done].info.prec;
 	}
 
-      /* Maybe the buffer is too small.  */
-      if (MAX (prec, width) + EXTSIZ > WORK_BUFFER_SIZE)
-	{
-	  if (__libc_use_alloca ((MAX (prec, width) + EXTSIZ)
-				 * sizeof (CHAR_T)))
-	    workend = ((CHAR_T *) alloca ((MAX (prec, width) + EXTSIZ)
-					  * sizeof (CHAR_T))
-		       + (MAX (prec, width) + EXTSIZ));
-	  else
-	    {
-	      workstart = (CHAR_T *) malloc ((MAX (prec, width) + EXTSIZ)
-					     * sizeof (CHAR_T));
-	      if (workstart == NULL)
-		{
-		  done = -1;
-		  goto all_done;
-		}
-	      workend = workstart + (MAX (prec, width) + EXTSIZ);
-	    }
-	}
-
       /* Process format specifiers.  */
       while (1)
 	{
@@ -2059,10 +1953,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
 	  break;
 	}
 
-      if (__glibc_unlikely (workstart != NULL))
-	free (workstart);
-      workstart = NULL;
-
       /* Write the following constant string.  */
       outstring (specs[nspecs_done].end_of_fmt,
 		 specs[nspecs_done].next_fmt
@@ -2071,8 +1961,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
  all_done:
   if (__glibc_unlikely (args_malloced != NULL))
     free (args_malloced);
-  if (__glibc_unlikely (workstart != NULL))
-    free (workstart);
   scratch_buffer_free (&specsbuf);
   return done;
 }