[PATCH] Deduplicate setenv.c

[PATCH] Deduplicate setenv.c

[Ondřej Bílka]

Hi,

In setenv.c file a logic of adding element is needlessly duplicated
based on if we extend __environ or not. This can be easily fixed in
following way:

	* stdlib/setenv.c: Remove duplicate code.

diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index 63c995b..e4b5b5e 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
 	  UNLOCK;
 	  return -1;
 	}
-
-      /* If the whole entry is given add it.  */
-      if (combined != NULL)
-	/* We must not add the string to the search tree since it belongs
-	   to the user.  */
-	new_environ[size] = (char *) combined;
-      else
-	{
-	  /* See whether the value is already known.  */
-#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
-	    {
-	      new_value = malloc (varlen);
-	      if (new_value == NULL)
-		{
-		  UNLOCK;
-		  if (last_environ == NULL)
-		    free (new_environ);
-		  return -1;
-		}
-	    }
-# ifdef _LIBC
-	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
-		     value, vallen);
-# else
-	  memcpy (new_value, name, namelen);
-	  new_value[namelen] = '=';
-	  memcpy (&new_value[namelen + 1], value, vallen);
-# endif
-
-	  new_environ[size] = KNOWN_VALUE (new_value);
-	  if (__builtin_expect (new_environ[size] == NULL, 1))
-#endif
-	    {
-#ifdef USE_TSEARCH
-	      if (__builtin_expect (! use_alloca, 0))
-		new_environ[size] = new_value;
-	      else
-#endif
-		{
-		  new_environ[size] = (char *) malloc (varlen);
-		  if (__builtin_expect (new_environ[size] == NULL, 0))
-		    {
-		      UNLOCK;
-		      return -1;
-		    }
-
-#ifdef USE_TSEARCH
-		  memcpy (new_environ[size], new_value, varlen);
-#else
-		  memcpy (new_environ[size], name, namelen);
-		  new_environ[size][namelen] = '=';
-		  memcpy (&new_environ[size][namelen + 1], value, vallen);
-#endif
-		}
-
-	      /* And save the value now.  We cannot do this when we remove
-		 the string since then we cannot decide whether it is a
-		 user string or not.  */
-	      STORE_VALUE (new_environ[size]);
-	    }
-	}
-
-      if (__environ != last_environ)
-	memcpy ((char *) new_environ, (char *) __environ,
-		size * sizeof (char *));
-
+      new_environ[size] = NULL;
       new_environ[size + 1] = NULL;
+      ep = new_environ + size;
 
       last_environ = __environ = new_environ;
     }
-  else if (replace)
+  if (*ep == NULL || replace)
     {
       char *np;
 

[PATCH 2a][BZ #15607] Make setenv thread safe.

[Ondřej Bílka]

On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> Hi,
> 
> In setenv.c file a logic of adding element is needlessly duplicated
> based on if we extend __environ or not. This can be easily fixed in
> following way:
> 
Previos patch cleared setenv implementation for this patch. 
A problem in this bug entry is that getenv can segfault when other
thread calls setenv which reallocates environment. As we need to leak
getenv entries we can also affort to leak old enviroments.

We need to double size for each reallocation to ensure that amount of
space allocated is at most double of space occupied by current environ
array.

This does not make getenv completely reentrant, a unsetenv could cause a
getenv call with unrelated key to fail.

A doubling of allocations is needed for future speeding lookups by hash
table, in case that we do not want this patch I will prepare a 2b
version that deallocates environments.

This patch causes a intl/mtrace-tst-gettext fail with memory not freed
message. How to suppress this?


	[BZ #15607]
	* stdlib/setenv.c: Make getenv thread safe.

---
 stdlib/setenv.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index 5524cc0..29faebf 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -97,7 +97,7 @@ static void *known_values;
 /* If this variable is not a null pointer we allocated the current
    environment.  */
 static char **last_environ;
-
+static size_t allocated;
 
 /* This function is used by `setenv' and `putenv'.  The difference between
    the two functions is that for the former must create a new string which
@@ -133,28 +133,34 @@ __add_to_environ (name, value, combined, replace)
 	  ++size;
     }
 
-  if (ep == NULL || __builtin_expect (*ep == NULL, 1))
+  if (ep == NULL || (*ep == NULL && __environ != last_environ) ||
+      (__environ == last_environ && size == allocated - 1))
     {
       char **new_environ;
+      size_t i;
+      size_t newsize = 2 * size + 2;
 
-      /* We allocated this space; we can extend it.  */
-      new_environ = (char **) realloc (last_environ,
-				       (size + 2) * sizeof (char *));
+      /* We need to keep old environments to make getenv thread safe.  */
+      new_environ = (char **) malloc ((newsize + 1) * sizeof (char *));
       if (new_environ == NULL)
 	{
 	  UNLOCK;
 	  return -1;
 	}
 
-      if (__environ != last_environ)
-	memcpy ((char *) new_environ, (char *) __environ,
-		size * sizeof (char *));
+      /* To keep valgrind from reporting leak.  */
+      new_environ[0] = last_environ;
+      new_environ++;
 
-      new_environ[size] = NULL;
-      ep = new_environ + size;
-      new_environ[size + 1] = NULL;
+      memcpy ((char *) new_environ, (char *) __environ,
+              size * sizeof (char *));
+
+      for (i = size; i < newsize; i++)
+        new_environ[i] = NULL;
 
       last_environ = __environ = new_environ;
+      allocated = newsize;
+      ep = new_environ + size;
     }
   if (*ep == NULL || replace)
     {
@@ -288,13 +294,6 @@ clearenv (void)
 {
   LOCK;
 
-  if (__environ == last_environ && __environ != NULL)
-    {
-      /* We allocated this environment so we can free it.  */
-      free (__environ);
-      last_environ = NULL;
-    }
-
   /* Clear the environment pointer removes the whole environment.  */
   __environ = NULL;
 
-- 
1.8.4.rc3

Re: [PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping,

I realized that this code movement fixes leak in bug 15894.

On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> Hi,
> 
> In setenv.c file a logic of adding element is needlessly duplicated
> based on if we extend __environ or not. This can be easily fixed in
> following way:
> 
> 	* stdlib/setenv.c: Remove duplicate code.
> 
> diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> index 63c995b..e4b5b5e 100644
> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
>  	  UNLOCK;
>  	  return -1;
>  	}
> -
> -      /* If the whole entry is given add it.  */
> -      if (combined != NULL)
> -	/* We must not add the string to the search tree since it belongs
> -	   to the user.  */
> -	new_environ[size] = (char *) combined;
> -      else
> -	{
> -	  /* See whether the value is already known.  */
> -#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
> -	    {
> -	      new_value = malloc (varlen);
> -	      if (new_value == NULL)
> -		{
> -		  UNLOCK;
> -		  if (last_environ == NULL)
> -		    free (new_environ);
> -		  return -1;
> -		}
> -	    }
> -# ifdef _LIBC
> -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> -		     value, vallen);
> -# else
> -	  memcpy (new_value, name, namelen);
> -	  new_value[namelen] = '=';
> -	  memcpy (&new_value[namelen + 1], value, vallen);
> -# endif
> -
> -	  new_environ[size] = KNOWN_VALUE (new_value);
> -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> -#endif
> -	    {
> -#ifdef USE_TSEARCH
> -	      if (__builtin_expect (! use_alloca, 0))
> -		new_environ[size] = new_value;
> -	      else
> -#endif
> -		{
> -		  new_environ[size] = (char *) malloc (varlen);
> -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> -		    {
> -		      UNLOCK;
> -		      return -1;
> -		    }
> -
> -#ifdef USE_TSEARCH
> -		  memcpy (new_environ[size], new_value, varlen);
> -#else
> -		  memcpy (new_environ[size], name, namelen);
> -		  new_environ[size][namelen] = '=';
> -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> -#endif
> -		}
> -
> -	      /* And save the value now.  We cannot do this when we remove
> -		 the string since then we cannot decide whether it is a
> -		 user string or not.  */
> -	      STORE_VALUE (new_environ[size]);
> -	    }
> -	}
> -
> -      if (__environ != last_environ)
> -	memcpy ((char *) new_environ, (char *) __environ,
> -		size * sizeof (char *));
> -
> +      new_environ[size] = NULL;
>        new_environ[size + 1] = NULL;
> +      ep = new_environ + size;
>  
>        last_environ = __environ = new_environ;
>      }
> -  else if (replace)
> +  if (*ep == NULL || replace)
>      {
>        char *np;
>  

-- 

NOTICE: alloc: /dev/null: filesystem full

[PING][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping
On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> Hi,
> 
> In setenv.c file a logic of adding element is needlessly duplicated
> based on if we extend __environ or not. This can be easily fixed in
> following way:
> 
> 	* stdlib/setenv.c: Remove duplicate code.
> 
> diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> index 63c995b..e4b5b5e 100644
> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
>  	  UNLOCK;
>  	  return -1;
>  	}
> -
> -      /* If the whole entry is given add it.  */
> -      if (combined != NULL)
> -	/* We must not add the string to the search tree since it belongs
> -	   to the user.  */
> -	new_environ[size] = (char *) combined;
> -      else
> -	{
> -	  /* See whether the value is already known.  */
> -#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
> -	    {
> -	      new_value = malloc (varlen);
> -	      if (new_value == NULL)
> -		{
> -		  UNLOCK;
> -		  if (last_environ == NULL)
> -		    free (new_environ);
> -		  return -1;
> -		}
> -	    }
> -# ifdef _LIBC
> -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> -		     value, vallen);
> -# else
> -	  memcpy (new_value, name, namelen);
> -	  new_value[namelen] = '=';
> -	  memcpy (&new_value[namelen + 1], value, vallen);
> -# endif
> -
> -	  new_environ[size] = KNOWN_VALUE (new_value);
> -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> -#endif
> -	    {
> -#ifdef USE_TSEARCH
> -	      if (__builtin_expect (! use_alloca, 0))
> -		new_environ[size] = new_value;
> -	      else
> -#endif
> -		{
> -		  new_environ[size] = (char *) malloc (varlen);
> -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> -		    {
> -		      UNLOCK;
> -		      return -1;
> -		    }
> -
> -#ifdef USE_TSEARCH
> -		  memcpy (new_environ[size], new_value, varlen);
> -#else
> -		  memcpy (new_environ[size], name, namelen);
> -		  new_environ[size][namelen] = '=';
> -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> -#endif
> -		}
> -
> -	      /* And save the value now.  We cannot do this when we remove
> -		 the string since then we cannot decide whether it is a
> -		 user string or not.  */
> -	      STORE_VALUE (new_environ[size]);
> -	    }
> -	}
> -
> -      if (__environ != last_environ)
> -	memcpy ((char *) new_environ, (char *) __environ,
> -		size * sizeof (char *));
> -
> +      new_environ[size] = NULL;
>        new_environ[size + 1] = NULL;
> +      ep = new_environ + size;
>  
>        last_environ = __environ = new_environ;
>      }
> -  else if (replace)
> +  if (*ep == NULL || replace)
>      {
>        char *np;
>  

-- 

pseudo-user on a pseudo-terminal

Re: [PING][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping
On Wed, Oct 23, 2013 at 03:17:26PM +0200, Ondřej Bílka wrote:
> ping
> On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > Hi,
> > 
> > In setenv.c file a logic of adding element is needlessly duplicated
> > based on if we extend __environ or not. This can be easily fixed in
> > following way:
> > 
> > 	* stdlib/setenv.c: Remove duplicate code.
> > 
> > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > index 63c995b..e4b5b5e 100644
> > --- a/stdlib/setenv.c
> > +++ b/stdlib/setenv.c
> > @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
> >  	  UNLOCK;
> >  	  return -1;
> >  	}
> > -
> > -      /* If the whole entry is given add it.  */
> > -      if (combined != NULL)
> > -	/* We must not add the string to the search tree since it belongs
> > -	   to the user.  */
> > -	new_environ[size] = (char *) combined;
> > -      else
> > -	{
> > -	  /* See whether the value is already known.  */
> > -#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
> > -	    {
> > -	      new_value = malloc (varlen);
> > -	      if (new_value == NULL)
> > -		{
> > -		  UNLOCK;
> > -		  if (last_environ == NULL)
> > -		    free (new_environ);
> > -		  return -1;
> > -		}
> > -	    }
> > -# ifdef _LIBC
> > -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> > -		     value, vallen);
> > -# else
> > -	  memcpy (new_value, name, namelen);
> > -	  new_value[namelen] = '=';
> > -	  memcpy (&new_value[namelen + 1], value, vallen);
> > -# endif
> > -
> > -	  new_environ[size] = KNOWN_VALUE (new_value);
> > -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> > -#endif
> > -	    {
> > -#ifdef USE_TSEARCH
> > -	      if (__builtin_expect (! use_alloca, 0))
> > -		new_environ[size] = new_value;
> > -	      else
> > -#endif
> > -		{
> > -		  new_environ[size] = (char *) malloc (varlen);
> > -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> > -		    {
> > -		      UNLOCK;
> > -		      return -1;
> > -		    }
> > -
> > -#ifdef USE_TSEARCH
> > -		  memcpy (new_environ[size], new_value, varlen);
> > -#else
> > -		  memcpy (new_environ[size], name, namelen);
> > -		  new_environ[size][namelen] = '=';
> > -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> > -#endif
> > -		}
> > -
> > -	      /* And save the value now.  We cannot do this when we remove
> > -		 the string since then we cannot decide whether it is a
> > -		 user string or not.  */
> > -	      STORE_VALUE (new_environ[size]);
> > -	    }
> > -	}
> > -
> > -      if (__environ != last_environ)
> > -	memcpy ((char *) new_environ, (char *) __environ,
> > -		size * sizeof (char *));
> > -
> > +      new_environ[size] = NULL;
> >        new_environ[size + 1] = NULL;
> > +      ep = new_environ + size;
> >  
> >        last_environ = __environ = new_environ;
> >      }
> > -  else if (replace)
> > +  if (*ep == NULL || replace)
> >      {
> >        char *np;
> >  
> 
> -- 
> 
> pseudo-user on a pseudo-terminal

-- 

operators on strike due to broken coffee machine

Re: [PING^3][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping,

Changelog became 

	[BZ #15894]
	* stdlib/setenv.c: Remove duplicate code.

On Wed, Oct 30, 2013 at 04:33:42PM +0100, Ondřej Bílka wrote:
> ping
> On Wed, Oct 23, 2013 at 03:17:26PM +0200, Ondřej Bílka wrote:
> > ping
> > On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > > Hi,
> > > 
> > > In setenv.c file a logic of adding element is needlessly duplicated
> > > based on if we extend __environ or not. This can be easily fixed in
> > > following way:
> > > 
> > > 	* stdlib/setenv.c: Remove duplicate code.
> > > 
> > > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > > index 63c995b..e4b5b5e 100644
> > > --- a/stdlib/setenv.c
> > > +++ b/stdlib/setenv.c
> > > @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
> > >  	  UNLOCK;
> > >  	  return -1;
> > >  	}
> > > -
> > > -      /* If the whole entry is given add it.  */
> > > -      if (combined != NULL)
> > > -	/* We must not add the string to the search tree since it belongs
> > > -	   to the user.  */
> > > -	new_environ[size] = (char *) combined;
> > > -      else
> > > -	{
> > > -	  /* See whether the value is already known.  */
> > > -#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
> > > -	    {
> > > -	      new_value = malloc (varlen);
> > > -	      if (new_value == NULL)
> > > -		{
> > > -		  UNLOCK;
> > > -		  if (last_environ == NULL)
> > > -		    free (new_environ);
> > > -		  return -1;
> > > -		}
> > > -	    }
> > > -# ifdef _LIBC
> > > -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> > > -		     value, vallen);
> > > -# else
> > > -	  memcpy (new_value, name, namelen);
> > > -	  new_value[namelen] = '=';
> > > -	  memcpy (&new_value[namelen + 1], value, vallen);
> > > -# endif
> > > -
> > > -	  new_environ[size] = KNOWN_VALUE (new_value);
> > > -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> > > -#endif
> > > -	    {
> > > -#ifdef USE_TSEARCH
> > > -	      if (__builtin_expect (! use_alloca, 0))
> > > -		new_environ[size] = new_value;
> > > -	      else
> > > -#endif
> > > -		{
> > > -		  new_environ[size] = (char *) malloc (varlen);
> > > -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> > > -		    {
> > > -		      UNLOCK;
> > > -		      return -1;
> > > -		    }
> > > -
> > > -#ifdef USE_TSEARCH
> > > -		  memcpy (new_environ[size], new_value, varlen);
> > > -#else
> > > -		  memcpy (new_environ[size], name, namelen);
> > > -		  new_environ[size][namelen] = '=';
> > > -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> > > -#endif
> > > -		}
> > > -
> > > -	      /* And save the value now.  We cannot do this when we remove
> > > -		 the string since then we cannot decide whether it is a
> > > -		 user string or not.  */
> > > -	      STORE_VALUE (new_environ[size]);
> > > -	    }
> > > -	}
> > > -
> > > -      if (__environ != last_environ)
> > > -	memcpy ((char *) new_environ, (char *) __environ,
> > > -		size * sizeof (char *));
> > > -
> > > +      new_environ[size] = NULL;
> > >        new_environ[size + 1] = NULL;
> > > +      ep = new_environ + size;
> > >  
> > >        last_environ = __environ = new_environ;
> > >      }
> > > -  else if (replace)
> > > +  if (*ep == NULL || replace)
> > >      {
> > >        char *np;
> > >  
> > 
> > -- 
> > 
> > pseudo-user on a pseudo-terminal
> 
> -- 
> 
> operators on strike due to broken coffee machine

-- 

Hot Java has gone cold

Re: [PATCH 2a][BZ #15607] Make setenv thread safe.

[Ondřej Bílka]

ping
On Tue, Oct 15, 2013 at 02:22:07PM +0200, Ondřej Bílka wrote:
> On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > Hi,
> > 
> > In setenv.c file a logic of adding element is needlessly duplicated
> > based on if we extend __environ or not. This can be easily fixed in
> > following way:
> > 
> Previos patch cleared setenv implementation for this patch. 
> A problem in this bug entry is that getenv can segfault when other
> thread calls setenv which reallocates environment. As we need to leak
> getenv entries we can also affort to leak old enviroments.
> 
> We need to double size for each reallocation to ensure that amount of
> space allocated is at most double of space occupied by current environ
> array.
> 
> This does not make getenv completely reentrant, a unsetenv could cause a
> getenv call with unrelated key to fail.
> 
> A doubling of allocations is needed for future speeding lookups by hash
> table, in case that we do not want this patch I will prepare a 2b
> version that deallocates environments.
> 
> This patch causes a intl/mtrace-tst-gettext fail with memory not freed
> message. How to suppress this?
> 
> 
> 	[BZ #15607]
> 	* stdlib/setenv.c: Make getenv thread safe.
> 
> ---
>  stdlib/setenv.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> index 5524cc0..29faebf 100644
> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -97,7 +97,7 @@ static void *known_values;
>  /* If this variable is not a null pointer we allocated the current
>     environment.  */
>  static char **last_environ;
> -
> +static size_t allocated;
>  
>  /* This function is used by `setenv' and `putenv'.  The difference between
>     the two functions is that for the former must create a new string which
> @@ -133,28 +133,34 @@ __add_to_environ (name, value, combined, replace)
>  	  ++size;
>      }
>  
> -  if (ep == NULL || __builtin_expect (*ep == NULL, 1))
> +  if (ep == NULL || (*ep == NULL && __environ != last_environ) ||
> +      (__environ == last_environ && size == allocated - 1))
>      {
>        char **new_environ;
> +      size_t i;
> +      size_t newsize = 2 * size + 2;
>  
> -      /* We allocated this space; we can extend it.  */
> -      new_environ = (char **) realloc (last_environ,
> -				       (size + 2) * sizeof (char *));
> +      /* We need to keep old environments to make getenv thread safe.  */
> +      new_environ = (char **) malloc ((newsize + 1) * sizeof (char *));
>        if (new_environ == NULL)
>  	{
>  	  UNLOCK;
>  	  return -1;
>  	}
>  
> -      if (__environ != last_environ)
> -	memcpy ((char *) new_environ, (char *) __environ,
> -		size * sizeof (char *));
> +      /* To keep valgrind from reporting leak.  */
> +      new_environ[0] = last_environ;
> +      new_environ++;
>  
> -      new_environ[size] = NULL;
> -      ep = new_environ + size;
> -      new_environ[size + 1] = NULL;
> +      memcpy ((char *) new_environ, (char *) __environ,
> +              size * sizeof (char *));
> +
> +      for (i = size; i < newsize; i++)
> +        new_environ[i] = NULL;
>  
>        last_environ = __environ = new_environ;
> +      allocated = newsize;
> +      ep = new_environ + size;
>      }
>    if (*ep == NULL || replace)
>      {
> @@ -288,13 +294,6 @@ clearenv (void)
>  {
>    LOCK;
>  
> -  if (__environ == last_environ && __environ != NULL)
> -    {
> -      /* We allocated this environment so we can free it.  */
> -      free (__environ);
> -      last_environ = NULL;
> -    }
> -
>    /* Clear the environment pointer removes the whole environment.  */
>    __environ = NULL;
>  
> -- 
> 1.8.4.rc3

[PING^4][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping
On Tue, Nov 05, 2013 at 03:24:58PM +0100, Ondřej Bílka wrote:
> ping,
> 
> Changelog became 
> 
> 	[BZ #15894]
> 	* stdlib/setenv.c: Remove duplicate code.
> 
> On Wed, Oct 30, 2013 at 04:33:42PM +0100, Ondřej Bílka wrote:
> > ping
> > On Wed, Oct 23, 2013 at 03:17:26PM +0200, Ondřej Bílka wrote:
> > > ping
> > > On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > > > Hi,
> > > > 
> > > > In setenv.c file a logic of adding element is needlessly duplicated
> > > > based on if we extend __environ or not. This can be easily fixed in
> > > > following way:
> > > > 
> > > > 	* stdlib/setenv.c: Remove duplicate code.
> > > > 
> > > > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > > > index 63c995b..e4b5b5e 100644
> > > > --- a/stdlib/setenv.c
> > > > +++ b/stdlib/setenv.c
> > > > @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
> > > >  	  UNLOCK;
> > > >  	  return -1;
> > > >  	}
> > > > -
> > > > -      /* If the whole entry is given add it.  */
> > > > -      if (combined != NULL)
> > > > -	/* We must not add the string to the search tree since it belongs
> > > > -	   to the user.  */
> > > > -	new_environ[size] = (char *) combined;
> > > > -      else
> > > > -	{
> > > > -	  /* See whether the value is already known.  */
> > > > -#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
> > > > -	    {
> > > > -	      new_value = malloc (varlen);
> > > > -	      if (new_value == NULL)
> > > > -		{
> > > > -		  UNLOCK;
> > > > -		  if (last_environ == NULL)
> > > > -		    free (new_environ);
> > > > -		  return -1;
> > > > -		}
> > > > -	    }
> > > > -# ifdef _LIBC
> > > > -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> > > > -		     value, vallen);
> > > > -# else
> > > > -	  memcpy (new_value, name, namelen);
> > > > -	  new_value[namelen] = '=';
> > > > -	  memcpy (&new_value[namelen + 1], value, vallen);
> > > > -# endif
> > > > -
> > > > -	  new_environ[size] = KNOWN_VALUE (new_value);
> > > > -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> > > > -#endif
> > > > -	    {
> > > > -#ifdef USE_TSEARCH
> > > > -	      if (__builtin_expect (! use_alloca, 0))
> > > > -		new_environ[size] = new_value;
> > > > -	      else
> > > > -#endif
> > > > -		{
> > > > -		  new_environ[size] = (char *) malloc (varlen);
> > > > -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> > > > -		    {
> > > > -		      UNLOCK;
> > > > -		      return -1;
> > > > -		    }
> > > > -
> > > > -#ifdef USE_TSEARCH
> > > > -		  memcpy (new_environ[size], new_value, varlen);
> > > > -#else
> > > > -		  memcpy (new_environ[size], name, namelen);
> > > > -		  new_environ[size][namelen] = '=';
> > > > -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> > > > -#endif
> > > > -		}
> > > > -
> > > > -	      /* And save the value now.  We cannot do this when we remove
> > > > -		 the string since then we cannot decide whether it is a
> > > > -		 user string or not.  */
> > > > -	      STORE_VALUE (new_environ[size]);
> > > > -	    }
> > > > -	}
> > > > -
> > > > -      if (__environ != last_environ)
> > > > -	memcpy ((char *) new_environ, (char *) __environ,
> > > > -		size * sizeof (char *));
> > > > -
> > > > +      new_environ[size] = NULL;
> > > >        new_environ[size + 1] = NULL;
> > > > +      ep = new_environ + size;
> > > >  
> > > >        last_environ = __environ = new_environ;
> > > >      }
> > > > -  else if (replace)
> > > > +  if (*ep == NULL || replace)
> > > >      {
> > > >        char *np;
> > > >  
> > > 
> > > -- 
> > > 
> > > pseudo-user on a pseudo-terminal
> > 
> > -- 
> > 
> > operators on strike due to broken coffee machine
> 
> -- 
> 
> Hot Java has gone cold

-- 

The electrician didn't know what the yellow cable was so he yanked the ethernet out.

Re: [PING^5][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping
On Tue, Nov 26, 2013 at 08:24:09PM +0100, Ondřej Bílka wrote:
> ping
> On Tue, Nov 05, 2013 at 03:24:58PM +0100, Ondřej Bílka wrote:
> > ping,
> > 
> > Changelog became 
> > 
> > 	[BZ #15894]
> > 	* stdlib/setenv.c: Remove duplicate code.
> > 
> > On Wed, Oct 30, 2013 at 04:33:42PM +0100, Ondřej Bílka wrote:
> > > ping
> > > On Wed, Oct 23, 2013 at 03:17:26PM +0200, Ondřej Bílka wrote:
> > > > ping
> > > > On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > > > > Hi,
> > > > > 
> > > > > In setenv.c file a logic of adding element is needlessly duplicated
> > > > > based on if we extend __environ or not. This can be easily fixed in
> > > > > following way:
> > > > > 
> > > > > 	* stdlib/setenv.c: Remove duplicate code.
> > > > > 
> > > > > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > > > > index 63c995b..e4b5b5e 100644
> > > > > --- a/stdlib/setenv.c
> > > > > +++ b/stdlib/setenv.c
> > > > > @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
> > > > >  	  UNLOCK;
> > > > >  	  return -1;
> > > > >  	}
> > > > > -
> > > > > -      /* If the whole entry is given add it.  */
> > > > > -      if (combined != NULL)
> > > > > -	/* We must not add the string to the search tree since it belongs
> > > > > -	   to the user.  */
> > > > > -	new_environ[size] = (char *) combined;
> > > > > -      else
> > > > > -	{
> > > > > -	  /* See whether the value is already known.  */
> > > > > -#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
> > > > > -	    {
> > > > > -	      new_value = malloc (varlen);
> > > > > -	      if (new_value == NULL)
> > > > > -		{
> > > > > -		  UNLOCK;
> > > > > -		  if (last_environ == NULL)
> > > > > -		    free (new_environ);
> > > > > -		  return -1;
> > > > > -		}
> > > > > -	    }
> > > > > -# ifdef _LIBC
> > > > > -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> > > > > -		     value, vallen);
> > > > > -# else
> > > > > -	  memcpy (new_value, name, namelen);
> > > > > -	  new_value[namelen] = '=';
> > > > > -	  memcpy (&new_value[namelen + 1], value, vallen);
> > > > > -# endif
> > > > > -
> > > > > -	  new_environ[size] = KNOWN_VALUE (new_value);
> > > > > -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> > > > > -#endif
> > > > > -	    {
> > > > > -#ifdef USE_TSEARCH
> > > > > -	      if (__builtin_expect (! use_alloca, 0))
> > > > > -		new_environ[size] = new_value;
> > > > > -	      else
> > > > > -#endif
> > > > > -		{
> > > > > -		  new_environ[size] = (char *) malloc (varlen);
> > > > > -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> > > > > -		    {
> > > > > -		      UNLOCK;
> > > > > -		      return -1;
> > > > > -		    }
> > > > > -
> > > > > -#ifdef USE_TSEARCH
> > > > > -		  memcpy (new_environ[size], new_value, varlen);
> > > > > -#else
> > > > > -		  memcpy (new_environ[size], name, namelen);
> > > > > -		  new_environ[size][namelen] = '=';
> > > > > -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> > > > > -#endif
> > > > > -		}
> > > > > -
> > > > > -	      /* And save the value now.  We cannot do this when we remove
> > > > > -		 the string since then we cannot decide whether it is a
> > > > > -		 user string or not.  */
> > > > > -	      STORE_VALUE (new_environ[size]);
> > > > > -	    }
> > > > > -	}
> > > > > -
> > > > > -      if (__environ != last_environ)
> > > > > -	memcpy ((char *) new_environ, (char *) __environ,
> > > > > -		size * sizeof (char *));
> > > > > -
> > > > > +      new_environ[size] = NULL;
> > > > >        new_environ[size + 1] = NULL;
> > > > > +      ep = new_environ + size;
> > > > >  
> > > > >        last_environ = __environ = new_environ;
> > > > >      }
> > > > > -  else if (replace)
> > > > > +  if (*ep == NULL || replace)
> > > > >      {
> > > > >        char *np;
> > > > >  
> > > > 
> > > > -- 
> > > > 
> > > > pseudo-user on a pseudo-terminal
> > > 
> > > -- 
> > > 
> > > operators on strike due to broken coffee machine
> > 
> > -- 
> > 
> > Hot Java has gone cold
> 
> -- 
> 
> The electrician didn't know what the yellow cable was so he yanked the ethernet out.

-- 

Maintenance window broken

Re: [PING^6][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

ping
On Wed, Dec 04, 2013 at 12:34:38PM +0100, Ondřej Bílka wrote:
> ping
> On Tue, Nov 26, 2013 at 08:24:09PM +0100, Ondřej Bílka wrote:
> > ping
> > On Tue, Nov 05, 2013 at 03:24:58PM +0100, Ondřej Bílka wrote:
> > > ping,
> > > 
> > > Changelog became 
> > > 
> > > 	[BZ #15894]
> > > 	* stdlib/setenv.c: Remove duplicate code.
> > > 
> > > On Wed, Oct 30, 2013 at 04:33:42PM +0100, Ondřej Bílka wrote:
> > > > ping
> > > > On Wed, Oct 23, 2013 at 03:17:26PM +0200, Ondřej Bílka wrote:
> > > > > ping
> > > > > On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > In setenv.c file a logic of adding element is needlessly duplicated
> > > > > > based on if we extend __environ or not. This can be easily fixed in
> > > > > > following way:
> > > > > > 
> > > > > > 	* stdlib/setenv.c: Remove duplicate code.
> > > > > > 
> > > > > > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > > > > > index 63c995b..e4b5b5e 100644
> > > > > > --- a/stdlib/setenv.c
> > > > > > +++ b/stdlib/setenv.c
> > > > > > @@ -146,82 +146,13 @@ __add_to_environ (name, value, combined, replace)
> > > > > >  	  UNLOCK;
> > > > > >  	  return -1;
> > > > > >  	}
> > > > > > -
> > > > > > -      /* If the whole entry is given add it.  */
> > > > > > -      if (combined != NULL)
> > > > > > -	/* We must not add the string to the search tree since it belongs
> > > > > > -	   to the user.  */
> > > > > > -	new_environ[size] = (char *) combined;
> > > > > > -      else
> > > > > > -	{
> > > > > > -	  /* See whether the value is already known.  */
> > > > > > -#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
> > > > > > -	    {
> > > > > > -	      new_value = malloc (varlen);
> > > > > > -	      if (new_value == NULL)
> > > > > > -		{
> > > > > > -		  UNLOCK;
> > > > > > -		  if (last_environ == NULL)
> > > > > > -		    free (new_environ);
> > > > > > -		  return -1;
> > > > > > -		}
> > > > > > -	    }
> > > > > > -# ifdef _LIBC
> > > > > > -	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
> > > > > > -		     value, vallen);
> > > > > > -# else
> > > > > > -	  memcpy (new_value, name, namelen);
> > > > > > -	  new_value[namelen] = '=';
> > > > > > -	  memcpy (&new_value[namelen + 1], value, vallen);
> > > > > > -# endif
> > > > > > -
> > > > > > -	  new_environ[size] = KNOWN_VALUE (new_value);
> > > > > > -	  if (__builtin_expect (new_environ[size] == NULL, 1))
> > > > > > -#endif
> > > > > > -	    {
> > > > > > -#ifdef USE_TSEARCH
> > > > > > -	      if (__builtin_expect (! use_alloca, 0))
> > > > > > -		new_environ[size] = new_value;
> > > > > > -	      else
> > > > > > -#endif
> > > > > > -		{
> > > > > > -		  new_environ[size] = (char *) malloc (varlen);
> > > > > > -		  if (__builtin_expect (new_environ[size] == NULL, 0))
> > > > > > -		    {
> > > > > > -		      UNLOCK;
> > > > > > -		      return -1;
> > > > > > -		    }
> > > > > > -
> > > > > > -#ifdef USE_TSEARCH
> > > > > > -		  memcpy (new_environ[size], new_value, varlen);
> > > > > > -#else
> > > > > > -		  memcpy (new_environ[size], name, namelen);
> > > > > > -		  new_environ[size][namelen] = '=';
> > > > > > -		  memcpy (&new_environ[size][namelen + 1], value, vallen);
> > > > > > -#endif
> > > > > > -		}
> > > > > > -
> > > > > > -	      /* And save the value now.  We cannot do this when we remove
> > > > > > -		 the string since then we cannot decide whether it is a
> > > > > > -		 user string or not.  */
> > > > > > -	      STORE_VALUE (new_environ[size]);
> > > > > > -	    }
> > > > > > -	}
> > > > > > -
> > > > > > -      if (__environ != last_environ)
> > > > > > -	memcpy ((char *) new_environ, (char *) __environ,
> > > > > > -		size * sizeof (char *));
> > > > > > -
> > > > > > +      new_environ[size] = NULL;
> > > > > >        new_environ[size + 1] = NULL;
> > > > > > +      ep = new_environ + size;
> > > > > >  
> > > > > >        last_environ = __environ = new_environ;
> > > > > >      }
> > > > > > -  else if (replace)
> > > > > > +  if (*ep == NULL || replace)
> > > > > >      {
> > > > > >        char *np;
> > > > > >  
> > > > > 
> > > > > -- 
> > > > > 
> > > > > pseudo-user on a pseudo-terminal
> > > > 
> > > > -- 
> > > > 
> > > > operators on strike due to broken coffee machine
> > > 
> > > -- 
> > > 
> > > Hot Java has gone cold
> > 
> > -- 
> > 
> > The electrician didn't know what the yellow cable was so he yanked the ethernet out.
> 
> -- 
> 
> Maintenance window broken

-- 

paradigm shift...without a clutch

Re: [PING^6][BZ #15894][PATCH] Deduplicate setenv.c

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 11 bytes --]

LGTM
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PING^6][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

On Sat, Feb 08, 2014 at 09:52:32AM -0500, Mike Frysinger wrote:
> LGTM
> -mike

I commited a sligthly different version as varlen became unused
variable.

diff --git a/ChangeLog b/ChangeLog
index bded2c3..cf1b17d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
 
+	[BZ #15894]
+	* stdlib/setenv.c (__add_to_environ): Remove duplicate code.
+
+2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
+
 	* malloc/arena.c (grow_heap, get_free_list, reused_arena,
 	arena_get2): Remove THREAD_STATS conditionals.
 	* malloc/malloc.c (__malloc_assert, __libc_realloc, _int_free,
diff --git a/NEWS b/NEWS
index 851167f..0f4b8d4 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,8 @@ Version 2.20
 
 * The following bugs are resolved with this release:
 
+  15894.
+
 * The am33 port, which had not worked for several years, has been removed
   from ports.
 \f
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index 7df5b3f..bb99c2a 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -135,7 +135,6 @@ __add_to_environ (name, value, combined, replace)
 
   if (ep == NULL || __builtin_expect (*ep == NULL, 1))
     {
-      const size_t varlen = namelen + 1 + vallen;
       char **new_environ;
 
       /* We allocated this space; we can extend it.  */
@@ -147,81 +146,17 @@ __add_to_environ (name, value, combined, replace)
 	  return -1;
 	}
 
-      /* If the whole entry is given add it.  */
-      if (combined != NULL)
-	/* We must not add the string to the search tree since it belongs
-	   to the user.  */
-	new_environ[size] = (char *) combined;
-      else
-	{
-	  /* See whether the value is already known.  */
-#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
-	    {
-	      new_value = malloc (varlen);
-	      if (new_value == NULL)
-		{
-		  UNLOCK;
-		  if (last_environ == NULL)
-		    free (new_environ);
-		  return -1;
-		}
-	    }
-# ifdef _LIBC
-	  __mempcpy (__mempcpy (__mempcpy (new_value, name, namelen), "=", 1),
-		     value, vallen);
-# else
-	  memcpy (new_value, name, namelen);
-	  new_value[namelen] = '=';
-	  memcpy (&new_value[namelen + 1], value, vallen);
-# endif
-
-	  new_environ[size] = KNOWN_VALUE (new_value);
-	  if (__builtin_expect (new_environ[size] == NULL, 1))
-#endif
-	    {
-#ifdef USE_TSEARCH
-	      if (__builtin_expect (! use_alloca, 0))
-		new_environ[size] = new_value;
-	      else
-#endif
-		{
-		  new_environ[size] = (char *) malloc (varlen);
-		  if (__builtin_expect (new_environ[size] == NULL, 0))
-		    {
-		      UNLOCK;
-		      return -1;
-		    }
-
-#ifdef USE_TSEARCH
-		  memcpy (new_environ[size], new_value, varlen);
-#else
-		  memcpy (new_environ[size], name, namelen);
-		  new_environ[size][namelen] = '=';
-		  memcpy (&new_environ[size][namelen + 1], value, vallen);
-#endif
-		}
-
-	      /* And save the value now.  We cannot do this when we remove
-		 the string since then we cannot decide whether it is a
-		 user string or not.  */
-	      STORE_VALUE (new_environ[size]);
-	    }
-	}
-
       if (__environ != last_environ)
 	memcpy ((char *) new_environ, (char *) __environ,
 		size * sizeof (char *));
 
+      new_environ[size] = NULL;
       new_environ[size + 1] = NULL;
+      ep = new_environ + size;
 
       last_environ = __environ = new_environ;
     }
-  else if (replace)
+  if (*ep == NULL || replace)
     {
       char *np;
 

Re: [PING^6][BZ #15894][PATCH] Deduplicate setenv.c

[Siddhesh Poyarekar]

On Mon, Feb 10, 2014 at 01:59:19PM +0100, Ondřej Bílka wrote:
> On Sat, Feb 08, 2014 at 09:52:32AM -0500, Mike Frysinger wrote:
> > LGTM
> > -mike
> 
> I commited a sligthly different version as varlen became unused
> variable.
> 
> diff --git a/ChangeLog b/ChangeLog
> index bded2c3..cf1b17d 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,10 @@
>  2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
>  
> +	[BZ #15894]
> +	* stdlib/setenv.c (__add_to_environ): Remove duplicate code.
> +
> +2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
> +
>  	* malloc/arena.c (grow_heap, get_free_list, reused_arena,
>  	arena_get2): Remove THREAD_STATS conditionals.
>  	* malloc/malloc.c (__malloc_assert, __libc_realloc, _int_free,

These need to be collapsed into a single entry separated by a blank
line:

2014-02-10  Ondřej Bílka  <neleai@seznam.cz>

	 [BZ #15894]
	 * stdlib/setenv.c (__add_to_environ): Remove duplicate code.

	 * malloc/arena.c (grow_heap, get_free_list, reused_arena,
	 arena_get2): Remove THREAD_STATS conditionals.
	 * malloc/malloc.c (__malloc_assert, __libc_realloc, _int_free,


> @@ -135,7 +135,6 @@ __add_to_environ (name, value, combined, replace)
>  
>    if (ep == NULL || __builtin_expect (*ep == NULL, 1))
>      {
> -      const size_t varlen = namelen + 1 + vallen;
>        char **new_environ;
>  

I think you forgot to do this - I can still see this line and the
build warning it's causing.

Siddhesh

Re: [PING^6][BZ #15894][PATCH] Deduplicate setenv.c

[Ondřej Bílka]

On Tue, Feb 11, 2014 at 04:08:41PM +0530, Siddhesh Poyarekar wrote:
> On Mon, Feb 10, 2014 at 01:59:19PM +0100, Ondřej Bílka wrote:
> > On Sat, Feb 08, 2014 at 09:52:32AM -0500, Mike Frysinger wrote:
> > > LGTM
> > > -mike
> > 
> > I commited a sligthly different version as varlen became unused
> > variable.
> > 
> > diff --git a/ChangeLog b/ChangeLog
> > index bded2c3..cf1b17d 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,5 +1,10 @@
> >  2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
> >  
> > +	[BZ #15894]
> > +	* stdlib/setenv.c (__add_to_environ): Remove duplicate code.
> > +
> > +2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
> > +
> >  	* malloc/arena.c (grow_heap, get_free_list, reused_arena,
> >  	arena_get2): Remove THREAD_STATS conditionals.
> >  	* malloc/malloc.c (__malloc_assert, __libc_realloc, _int_free,
> 
> These need to be collapsed into a single entry separated by a blank
> line:
> 
> 2014-02-10  Ondřej Bílka  <neleai@seznam.cz>
> 
> 	 [BZ #15894]
> 	 * stdlib/setenv.c (__add_to_environ): Remove duplicate code.
> 
> 	 * malloc/arena.c (grow_heap, get_free_list, reused_arena,
> 	 arena_get2): Remove THREAD_STATS conditionals.
> 	 * malloc/malloc.c (__malloc_assert, __libc_realloc, _int_free,
> 
> 
> > @@ -135,7 +135,6 @@ __add_to_environ (name, value, combined, replace)
> >  
> >    if (ep == NULL || __builtin_expect (*ep == NULL, 1))
> >      {
> > -      const size_t varlen = namelen + 1 + vallen;
> >        char **new_environ;
> >  
> 
> I think you forgot to do this - I can still see this line and the
> build warning it's causing.

Thanks, I applied a older version of patch than I send, fixed now.

[PING][PATCH 2a][BZ #15607] Make setenv thread safe.

[Ondřej Bílka]

ping
On Sat, Nov 09, 2013 at 10:31:22AM +0100, Ondřej Bílka wrote:
> ping
> On Tue, Oct 15, 2013 at 02:22:07PM +0200, Ondřej Bílka wrote:
> > On Tue, Oct 15, 2013 at 11:34:50AM +0200, Ondřej Bílka wrote:
> > > Hi,
> > > 
> > > In setenv.c file a logic of adding element is needlessly duplicated
> > > based on if we extend __environ or not. This can be easily fixed in
> > > following way:
> > > 
> > Previos patch cleared setenv implementation for this patch. 
> > A problem in this bug entry is that getenv can segfault when other
> > thread calls setenv which reallocates environment. As we need to leak
> > getenv entries we can also affort to leak old enviroments.
> > 
> > We need to double size for each reallocation to ensure that amount of
> > space allocated is at most double of space occupied by current environ
> > array.
> > 
> > This does not make getenv completely reentrant, a unsetenv could cause a
> > getenv call with unrelated key to fail.
> > 
> > A doubling of allocations is needed for future speeding lookups by hash
> > table, in case that we do not want this patch I will prepare a 2b
> > version that deallocates environments.
> > 
> > This patch causes a intl/mtrace-tst-gettext fail with memory not freed
> > message. How to suppress this?
> > 
> > 
> > 	[BZ #15607]
> > 	* stdlib/setenv.c: Make getenv thread safe.
> > 
> > ---
> >  stdlib/setenv.c | 35 +++++++++++++++++------------------
> >  1 file changed, 17 insertions(+), 18 deletions(-)
> > 
> > diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> > index 5524cc0..29faebf 100644
> > --- a/stdlib/setenv.c
> > +++ b/stdlib/setenv.c
> > @@ -97,7 +97,7 @@ static void *known_values;
> >  /* If this variable is not a null pointer we allocated the current
> >     environment.  */
> >  static char **last_environ;
> > -
> > +static size_t allocated;
> >  
> >  /* This function is used by `setenv' and `putenv'.  The difference between
> >     the two functions is that for the former must create a new string which
> > @@ -133,28 +133,34 @@ __add_to_environ (name, value, combined, replace)
> >  	  ++size;
> >      }
> >  
> > -  if (ep == NULL || __builtin_expect (*ep == NULL, 1))
> > +  if (ep == NULL || (*ep == NULL && __environ != last_environ) ||
> > +      (__environ == last_environ && size == allocated - 1))
> >      {
> >        char **new_environ;
> > +      size_t i;
> > +      size_t newsize = 2 * size + 2;
> >  
> > -      /* We allocated this space; we can extend it.  */
> > -      new_environ = (char **) realloc (last_environ,
> > -				       (size + 2) * sizeof (char *));
> > +      /* We need to keep old environments to make getenv thread safe.  */
> > +      new_environ = (char **) malloc ((newsize + 1) * sizeof (char *));
> >        if (new_environ == NULL)
> >  	{
> >  	  UNLOCK;
> >  	  return -1;
> >  	}
> >  
> > -      if (__environ != last_environ)
> > -	memcpy ((char *) new_environ, (char *) __environ,
> > -		size * sizeof (char *));
> > +      /* To keep valgrind from reporting leak.  */
> > +      new_environ[0] = last_environ;
> > +      new_environ++;
> >  
> > -      new_environ[size] = NULL;
> > -      ep = new_environ + size;
> > -      new_environ[size + 1] = NULL;
> > +      memcpy ((char *) new_environ, (char *) __environ,
> > +              size * sizeof (char *));
> > +
> > +      for (i = size; i < newsize; i++)
> > +        new_environ[i] = NULL;
> >  
> >        last_environ = __environ = new_environ;
> > +      allocated = newsize;
> > +      ep = new_environ + size;
> >      }
> >    if (*ep == NULL || replace)
> >      {
> > @@ -288,13 +294,6 @@ clearenv (void)
> >  {
> >    LOCK;
> >  
> > -  if (__environ == last_environ && __environ != NULL)
> > -    {
> > -      /* We allocated this environment so we can free it.  */
> > -      free (__environ);
> > -      last_environ = NULL;
> > -    }
> > -
> >    /* Clear the environment pointer removes the whole environment.  */
> >    __environ = NULL;
> >  
> > -- 
> > 1.8.4.rc3