[PATCH v3] vfprintf-internal: Replace alloca with malloc.

[PATCH v3] vfprintf-internal: Replace alloca with malloc.

[Joe Simmons-Talbott]

Avoid potential stack overflow from unbounded alloca.
---
 stdio-common/vfprintf-internal.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
index c76c06e49b..2d134ea08f 100644
--- a/stdio-common/vfprintf-internal.c
+++ b/stdio-common/vfprintf-internal.c
@@ -1001,6 +1001,7 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
   scratch_buffer_init (&specsbuf);
   struct printf_spec *specs = specsbuf.data;
   size_t specs_limit = specsbuf.length / sizeof (specs[0]);
+  bool malloced_pa_user = false;
 
   /* Used as a backing store for args_value, args_size, args_type
      below.  */
@@ -1171,7 +1172,10 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
 	else if (__glibc_unlikely (__printf_va_arg_table != NULL)
 		 && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
 	  {
-	    args_value[cnt].pa_user = alloca (args_size[cnt]);
+	    args_value[cnt].pa_user = malloc (args_size[cnt]);
+	    if (args_value[cnt].pa_user == NULL)
+	       break;
+	    malloced_pa_user = true;
 	    (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
 	      (args_value[cnt].pa_user, ap_savep);
 	  }
@@ -1335,6 +1339,9 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
 			       - specs[nspecs_done].end_of_fmt));
     }
  all_done:
+  if (malloced_pa_user)
+    for (cnt = 0; cnt < nargs; ++cnt)
+      free (args_value[cnt].pa_user);
   scratch_buffer_free (&argsbuf);
   scratch_buffer_free (&specsbuf);
 }
-- 
2.39.2

Re: [PATCH v3] vfprintf-internal: Replace alloca with malloc.

[Paul Eggert]

On 2023-05-15 11:50, Joe Simmons-Talbott via Libc-alpha wrote:
> +	    args_value[cnt].pa_user = malloc (args_size[cnt]);
> +	    if (args_value[cnt].pa_user == NULL)
> +	       break;

Shouldn't an error be returned if a printf function runs out of memory 
internally?

Also, that function already uses a scratch buffer; why not grow the 
scratch buffer instead of calling malloc separately?

Re: [PATCH v3] vfprintf-internal: Replace alloca with malloc.

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Wed, May 17, 2023 at 2:10 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

> On 2023-05-15 11:50, Joe Simmons-Talbott via Libc-alpha wrote:
> > +         args_value[cnt].pa_user = malloc (args_size[cnt]);
> > +         if (args_value[cnt].pa_user == NULL)
> > +            break;
>
> Shouldn't an error be returned if a printf function runs out of memory
> internally?
>

Yes.


>
> Also, that function already uses a scratch buffer; why not grow the
> scratch buffer instead of calling malloc separately?
>

I was going to make the suggestion.. scratch_buffer all things possible
since afair it was built specifically to replace most if not all the
__use_alloca thingies.

Re: [PATCH v3] vfprintf-internal: Replace alloca with malloc.

[Joe Simmons-Talbott]

On Wed, May 17, 2023 at 11:08:39AM -0700, Paul Eggert wrote:
> On 2023-05-15 11:50, Joe Simmons-Talbott via Libc-alpha wrote:
> > +	    args_value[cnt].pa_user = malloc (args_size[cnt]);
> > +	    if (args_value[cnt].pa_user == NULL)
> > +	       break;
> 
> Shouldn't an error be returned if a printf function runs out of memory
> internally?

I'll fix this in the next version.  Thanks.

> 
> Also, that function already uses a scratch buffer; why not grow the scratch
> buffer instead of calling malloc separately?
> 

It seems to me that the code to grow the scratch buffer would be more
difficult to understand than calling malloc separately. Perhaps I'm just
intimidated by the pointer math.

Thanks,
Joe

Re: [PATCH v3] vfprintf-internal: Replace alloca with malloc.

[Paul Eggert]

On 2023-05-17 12:41, Joe Simmons-Talbott wrote:
> It seems to me that the code to grow the scratch buffer would be more
> difficult to understand than calling malloc separately.

True, but in the normal case this means malloc will not be called (as 
the data will live in a small scratch buffer on the stack), and that's a 
win worth striving for.

Re: [PATCH v3] vfprintf-internal: Replace alloca with malloc.

[Joe Simmons-Talbott]

On Wed, May 17, 2023 at 01:07:58PM -0700, Paul Eggert wrote:
> On 2023-05-17 12:41, Joe Simmons-Talbott wrote:
> > It seems to me that the code to grow the scratch buffer would be more
> > difficult to understand than calling malloc separately.
> 
> True, but in the normal case this means malloc will not be called (as the
> data will live in a small scratch buffer on the stack), and that's a win
> worth striving for.
> 

I've posted a new patch with my attempt at reusing the scratch_buffer.
[1]

Thanks,
Joe

[1] https://sourceware.org/pipermail/libc-alpha/2023-May/148232.html