diff mbox

vfprintf: Use struct scratch_buffer for positional arguments allocation

Message ID 20170619161949.12053402AEC0E@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer June 19, 2017, 4:19 p.m. UTC
2017-06-19  Florian Weimer  <fweimer@redhat.com>

	* stdio-common/vfprintf.c (printf_positional): Use struct
	scratch_buffer to allocate backing storage for the args_value,
	args_size, args_type arrays.

Comments

Adhemerval Zanella Netto June 27, 2017, 6:07 p.m. UTC | #1
On 19/06/2017 13:19, Florian Weimer wrote:
> 2017-06-19  Florian Weimer  <fweimer@redhat.com>
> 
> 	* stdio-common/vfprintf.c (printf_positional): Use struct
> 	scratch_buffer to allocate backing storage for the args_value,
> 	args_size, args_type arrays.
> 


> -  /* Set up the remaining two arrays to each point past the end of the
> -     prior array, since space for all three has been allocated now.  */
> -  args_size = &args_value[nargs].pa_int;
> -  args_type = &args_size[nargs];
> -  memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
> -	  nargs * sizeof (*args_type));
> +  union printf_arg *args_value;
> +  int *args_size;
> +  int *args_type;
> +  {
> +    /* Calculate total size needed to represent a single argument
> +       across all three argument-related arrays.  */
> +    size_t bytes_per_arg
> +      = sizeof (*args_value) + sizeof (*args_size) + sizeof (*args_type);

LGTM, although I do not see much gain for using brackets just for 
bytes_per_arg.

> +    if (!scratch_buffer_set_array_size (&argsbuf, nargs, bytes_per_arg))
> +      {
> +	done = -1;
> +	goto all_done;
> +      }
> +    args_value = argsbuf.data;
> +    /* Set up the remaining two arrays to each point past the end of
> +       the prior array, since space for all three has been allocated
> +       now.  */
> +    args_size = &args_value[nargs].pa_int;
> +    args_type = &args_size[nargs];
> +    memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
> +	    nargs * sizeof (*args_type));
> +  }
>  
>    /* XXX Could do sanity check here: If any element in ARGS_TYPE is
>       still zero after this loop, format is invalid.  For now we
> @@ -1966,8 +1955,7 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
>  		 - specs[nspecs_done].end_of_fmt);
>      }
>   all_done:
> -  if (__glibc_unlikely (args_malloced != NULL))
> -    free (args_malloced);
> +  scratch_buffer_free (&argsbuf);
>    scratch_buffer_free (&specsbuf);
>    return done;
>  }
>
diff mbox

Patch

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 76614fc..f0ea4fe 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -1627,15 +1627,17 @@  printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
 		   CHAR_T *work_buffer, int save_errno,
 		   const char *grouping, THOUSANDS_SEP_T thousands_sep)
 {
-  /* For the argument descriptions, which may be allocated on the heap.  */
-  void *args_malloced = NULL;
-
   /* For positional argument handling.  */
   struct scratch_buffer specsbuf;
   scratch_buffer_init (&specsbuf);
   struct printf_spec *specs = specsbuf.data;
   size_t specs_limit = specsbuf.length / sizeof (specs[0]);
 
+  /* Used as a backing store for args_value, args_size, args_type
+     below.  */
+  struct scratch_buffer argsbuf;
+  scratch_buffer_init (&argsbuf);
+
   /* Array with information about the needed arguments.  This has to
      be dynamically extensible.  */
   size_t nspecs = 0;
@@ -1644,10 +1646,6 @@  printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
      determine the size of the array needed to store the argument
      attributes.  */
   size_t nargs = 0;
-  size_t bytes_per_arg;
-  union printf_arg *args_value;
-  int *args_size;
-  int *args_type;
 
   /* Positional parameters refer to arguments directly.  This could
      also determine the maximum number of arguments.  Track the
@@ -1695,38 +1693,29 @@  printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
 
   /* Determine the number of arguments the format string consumes.  */
   nargs = MAX (nargs, max_ref_arg);
-  /* Calculate total size needed to represent a single argument across
-     all three argument-related arrays.  */
-  bytes_per_arg = (sizeof (*args_value) + sizeof (*args_size)
-		   + sizeof (*args_type));
 
-  /* Check for potential integer overflow.  */
-  if (__glibc_unlikely (nargs > INT_MAX / bytes_per_arg))
-    {
-      __set_errno (EOVERFLOW);
-      done = -1;
-      goto all_done;
-    }
-
-  /* Allocate memory for all three argument arrays.  */
-  if (__libc_use_alloca (nargs * bytes_per_arg))
-    args_value = alloca (nargs * bytes_per_arg);
-  else
-    {
-      args_value = args_malloced = malloc (nargs * bytes_per_arg);
-      if (args_value == NULL)
-	{
-	  done = -1;
-	  goto all_done;
-	}
-    }
-
-  /* Set up the remaining two arrays to each point past the end of the
-     prior array, since space for all three has been allocated now.  */
-  args_size = &args_value[nargs].pa_int;
-  args_type = &args_size[nargs];
-  memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
-	  nargs * sizeof (*args_type));
+  union printf_arg *args_value;
+  int *args_size;
+  int *args_type;
+  {
+    /* Calculate total size needed to represent a single argument
+       across all three argument-related arrays.  */
+    size_t bytes_per_arg
+      = sizeof (*args_value) + sizeof (*args_size) + sizeof (*args_type);
+    if (!scratch_buffer_set_array_size (&argsbuf, nargs, bytes_per_arg))
+      {
+	done = -1;
+	goto all_done;
+      }
+    args_value = argsbuf.data;
+    /* Set up the remaining two arrays to each point past the end of
+       the prior array, since space for all three has been allocated
+       now.  */
+    args_size = &args_value[nargs].pa_int;
+    args_type = &args_size[nargs];
+    memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
+	    nargs * sizeof (*args_type));
+  }
 
   /* XXX Could do sanity check here: If any element in ARGS_TYPE is
      still zero after this loop, format is invalid.  For now we
@@ -1966,8 +1955,7 @@  printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
 		 - specs[nspecs_done].end_of_fmt);
     }
  all_done:
-  if (__glibc_unlikely (args_malloced != NULL))
-    free (args_malloced);
+  scratch_buffer_free (&argsbuf);
   scratch_buffer_free (&specsbuf);
   return done;
 }