getenv not thread safe

[Michael Eager <eager at mvista dot com>]

On an SMP system, it's possible that getenv may be walking the 
environment while setenv is reallocating it.  This results in a SEGV
when getenv accesses memory which has already been free'd.  

Here's a patch which fixes this by having getenv() acquire the same 
lock that setenv uses before walking the environment.   A test case
is also provided.



2004-02-22  Michael Eager <eager@mvista.com>

	* sysdeps/generic/setenv.c (setenv)   Make lock variable 
	global so it can be shared with getenv().
	* sysdeps/generic/getenv.c (getenv)   Get lock before
	walking environment.

Index: sysdeps/generic/getenv.c
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/getenv.c,v
retrieving revision 1.10
diff -u -r1.10 getenv.c
--- sysdeps/generic/getenv.c    3 Aug 2002 12:59:00 -0000       1.10
+++ sysdeps/generic/getenv.c    24 Feb 2004 00:53:49 -0000
@@ -27,6 +27,17 @@
 #define        __environ       environ
 #endif

+#if _LIBC
+/* This lock protects against simultaneous modifications of `environ'.  */
+# include <bits/libc-lock.h>
+__libc_lock_define_initialized (, envlock)
+# define LOCK   __libc_lock_lock (envlock)
+# define UNLOCK __libc_lock_unlock (envlock)
+#else
+# define LOCK
+# define UNLOCK
+#endif
+
 /* Return the value of the environment variable NAME.  This implementation
    is tuned a bit in that it assumes no environment variable has an empty
    name which of course should always be true.  We have a special case for
@@ -58,6 +69,7 @@
  #error "Funny byte order."
 # endif
 #endif
+      LOCK;
       for (ep = __environ; *ep != NULL; ++ep)
        {
 #if _STRING_ARCH_unaligned
@@ -67,8 +79,12 @@
                               | (((unsigned char *) *ep)[1] << 8));
 #endif
          if (name_start == ep_start)
-           return &(*ep)[2];
+           {
+             UNLOCK;
+             return &(*ep)[2];
+           }
        }
+       UNLOCK;
     }
   else
     {
@@ -81,6 +97,7 @@
       len -= 2;
       name += 2;

+      LOCK;
       for (ep = __environ; *ep != NULL; ++ep)
        {
 #if _STRING_ARCH_unaligned
@@ -92,8 +109,12 @@

          if (name_start == ep_start && !strncmp (*ep + 2, name, len)
              && (*ep)[len + 2] == '=')
-           return &(*ep)[len + 3];
+           {
+             UNLOCK;
+             return &(*ep)[len + 3];
+           }
        }
+       UNLOCK;
     }

   return NULL;
Index: sysdeps/generic/setenv.c
===================================================================
RCS file: /cvs/glibc/libc/sysdeps/generic/setenv.c,v
retrieving revision 1.30
diff -u -r1.30 setenv.c
--- sysdeps/generic/setenv.c    11 Feb 2004 06:32:32 -0000      1.30
+++ sysdeps/generic/setenv.c    24 Feb 2004 00:53:49 -0000
@@ -48,7 +48,7 @@
 #if _LIBC
 /* This lock protects against simultaneous modifications of `environ'.  */
 # include <bits/libc-lock.h>
-__libc_lock_define_initialized (static, envlock)
+__libc_lock_define_initialized (, envlock)
 # define LOCK  __libc_lock_lock (envlock)
 # define UNLOCK        __libc_lock_unlock (envlock)
 #else



Test case.  Gets SEGV in getenv on SMP system, runs until canceled
on UP system.  


#include <stdlib.h>
#include <pthread.h>

static const char* names[] = {
        "ENV_0", "ENV_1", "ENV_2", "ENV_3",
        "ENV_4", "ENV_5", "ENV_6", "ENV_7"
};

void* thread_getenv(void* param)
{
        char* env;
        int i;
        for(;;){
                for(i = 0; i < sizeof(names) / sizeof(char*); i++){
                        env = getenv(names[i]);
                }
        }
        return NULL;
}

void* thread_setenv(void* param)
{
        char* var;
        int i, size;
        size = 2;
        for(;;){
                var = (char*)malloc(size);
                if(var == NULL) break;
                memset(var, '0', size - 1);
                var[size - 1] = 0;
                for(i = 0; i < sizeof(names) / sizeof(char*); i++){
                        setenv(names[i], var, 1);
                }
                free(var);
                size++;
        }
        return NULL;
}

void* thread_malloc(void* param)
{
        int size;
        char* ptr;
        size = 16;
        for(;;){
                ptr = (char*)malloc(size);
                memset(ptr, '1', size);
                free(ptr);
                size++;
        }
        return NULL;
}

int main(void)
{
        pthread_t t_get, t_set, t_mem;

        pthread_create(&t_get, NULL, thread_getenv, NULL);
        pthread_create(&t_set, NULL, thread_setenv, NULL);
        pthread_create(&t_mem, NULL, thread_malloc, NULL);

        for(;;) sleep(1);

        return 0;
}

	



--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085