* [PATCH] setenv.c: Get rid of alloca.
@ 2023-06-23 15:25 Joe Simmons-Talbott
2023-06-26 13:12 ` Carlos O'Donell
2023-06-29 14:39 ` Adhemerval Zanella Netto
0 siblings, 2 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-06-23 15:25 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott
Use a scratch_buffer rather than alloca to avoid potential stack
overflow.
---
stdlib/setenv.c | 35 ++++++++++-------------------------
1 file changed, 10 insertions(+), 25 deletions(-)
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index ba5257d3bf..90bc30b219 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -44,6 +44,8 @@ extern int errno;
# include <unistd.h>
#endif
+#include <scratch_buffer.h>
+
#if !_LIBC
# define __environ environ
# ifndef HAVE_ENVIRON_DECL
@@ -182,19 +184,14 @@ __add_to_environ (const char *name, const char *value, const char *combined,
{
const size_t varlen = namelen + 1 + vallen;
#ifdef USE_TSEARCH
- char *new_value;
- int use_alloca = __libc_use_alloca (varlen);
- if (__builtin_expect (use_alloca, 1))
- new_value = (char *) alloca (varlen);
- else
+ struct scratch_buffer buf;
+ scratch_buffer_init (&buf);
+ if (!scratch_buffer_set_array_size (&buf, 1, varlen))
{
- new_value = malloc (varlen);
- if (new_value == NULL)
- {
- UNLOCK;
- return -1;
- }
+ UNLOCK;
+ return -1;
}
+ char *new_value = buf.data;
# ifdef _LIBC
__mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
value, vallen);
@@ -209,18 +206,9 @@ __add_to_environ (const char *name, const char *value, const char *combined,
#endif
{
#ifdef USE_TSEARCH
- if (__glibc_unlikely (! use_alloca))
- np = new_value;
- else
+ np = new_value;
#endif
{
- np = malloc (varlen);
- if (__glibc_unlikely (np == NULL))
- {
- UNLOCK;
- return -1;
- }
-
#ifdef USE_TSEARCH
memcpy (np, new_value, varlen);
#else
@@ -234,10 +222,7 @@ __add_to_environ (const char *name, const char *value, const char *combined,
}
#ifdef USE_TSEARCH
else
- {
- if (__glibc_unlikely (! use_alloca))
- free (new_value);
- }
+ scratch_buffer_free (&buf);
#endif
}
--
2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] setenv.c: Get rid of alloca.
2023-06-23 15:25 [PATCH] setenv.c: Get rid of alloca Joe Simmons-Talbott
@ 2023-06-26 13:12 ` Carlos O'Donell
2023-06-29 14:39 ` Adhemerval Zanella Netto
1 sibling, 0 replies; 4+ messages in thread
From: Carlos O'Donell @ 2023-06-26 13:12 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha
On 6/23/23 11:25, Joe Simmons-Talbott via Libc-alpha wrote:
> Use a scratch_buffer rather than alloca to avoid potential stack
> overflow.
Fails pre-commit CI:
https://patchwork.sourceware.org/project/glibc/patch/20230623152517.3268336-1-josimmon@redhat.com/
> ---
> stdlib/setenv.c | 35 ++++++++++-------------------------
> 1 file changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> index ba5257d3bf..90bc30b219 100644
> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -44,6 +44,8 @@ extern int errno;
> # include <unistd.h>
> #endif
>
> +#include <scratch_buffer.h>
> +
> #if !_LIBC
> # define __environ environ
> # ifndef HAVE_ENVIRON_DECL
> @@ -182,19 +184,14 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> {
> const size_t varlen = namelen + 1 + vallen;
> #ifdef USE_TSEARCH
> - char *new_value;
> - int use_alloca = __libc_use_alloca (varlen);
> - if (__builtin_expect (use_alloca, 1))
> - new_value = (char *) alloca (varlen);
> - else
> + struct scratch_buffer buf;
> + scratch_buffer_init (&buf);
> + if (!scratch_buffer_set_array_size (&buf, 1, varlen))
> {
> - new_value = malloc (varlen);
> - if (new_value == NULL)
> - {
> - UNLOCK;
> - return -1;
> - }
> + UNLOCK;
> + return -1;
> }
> + char *new_value = buf.data;
> # ifdef _LIBC
> __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> value, vallen);
> @@ -209,18 +206,9 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> #endif
> {
> #ifdef USE_TSEARCH
> - if (__glibc_unlikely (! use_alloca))
> - np = new_value;
> - else
> + np = new_value;
> #endif
> {
> - np = malloc (varlen);
> - if (__glibc_unlikely (np == NULL))
> - {
> - UNLOCK;
> - return -1;
> - }
> -
> #ifdef USE_TSEARCH
> memcpy (np, new_value, varlen);
> #else
> @@ -234,10 +222,7 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> }
> #ifdef USE_TSEARCH
> else
> - {
> - if (__glibc_unlikely (! use_alloca))
> - free (new_value);
> - }
> + scratch_buffer_free (&buf);
> #endif
> }
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] setenv.c: Get rid of alloca.
2023-06-23 15:25 [PATCH] setenv.c: Get rid of alloca Joe Simmons-Talbott
2023-06-26 13:12 ` Carlos O'Donell
@ 2023-06-29 14:39 ` Adhemerval Zanella Netto
2023-06-29 15:25 ` Joe Simmons-Talbott
1 sibling, 1 reply; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-06-29 14:39 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha
On 23/06/23 12:25, Joe Simmons-Talbott via Libc-alpha wrote:
> Use a scratch_buffer rather than alloca to avoid potential stack
> overflow.
> ---
> stdlib/setenv.c | 35 ++++++++++-------------------------
> 1 file changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> index ba5257d3bf..90bc30b219 100644
> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -44,6 +44,8 @@ extern int errno;
> # include <unistd.h>
> #endif
>
> +#include <scratch_buffer.h>
> +
There is no use of scratch_buffer, the rest looks ok.
> #if !_LIBC
> # define __environ environ
> # ifndef HAVE_ENVIRON_DECL
> @@ -182,19 +184,14 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> {
> const size_t varlen = namelen + 1 + vallen;
> #ifdef USE_TSEARCH
> - char *new_value;
> - int use_alloca = __libc_use_alloca (varlen);
> - if (__builtin_expect (use_alloca, 1))
> - new_value = (char *) alloca (varlen);
> - else
> + struct scratch_buffer buf;
> + scratch_buffer_init (&buf);
> + if (!scratch_buffer_set_array_size (&buf, 1, varlen))
> {
> - new_value = malloc (varlen);
> - if (new_value == NULL)
> - {
> - UNLOCK;
> - return -1;
> - }
> + UNLOCK;
> + return -1;
> }
> + char *new_value = buf.data;
> # ifdef _LIBC
> __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> value, vallen);
> @@ -209,18 +206,9 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> #endif
> {
> #ifdef USE_TSEARCH
> - if (__glibc_unlikely (! use_alloca))
> - np = new_value;
> - else
> + np = new_value;
> #endif
> {
> - np = malloc (varlen);
> - if (__glibc_unlikely (np == NULL))
> - {
> - UNLOCK;
> - return -1;
> - }
> -
> #ifdef USE_TSEARCH
> memcpy (np, new_value, varlen);
> #else
> @@ -234,10 +222,7 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> }
> #ifdef USE_TSEARCH
> else
> - {
> - if (__glibc_unlikely (! use_alloca))
> - free (new_value);
> - }
> + scratch_buffer_free (&buf);
> #endif
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] setenv.c: Get rid of alloca.
2023-06-29 14:39 ` Adhemerval Zanella Netto
@ 2023-06-29 15:25 ` Joe Simmons-Talbott
0 siblings, 0 replies; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-06-29 15:25 UTC (permalink / raw)
To: Adhemerval Zanella Netto; +Cc: libc-alpha
On Thu, Jun 29, 2023 at 11:39:08AM -0300, Adhemerval Zanella Netto wrote:
>
>
> On 23/06/23 12:25, Joe Simmons-Talbott via Libc-alpha wrote:
> > Use a scratch_buffer rather than alloca to avoid potential stack
> > overflow.
> > ---
> > stdlib/setenv.c | 35 ++++++++++-------------------------
> > 1 file changed, 10 insertions(+), 25 deletions(-)
> >
> > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > index ba5257d3bf..90bc30b219 100644
> > --- a/stdlib/setenv.c
> > +++ b/stdlib/setenv.c
> > @@ -44,6 +44,8 @@ extern int errno;
> > # include <unistd.h>
> > #endif
> >
> > +#include <scratch_buffer.h>
> > +
>
> There is no use of scratch_buffer, the rest looks ok.
Thanks for catching that and for the review. Fix pushed in v3.
Thanks,
Joe
>
> > #if !_LIBC
> > # define __environ environ
> > # ifndef HAVE_ENVIRON_DECL
> > @@ -182,19 +184,14 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> > {
> > const size_t varlen = namelen + 1 + vallen;
> > #ifdef USE_TSEARCH
> > - char *new_value;
> > - int use_alloca = __libc_use_alloca (varlen);
> > - if (__builtin_expect (use_alloca, 1))
> > - new_value = (char *) alloca (varlen);
> > - else
> > + struct scratch_buffer buf;
> > + scratch_buffer_init (&buf);
> > + if (!scratch_buffer_set_array_size (&buf, 1, varlen))
> > {
> > - new_value = malloc (varlen);
> > - if (new_value == NULL)
> > - {
> > - UNLOCK;
> > - return -1;
> > - }
> > + UNLOCK;
> > + return -1;
> > }
> > + char *new_value = buf.data;
> > # ifdef _LIBC
> > __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> > value, vallen);
> > @@ -209,18 +206,9 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> > #endif
> > {
> > #ifdef USE_TSEARCH
> > - if (__glibc_unlikely (! use_alloca))
> > - np = new_value;
> > - else
> > + np = new_value;
> > #endif
> > {
> > - np = malloc (varlen);
> > - if (__glibc_unlikely (np == NULL))
> > - {
> > - UNLOCK;
> > - return -1;
> > - }
> > -
> > #ifdef USE_TSEARCH
> > memcpy (np, new_value, varlen);
> > #else
> > @@ -234,10 +222,7 @@ __add_to_environ (const char *name, const char *value, const char *combined,
> > }
> > #ifdef USE_TSEARCH
> > else
> > - {
> > - if (__glibc_unlikely (! use_alloca))
> > - free (new_value);
> > - }
> > + scratch_buffer_free (&buf);
> > #endif
> > }
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-29 15:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-23 15:25 [PATCH] setenv.c: Get rid of alloca Joe Simmons-Talbott
2023-06-26 13:12 ` Carlos O'Donell
2023-06-29 14:39 ` Adhemerval Zanella Netto
2023-06-29 15:25 ` 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).