diff mbox series

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

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

Checks

Context Check Description
dj/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent
dj/TryBot-32bit fail Patch series failed to apply

Commit Message

Joe Simmons-Talbott May 10, 2023, 3:32 p.m. UTC 
  Avoid potential stack overflow from unbounded alloca.
---
Changes to v1:
* Don't check pointer before calling free()
* Move deallocation code after the all_done label

 stdio-common/vfprintf-internal.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Carlos O'Donell May 15, 2023, 6:35 p.m. UTC | #1 
On 5/10/23 11:32, Joe Simmons-Talbott via Libc-alpha wrote:
> Avoid potential stack overflow from unbounded alloca.

This fails to apply in pre-commit CI e.g. error: corrupt patch at line 145

How did you generate this patch?

> ---
> Changes to v1:
> * Don't check pointer before calling free()
> * Move deallocation code after the all_done label
> 
>  stdio-common/vfprintf-internal.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> index c76c06e49b..58c21481ce 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,7 +1338,11 @@ printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
>  			      (specs[nspecs_done].next_fmt
>  			       - 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);
>  }
Joe Simmons-Talbott May 15, 2023, 6:49 p.m. UTC | #2 
On Mon, May 15, 2023 at 02:35:36PM -0400, Carlos O'Donell wrote:
> On 5/10/23 11:32, Joe Simmons-Talbott via Libc-alpha wrote:
> > Avoid potential stack overflow from unbounded alloca.
> 
> This fails to apply in pre-commit CI e.g. error: corrupt patch at line 145
> 
> How did you generate this patch?

I used 'git format-patch' but then removed a blank line that the patch
added during 'git send-email --annotate'.  Sorry.  I'll post a v3
shortly.

Thanks,
Joe
diff mbox series

Patch

diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
index c76c06e49b..58c21481ce 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,7 +1338,11 @@  printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
 			      (specs[nspecs_done].next_fmt
 			       - 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