This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PING][PATCH 2a][BZ #15607] Make setenv thread safe.
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: libc-alpha at sourceware dot org
- Date: Sat, 11 Jul 2015 23:46:54 +0200
- Subject: [PING][PATCH 2a][BZ #15607] Make setenv thread safe.
- Authentication-results: sourceware.org; auth=none
- References: <20131015093450 dot GA1459 at domone dot podge> <20131015122207 dot GA2282 at domone dot podge> <20131109093122 dot GA8314 at domone>
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