vfprintf-internal: Replace alloca with malloc.

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

Hi Joe,

On 5/9/23 19:43, Joe Simmons-Talbott via Libc-alpha wrote:
> Avoid potential stack overflow from unbounded alloca.
> ---
>  stdio-common/vfprintf-internal.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> index c76c06e49b..93f327c6b4 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);
>  	  }
> @@ -1334,6 +1338,13 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
>  			      (specs[nspecs_done].next_fmt
>  			       - specs[nspecs_done].end_of_fmt));
>      }
> +  if (malloced_pa_user)
> +    for (cnt = 0; cnt < nargs; ++cnt)
> +      {
> +         if (args_value[cnt].pa_user != NULL)
> +           free (args_value[cnt].pa_user);

free(NULL) is a no-op since at least C89.  The test is redundant.

Cheers,
Alex

> +      }
> +
>   all_done:
>    scratch_buffer_free (&argsbuf);
>    scratch_buffer_free (&specsbuf);
  

Hi Joe, Adhemerval,

On 5/10/23 12:40, Alejandro Colomar wrote:
> Hi Joe,
> 
> On 5/9/23 19:43, Joe Simmons-Talbott via Libc-alpha wrote:
>> Avoid potential stack overflow from unbounded alloca.
>> ---
>>  stdio-common/vfprintf-internal.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
>> index c76c06e49b..93f327c6b4 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);
>>  	  }
>> @@ -1334,6 +1338,13 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
>>  			      (specs[nspecs_done].next_fmt
>>  			       - specs[nspecs_done].end_of_fmt));
>>      }
>> +  if (malloced_pa_user)
>> +    for (cnt = 0; cnt < nargs; ++cnt)
>> +      {
>> +         if (args_value[cnt].pa_user != NULL)
>> +           free (args_value[cnt].pa_user);
> 
> free(NULL) is a no-op since at least C89.  The test is redundant.

BTW, I thought it might be an optimization to avoid a function call, but
just checked the current (well, a few months outdated, since I didn't
fetch git, but recent enough) glibc for similar cases, and there aren't
many:


--
elf/ldconfig.c-701-      if (opt_chroot != NULL && dir_name != NULL)
elf/ldconfig.c:702:	free (dir_name);
--
elf/ldconfig.c-996-  if (opt_chroot != NULL && dir_name != NULL)
elf/ldconfig.c:997:    free (dir_name);
--
elf/dl-object.c:230:		 Note that free(origin) is OK if origin == NULL.  */
elf/dl-object.c:231:	      free (origin);
--
malloc/set-freeres.c-73-  if (&__ptr != NULL)						\
malloc/set-freeres.c:74:    free (__ptr);
--
misc/tst-ldbl-warn.c-78-  if (stream.buffer != NULL)
misc/tst-ldbl-warn.c:79:    free (stream.buffer);


I found these with a simple grep that checked the line previous to the
free(3).  There may be some more cases that are not so obvious, but I
don't expect many more.  So I guess it's not an optimization thing, or
it would have been done more often.  I can send a patch for removing
all this redundant checks.

BTW, the malloc/set-freeres.c case seems at least weird.  Can an
address ever be NULL?  Maybe, since it's a macro, and if the input
might be something like foo(*NULL), then it would evaluate to &*NULL,
which doesn't dereference it, so the address of *NULL is actually NULL.
But the macro doesn't seem to be called that way, and even then I think
it would be very weird.  It looks more like a typo and (__ptr != NULL)
was more likely intended.  Adhemerval, since you wrote that line, could
you confirm if it's a typo?  Should I fix all of the above to remove
the checks?

Cheers,
Alex

> 
> Cheers,
> Alex
> 
>> +      }
>> +
>>   all_done:
>>    scratch_buffer_free (&argsbuf);
>>    scratch_buffer_free (&specsbuf);
  

On Mai 10 2023, Alejandro Colomar via Libc-alpha wrote:

> BTW, the malloc/set-freeres.c case seems at least weird.  Can an
> address ever be NULL?

Weak references.  So that a static link won't drag in all the world.
  

* Joe Simmons-Talbott via Libc-alpha:

> @@ -1334,6 +1338,13 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
>  			      (specs[nspecs_done].next_fmt
>  			       - specs[nspecs_done].end_of_fmt));
>      }
> +  if (malloced_pa_user)
> +    for (cnt = 0; cnt < nargs; ++cnt)
> +      {
> +         if (args_value[cnt].pa_user != NULL)
> +           free (args_value[cnt].pa_user);
> +      }
> +
>   all_done:
>    scratch_buffer_free (&argsbuf);
>    scratch_buffer_free (&specsbuf);

I think the deallocation code needs to come after the all_done:
label, otherwise memory will leak on errors.

Thanks,
Florian
  

On Wed, May 10, 2023 at 12:40:52PM +0200, Alejandro Colomar wrote:
> Hi Joe,
> 
> On 5/9/23 19:43, Joe Simmons-Talbott via Libc-alpha wrote:
> > Avoid potential stack overflow from unbounded alloca.
> > ---
> >  stdio-common/vfprintf-internal.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> > index c76c06e49b..93f327c6b4 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);
> >  	  }
> > @@ -1334,6 +1338,13 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
> >  			      (specs[nspecs_done].next_fmt
> >  			       - specs[nspecs_done].end_of_fmt));
> >      }
> > +  if (malloced_pa_user)
> > +    for (cnt = 0; cnt < nargs; ++cnt)
> > +      {
> > +         if (args_value[cnt].pa_user != NULL)
> > +           free (args_value[cnt].pa_user);
> 
> free(NULL) is a no-op since at least C89.  The test is redundant.

Fixed in v2.  Thanks for the review.

Thanks,
Joe
  

On Wed, May 10, 2023 at 02:55:43PM +0200, Florian Weimer wrote:
> * Joe Simmons-Talbott via Libc-alpha:
> 
> > @@ -1334,6 +1338,13 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
> >  			      (specs[nspecs_done].next_fmt
> >  			       - specs[nspecs_done].end_of_fmt));
> >      }
> > +  if (malloced_pa_user)
> > +    for (cnt = 0; cnt < nargs; ++cnt)
> > +      {
> > +         if (args_value[cnt].pa_user != NULL)
> > +           free (args_value[cnt].pa_user);
> > +      }
> > +
> >   all_done:
> >    scratch_buffer_free (&argsbuf);
> >    scratch_buffer_free (&specsbuf);
> 
> I think the deallocation code needs to come after the all_done:
> label, otherwise memory will leak on errors.

Fixed in v2.  Thanks for the review.

Thanks,
Joe