diff mbox series

setenv.c: Get rid of alloca.

Message ID 20230623152517.3268336-1-josimmon@redhat.com
State New
Headers show
Series setenv.c: Get rid of alloca. | expand

Commit Message

Joe Simmons-Talbott June 23, 2023, 3:25 p.m. UTC 
Use a scratch_buffer rather than alloca to avoid potential stack
overflow.
---
 stdlib/setenv.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

Comments

Carlos O'Donell June 26, 2023, 1:12 p.m. UTC | #1 
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
>  	}
>
Adhemerval Zanella Netto June 29, 2023, 2:39 p.m. UTC | #2 
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
>  	}
>
Joe Simmons-Talbott June 29, 2023, 3:25 p.m. UTC | #3 
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
> >  	}
> >  
>
diff mbox series

Patch

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
 	}