* [PATCH] vfprintf-internal: Replace alloca with malloc.
@ 2023-05-09 17:43 Joe Simmons-Talbott
2023-05-10 10:40 ` Alejandro Colomar
2023-05-10 12:55 ` Florian Weimer
0 siblings, 2 replies; 7+ messages in thread
From: Joe Simmons-Talbott @ 2023-05-09 17:43 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott
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);
+ }
+
all_done:
scratch_buffer_free (&argsbuf);
scratch_buffer_free (&specsbuf);
--
2.39.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfprintf-internal: Replace alloca with malloc.
2023-05-09 17:43 [PATCH] vfprintf-internal: Replace alloca with malloc Joe Simmons-Talbott
@ 2023-05-10 10:40 ` Alejandro Colomar
2023-05-10 10:58 ` Alejandro Colomar
2023-05-10 15:34 ` Joe Simmons-Talbott
2023-05-10 12:55 ` Florian Weimer
1 sibling, 2 replies; 7+ messages in thread
From: Alejandro Colomar @ 2023-05-10 10:40 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfprintf-internal: Replace alloca with malloc.
2023-05-10 10:40 ` Alejandro Colomar
@ 2023-05-10 10:58 ` Alejandro Colomar
2023-05-10 11:25 ` Andreas Schwab
2023-05-10 15:34 ` Joe Simmons-Talbott
1 sibling, 1 reply; 7+ messages in thread
From: Alejandro Colomar @ 2023-05-10 10:58 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha, Adhemerval Zanella
[-- Attachment #1.1: Type: text/plain, Size: 3914 bytes --]
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);
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfprintf-internal: Replace alloca with malloc.
2023-05-10 10:58 ` Alejandro Colomar
@ 2023-05-10 11:25 ` Andreas Schwab
0 siblings, 0 replies; 7+ messages in thread
From: Andreas Schwab @ 2023-05-10 11:25 UTC (permalink / raw)
To: Alejandro Colomar via Libc-alpha
Cc: Joe Simmons-Talbott, Adhemerval Zanella, Alejandro Colomar
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.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfprintf-internal: Replace alloca with malloc.
2023-05-09 17:43 [PATCH] vfprintf-internal: Replace alloca with malloc Joe Simmons-Talbott
2023-05-10 10:40 ` Alejandro Colomar
@ 2023-05-10 12:55 ` Florian Weimer
2023-05-10 15:35 ` Joe Simmons-Talbott
1 sibling, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2023-05-10 12:55 UTC (permalink / raw)
To: Joe Simmons-Talbott via Libc-alpha; +Cc: Joe Simmons-Talbott
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfprintf-internal: Replace alloca with malloc.
2023-05-10 10:40 ` Alejandro Colomar
2023-05-10 10:58 ` Alejandro Colomar
@ 2023-05-10 15:34 ` Joe Simmons-Talbott
1 sibling, 0 replies; 7+ messages in thread
From: Joe Simmons-Talbott @ 2023-05-10 15:34 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: libc-alpha
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] vfprintf-internal: Replace alloca with malloc.
2023-05-10 12:55 ` Florian Weimer
@ 2023-05-10 15:35 ` Joe Simmons-Talbott
0 siblings, 0 replies; 7+ messages in thread
From: Joe Simmons-Talbott @ 2023-05-10 15:35 UTC (permalink / raw)
To: Florian Weimer; +Cc: Joe Simmons-Talbott via Libc-alpha
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-10 15:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-09 17:43 [PATCH] vfprintf-internal: Replace alloca with malloc Joe Simmons-Talbott
2023-05-10 10:40 ` Alejandro Colomar
2023-05-10 10:58 ` Alejandro Colomar
2023-05-10 11:25 ` Andreas Schwab
2023-05-10 15:34 ` Joe Simmons-Talbott
2023-05-10 12:55 ` Florian Weimer
2023-05-10 15:35 ` Joe Simmons-Talbott
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).