| Message ID | 20230509174329.1959380-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfprintf-internal: Replace alloca with malloc. | expand |
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
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); + } + all_done: scratch_buffer_free (&argsbuf); scratch_buffer_free (&specsbuf);