diff mbox series

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

Message ID 20230515185012.2768620-1-josimmon@redhat.com
State Superseded
Headers
Series [v3] vfprintf-internal: Replace alloca with malloc. |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Joe Simmons-Talbott May 15, 2023, 6:50 p.m. UTC 
  Avoid potential stack overflow from unbounded alloca.
---
 stdio-common/vfprintf-internal.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Paul Eggert May 17, 2023, 6:08 p.m. UTC | #1 
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?
Cristian Rodríguez May 17, 2023, 6:53 p.m. UTC | #2 
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.
Joe Simmons-Talbott May 17, 2023, 7:41 p.m. UTC | #3 
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
Paul Eggert May 17, 2023, 8:07 p.m. UTC | #4 
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.
Joe Simmons-Talbott May 17, 2023, 9:32 p.m. UTC | #5 
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
diff mbox series

Patch

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);
 }