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

[OndÅej BÃlka <neleai at seznam dot cz>]

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