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

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

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