* [PATCH v2] argp-help: Get rid of alloca.
@ 2023-08-28 18:27 Joe Simmons-Talbott
2023-09-13 13:02 ` Joe Simmons-Talbott
2023-09-13 14:39 ` Adhemerval Zanella Netto
0 siblings, 2 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-08-28 18:27 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott
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);
}
}
\f
@@ -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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] argp-help: Get rid of alloca.
2023-08-28 18:27 [PATCH v2] argp-help: Get rid of alloca Joe Simmons-Talbott
@ 2023-09-13 13:02 ` Joe Simmons-Talbott
2023-09-13 14:39 ` Adhemerval Zanella Netto
1 sibling, 0 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-09-13 13:02 UTC (permalink / raw)
To: libc-alpha
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);
> }
> }
> \f
> @@ -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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] argp-help: Get rid of alloca.
2023-08-28 18:27 [PATCH v2] argp-help: Get rid of alloca Joe Simmons-Talbott
2023-09-13 13:02 ` Joe Simmons-Talbott
@ 2023-09-13 14:39 ` Adhemerval Zanella Netto
2023-09-13 16:02 ` Joe Simmons-Talbott
1 sibling, 1 reply; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-09-13 14:39 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha
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);
> }
> }
> \f
> @@ -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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] argp-help: Get rid of alloca.
2023-09-13 14:39 ` Adhemerval Zanella Netto
@ 2023-09-13 16:02 ` Joe Simmons-Talbott
0 siblings, 0 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-09-13 16:02 UTC (permalink / raw)
To: Adhemerval Zanella Netto; +Cc: libc-alpha
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-13 16:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-28 18:27 [PATCH v2] argp-help: Get rid of alloca Joe Simmons-Talbott
2023-09-13 13:02 ` Joe Simmons-Talbott
2023-09-13 14:39 ` Adhemerval Zanella Netto
2023-09-13 16:02 ` 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).