[Bug libc/5069] env thread safety problem

["t-nishiie at soft dot fujitsu dot com" <sourceware-bugzilla at sourceware dot org>]

------- Additional Comments From t-nishiie at soft dot fujitsu dot com  2007-09-26 01:33 -------
(In reply to comment #1)
Hi everyone,

A problem is not solved in changing getenv(). Since __add_to_environ() has 
called realloc() and realloc() has called getenv() inside, a deadlock may 
occur.
If it corrects, __add_to_environ(), unsetenv(), and clearenv() are corrected. 
For example, they are the following corrections proposed by Bugzilla:#4887.

===========
  *putenv()/setenv()/clearenv()/unsetenv() should be thread safe.

diff -ruN glibc-2.6.orig/stdlib/setenv.c glibc-2.6/stdlib/setenv.c
--- glibc-2.6.orig/stdlib/setenv.c	2005-12-14 19:44:05.000000000 +0900
+++ glibc-2.6/stdlib/setenv.c	2007-07-26 19:08:27.000000000 +0900
@@ -98,6 +98,9 @@
 /* If this variable is not a null pointer we allocated the current
    environment.  */
 static char **last_environ;
+static char **environ_region;
+static size_t environ_region_size;
+static char **old_environ_region;
 
 
 /* This function is used by `setenv' and `putenv'.  The difference between
@@ -113,8 +116,9 @@
      const char *combined;
      int replace;
 {
-  register char **ep;
-  register size_t size;
+  char **ep;
+  size_t size;
+  char **old_environ;
   const size_t namelen = strlen (name);
   const size_t vallen = value != NULL ? strlen (value) + 1 : 0;
 
@@ -122,7 +126,7 @@
 
   /* We have to get the pointer now that we have the lock and not earlier
      since another thread might have created a new environment.  */
-  ep = __environ;
+  ep = old_environ = __environ;
 
   size = 0;
   if (ep != NULL)
@@ -137,21 +141,51 @@
   if (ep == NULL || __builtin_expect (*ep == NULL, 1))
     {
       char **new_environ;
+      const size_t pagesize = (size_t) __sysconf(_SC_PAGESIZE);
+      size_t new_environ_region_size;
+      char **new_environ_region;
+      size_t new_size;
 
       /* We allocated this space; we can extend it.  */
-      new_environ = (char **) realloc (last_environ,
-				       (size + 2) * sizeof (char *));
-      if (new_environ == NULL)
+      if (environ_region == NULL || last_environ != old_environ)
+	environ_region_size = 0;
+      new_environ_region_size = pagesize;
+      while (new_environ_region_size < (size + 2) * sizeof (char*))
+	new_environ_region_size *= 2;
+      if (environ_region_size < new_environ_region_size || environ_region == 
NULL)
 	{
-	  UNLOCK;
-	  return -1;
+	  if (environ_region != NULL)
+	    {
+	      if (old_environ_region != NULL)
+		free(old_environ_region);
+	      old_environ_region = environ_region;
+	    }
+	  new_environ_region = (char **) malloc (new_environ_region_size);
+	  if (new_environ_region == NULL)
+	    {
+	      __set_errno (ENOMEM);
+	      UNLOCK;
+	      return -1;
+	    }
+	  new_size = new_environ_region_size / sizeof (char*);
+	  new_environ = new_environ_region + (new_size - (size + 1));
+	  if (size != 0)
+	    memcpy ((char *) new_environ,
+		    (char *) old_environ,
+		    size * sizeof (char *));
+	  new_environ[size + 1] = NULL;
+	  environ_region_size = new_environ_region_size;
+	  environ_region = new_environ_region;
+	  --new_environ;
 	}
+      else
+	new_environ = old_environ - 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;
+	new_environ[0] = (char *) combined;
       else
 	{
 	  /* See whether the value is already known.  */
@@ -170,12 +204,12 @@
 	  memcpy (&new_value[namelen + 1], value, vallen);
 # endif
 
-	  new_environ[size] = KNOWN_VALUE (new_value);
-	  if (__builtin_expect (new_environ[size] == NULL, 1))
+	  new_environ[0] = KNOWN_VALUE (new_value);
+	  if (__builtin_expect (new_environ[0] == NULL, 1))
 #endif
 	    {
-	      new_environ[size] = (char *) malloc (namelen + 1 + vallen);
-	      if (__builtin_expect (new_environ[size] == NULL, 0))
+	      new_environ[0] = (char *) malloc (namelen + 1 + vallen);
+	      if (__builtin_expect (new_environ[0] == NULL, 0))
 		{
 		  __set_errno (ENOMEM);
 		  UNLOCK;
@@ -183,25 +217,19 @@
 		}
 
 #ifdef USE_TSEARCH
-	      memcpy (new_environ[size], new_value, namelen + 1 + vallen);
+	      memcpy (new_environ[0], new_value, namelen + 1 + vallen);
 #else
-	      memcpy (new_environ[size], name, namelen);
-	      new_environ[size][namelen] = '=';
-	      memcpy (&new_environ[size][namelen + 1], value, vallen);
+	      memcpy (new_environ[0], name, namelen);
+	      new_environ[0][namelen] = '=';
+	      memcpy (&new_environ[0][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]);
+	      STORE_VALUE (new_environ[0]);
 	    }
 	}
 
-      if (__environ != last_environ)
-	memcpy ((char *) new_environ, (char *) __environ,
-		size * sizeof (char *));
-
-      new_environ[size + 1] = NULL;
-
       last_environ = __environ = new_environ;
     }
   else if (replace)
@@ -235,6 +263,7 @@
 	      np = malloc (namelen + 1 + vallen);
 	      if (__builtin_expect (np == NULL, 0))
 		{
+		  __set_errno (ENOMEM);
 		  UNLOCK;
 		  return -1;
 		}
@@ -280,6 +309,13 @@
 {
   size_t len;
   char **ep;
+  char **old_environ;
+  size_t size;
+  char **new_environ;
+  const size_t pagesize = (size_t) __sysconf(_SC_PAGESIZE);
+  size_t new_environ_region_size;
+  char **new_environ_region;
+  size_t new_size;
 
   if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
     {
@@ -291,21 +327,68 @@
 
   LOCK;
 
-  ep = __environ;
+  ep = old_environ = __environ;
+  if (ep == NULL)
+    {
+      UNLOCK;
+      return 0;
+    }
+  if (*ep == NULL)
+    {
+      UNLOCK;
+      return 0;
+    }
+
+  if (environ_region == NULL || last_environ != old_environ)
+    environ_region_size = 0;
+  size = 0;
+  for (; *ep != NULL; ++ep)
+     ++size;
+  new_environ_region_size = pagesize;
+  while (new_environ_region_size < (size + 1) * sizeof (char*))
+    new_environ_region_size *= 2;
+  if (environ_region_size < new_environ_region_size || environ_region == NULL)
+    {
+      if (environ_region != NULL)
+	{
+	  if (old_environ_region != NULL)
+	    free(old_environ_region);
+	  old_environ_region = environ_region;
+	}
+      new_environ_region = (char **) malloc (new_environ_region_size);
+      if (new_environ_region == NULL)
+	{
+	  __set_errno (ENOMEM);
+	  UNLOCK;
+	  return -1;
+	}
+      new_size = new_environ_region_size / sizeof (char*);
+      new_environ = new_environ_region + (new_size - (size + 1));
+      if (size != 0)
+	memcpy ((char *) new_environ,
+		(char *) old_environ,
+		size * sizeof (char *));
+      new_environ[size + 1] = NULL;
+      environ_region_size = new_environ_region_size;
+      environ_region = new_environ_region;
+    }
+  else
+    new_environ = old_environ;
+
+  ep = new_environ;
   while (*ep != NULL)
     if (!strncmp (*ep, name, len) && (*ep)[len] == '=')
       {
 	/* Found it.  Remove this pointer by moving later ones back.  */
-	char **dp = ep;
-
-	do
-	  dp[0] = dp[1];
-	while (*dp++);
+	ep[0] = new_environ[0];
+	++new_environ;
 	/* Continue the loop in case NAME appears again.  */
       }
     else
       ++ep;
 
+  last_environ = __environ = new_environ;
+
   UNLOCK;
 
   return 0;
@@ -322,7 +405,11 @@
   if (__environ == last_environ && __environ != NULL)
     {
       /* We allocated this environment so we can free it.  */
-      free (__environ);
+      if (old_environ_region != NULL)
+	free(old_environ_region);
+      old_environ_region = environ_region;
+      environ_region = NULL;
+      environ_region_size = 0;
       last_environ = NULL;
     }
 
@@ -338,6 +425,11 @@
 {
   /* Remove all traces.  */
   clearenv ();
+  if (old_environ_region != NULL)
+    {
+      free (old_environ_region);
+      old_environ_region = NULL;
+    }
 
   /* Now remove the search tree.  */
   __tdestroy (known_values, free);
===========

See also.

(1) See exec(), for restrictions on changing the environment 
    in multi-threaded applications.

http://www.opengroup.org/onlinepubs/009695399/functions/exec.html
  Conforming multi-threaded applications shall not use the environ
variable to access or modify any environment variable while any
other thread is concurrently modifying any environment variable.
A call to any function dependent on any environment variable shall
be considered a use of the environ variable to access that 
environment variable. 

(2) Bugzilla:#4350 Reentrancy problem between strftime() and setenv()
http://sourceware.org/bugzilla/show_bug.cgi?id=4350

(3) Bugzilla:#4887 Wordexp() should be thread safe.
http://sourceware.org/bugzilla/show_bug.cgi?id=4887

(4) getenv not thread safe
http://sources.redhat.com/ml/libc-alpha/2004-02/msg00165.html

(5) getenv not thread safe
http://sources.redhat.com/ml/libc-alpha/2005-08/msg00050.html




-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=5069

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.