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

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

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