[v2] argp-help: Get rid of alloca.
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
Commit Message
Replace alloca with a scratch_buffer to avoid potential stack overflow.
Checked on x86_64-linux-gnu
---
Changes to v1:
* Call assert on allocation failure.
argp/argp-help.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
Comments
Ping.
On Mon, Aug 28, 2023 at 02:27:01PM -0400, Joe Simmons-Talbott wrote:
> Replace alloca with a scratch_buffer to avoid potential stack overflow.
>
> Checked on x86_64-linux-gnu
> ---
> Changes to v1:
> * Call assert on allocation failure.
>
> argp/argp-help.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/argp/argp-help.c b/argp/argp-help.c
> index d019ed58d2..af99649f20 100644
> --- a/argp/argp-help.c
> +++ b/argp/argp-help.c
> @@ -40,6 +40,7 @@ char *alloca ();
> # endif
> #endif
>
> +#include <scratch_buffer.h>
> #include <stdbool.h>
> #include <stddef.h>
> #include <stdlib.h>
> @@ -1450,8 +1451,19 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
> {
> unsigned nentries;
> struct hol_entry *entry;
> - char *short_no_arg_opts = alloca (strlen (hol->short_options) + 1);
> - char *snao_end = short_no_arg_opts;
> + struct scratch_buffer buf;
> + scratch_buffer_init (&buf);
> + char *short_no_arg_opts;
> + char *snao_end;
> + bool result;
> +
> + result = scratch_buffer_set_array_size (&buf, 1,
> + strlen (hol->short_options) + 1);
> + assert (result);
> +
> + short_no_arg_opts = buf.data;
> + snao_end = short_no_arg_opts;
> +
>
> /* First we put a list of short options without arguments. */
> for (entry = hol->entries, nentries = hol->num_entries
> @@ -1478,6 +1490,8 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
> ; entry++, nentries--)
> hol_entry_long_iterate (entry, usage_long_opt,
> entry->argp->argp_domain, stream);
> +
> + scratch_buffer_free (&buf);
> }
> }
>
> @@ -1698,7 +1712,15 @@ _help (const struct argp *argp, const struct argp_state *state, FILE *stream,
> {
> int first_pattern = 1, more_patterns;
> size_t num_pattern_levels = argp_args_levels (argp);
> - char *pattern_levels = alloca (num_pattern_levels);
> + struct scratch_buffer buf;
> + scratch_buffer_init (&buf);
> + char *pattern_levels;
> + bool result;
> +
> + result = scratch_buffer_set_array_size (&buf, 1, num_pattern_levels);
> + assert (result);
> +
> + pattern_levels = buf.data;
>
> memset (pattern_levels, 0, num_pattern_levels);
>
> @@ -1746,6 +1768,8 @@ _help (const struct argp *argp, const struct argp_state *state, FILE *stream,
> first_pattern = 0;
> }
> while (more_patterns);
> +
> + scratch_buffer_free (&buf);
> }
>
> if (flags & ARGP_HELP_PRE_DOC)
> --
> 2.39.2
>
On 28/08/23 15:27, Joe Simmons-Talbott via Libc-alpha wrote:
> Replace alloca with a scratch_buffer to avoid potential stack overflow.
>
> Checked on x86_64-linux-gnu
> ---
> Changes to v1:
> * Call assert on allocation failure.
>
> argp/argp-help.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/argp/argp-help.c b/argp/argp-help.c
> index d019ed58d2..af99649f20 100644
> --- a/argp/argp-help.c
> +++ b/argp/argp-help.c
> @@ -40,6 +40,7 @@ char *alloca ();
> # endif
> #endif
>
> +#include <scratch_buffer.h>
> #include <stdbool.h>
> #include <stddef.h>
> #include <stdlib.h>
> @@ -1450,8 +1451,19 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
> {
> unsigned nentries;
> struct hol_entry *entry;
> - char *short_no_arg_opts = alloca (strlen (hol->short_options) + 1);
> - char *snao_end = short_no_arg_opts;
> + struct scratch_buffer buf;
> + scratch_buffer_init (&buf);
> + char *short_no_arg_opts;
> + char *snao_end;
> + bool result;
> +
> + result = scratch_buffer_set_array_size (&buf, 1,
> + strlen (hol->short_options) + 1);
> + assert (result);
> +
> + short_no_arg_opts = buf.data;
> + snao_end = short_no_arg_opts;
> +
>
With a second look, this interface is really not well designed for memory
allocation failures so I think it would be simpler to just use malloc here
and assert for allocation failure (It would also make it simpler to eventually
sync with gnulib).
Also, remove the boilerplate alloca definition above.
> /* First we put a list of short options without arguments. */
> for (entry = hol->entries, nentries = hol->num_entries
> @@ -1478,6 +1490,8 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
> ; entry++, nentries--)
> hol_entry_long_iterate (entry, usage_long_opt,
> entry->argp->argp_domain, stream);
> +
> + scratch_buffer_free (&buf);
> }
> }
>
> @@ -1698,7 +1712,15 @@ _help (const struct argp *argp, const struct argp_state *state, FILE *stream,
> {
> int first_pattern = 1, more_patterns;
> size_t num_pattern_levels = argp_args_levels (argp);
> - char *pattern_levels = alloca (num_pattern_levels);
> + struct scratch_buffer buf;
> + scratch_buffer_init (&buf);
> + char *pattern_levels;
> + bool result;
> +
> + result = scratch_buffer_set_array_size (&buf, 1, num_pattern_levels);
> + assert (result);
> +
> + pattern_levels = buf.data;
>
> memset (pattern_levels, 0, num_pattern_levels);
>
> @@ -1746,6 +1768,8 @@ _help (const struct argp *argp, const struct argp_state *state, FILE *stream,
> first_pattern = 0;
> }
> while (more_patterns);
> +
> + scratch_buffer_free (&buf);
> }
>
> if (flags & ARGP_HELP_PRE_DOC)
On Wed, Sep 13, 2023 at 11:39:10AM -0300, Adhemerval Zanella Netto wrote:
>
>
> On 28/08/23 15:27, Joe Simmons-Talbott via Libc-alpha wrote:
> > Replace alloca with a scratch_buffer to avoid potential stack overflow.
> >
> > Checked on x86_64-linux-gnu
> > ---
> > Changes to v1:
> > * Call assert on allocation failure.
> >
> > argp/argp-help.c | 30 +++++++++++++++++++++++++++---
> > 1 file changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/argp/argp-help.c b/argp/argp-help.c
> > index d019ed58d2..af99649f20 100644
> > --- a/argp/argp-help.c
> > +++ b/argp/argp-help.c
> > @@ -40,6 +40,7 @@ char *alloca ();
> > # endif
> > #endif
> >
> > +#include <scratch_buffer.h>
> > #include <stdbool.h>
> > #include <stddef.h>
> > #include <stdlib.h>
> > @@ -1450,8 +1451,19 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
> > {
> > unsigned nentries;
> > struct hol_entry *entry;
> > - char *short_no_arg_opts = alloca (strlen (hol->short_options) + 1);
> > - char *snao_end = short_no_arg_opts;
> > + struct scratch_buffer buf;
> > + scratch_buffer_init (&buf);
> > + char *short_no_arg_opts;
> > + char *snao_end;
> > + bool result;
> > +
> > + result = scratch_buffer_set_array_size (&buf, 1,
> > + strlen (hol->short_options) + 1);
> > + assert (result);
> > +
> > + short_no_arg_opts = buf.data;
> > + snao_end = short_no_arg_opts;
> > +
> >
>
> With a second look, this interface is really not well designed for memory
> allocation failures so I think it would be simpler to just use malloc here
> and assert for allocation failure (It would also make it simpler to eventually
> sync with gnulib).
>
> Also, remove the boilerplate alloca definition above.
Ack. Fixed in v3.
Thanks,
Joe
@@ -40,6 +40,7 @@ char *alloca ();
# endif
#endif
+#include <scratch_buffer.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
@@ -1450,8 +1451,19 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
{
unsigned nentries;
struct hol_entry *entry;
- char *short_no_arg_opts = alloca (strlen (hol->short_options) + 1);
- char *snao_end = short_no_arg_opts;
+ struct scratch_buffer buf;
+ scratch_buffer_init (&buf);
+ char *short_no_arg_opts;
+ char *snao_end;
+ bool result;
+
+ result = scratch_buffer_set_array_size (&buf, 1,
+ strlen (hol->short_options) + 1);
+ assert (result);
+
+ short_no_arg_opts = buf.data;
+ snao_end = short_no_arg_opts;
+
/* First we put a list of short options without arguments. */
for (entry = hol->entries, nentries = hol->num_entries
@@ -1478,6 +1490,8 @@ hol_usage (struct hol *hol, argp_fmtstream_t stream)
; entry++, nentries--)
hol_entry_long_iterate (entry, usage_long_opt,
entry->argp->argp_domain, stream);
+
+ scratch_buffer_free (&buf);
}
}
@@ -1698,7 +1712,15 @@ _help (const struct argp *argp, const struct argp_state *state, FILE *stream,
{
int first_pattern = 1, more_patterns;
size_t num_pattern_levels = argp_args_levels (argp);
- char *pattern_levels = alloca (num_pattern_levels);
+ struct scratch_buffer buf;
+ scratch_buffer_init (&buf);
+ char *pattern_levels;
+ bool result;
+
+ result = scratch_buffer_set_array_size (&buf, 1, num_pattern_levels);
+ assert (result);
+
+ pattern_levels = buf.data;
memset (pattern_levels, 0, num_pattern_levels);
@@ -1746,6 +1768,8 @@ _help (const struct argp *argp, const struct argp_state *state, FILE *stream,
first_pattern = 0;
}
while (more_patterns);
+
+ scratch_buffer_free (&buf);
}
if (flags & ARGP_HELP_PRE_DOC)