This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Making getenv more multi-threading--robust?


[I had initially erroneously sent this to libc-help instead.]

I stumbled over this when running the LibreOffice test suite and getting sporadic crashes in getenv(3). It turns out that LibreOffice contains many places that call getenv (mostly to check whether to enable one tweak or another) at arbitrary times and from arbitrary threads, and apparently also contains the odd call to setenv etc. to modify the environment.

Though getenv is generally known to not be thread-safe, glibc already contains measures to make concurrent calls to setenv etc. thread-safe, so I wonder whether something like the attached 0001-Prevent-races-when-getenv-walks-__environ-while-sete.patch would make sense. While this would not solve my problems with (cross-platform) LibreOffice, it would help prevent glibc getenv crashes as demonstrated by the little program in the patch's commit message.

Stephan
>From 99ee3bea07a169a6bcd49b26d300c3d4a04b679c Mon Sep 17 00:00:00 2001
From: Stephan Bergmann <sbergman@redhat.com>
Date: Thu, 13 Sep 2012 11:52:42 +0200
Subject: [PATCH] Prevent races when getenv walks __environ while setenv
 modifies it

... so that the below program does not crash in getenv:

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

  static void * thread(void * unused) {
    for (;;) {
      getenv("NOT_SET");
    }
  }

  int main() {
    pthread_t t;
    int n;
    char b[sizeof "KEY9999"];
    if (pthread_create(&t, NULL, &thread, NULL) != 0) {
      abort();
    }
    for (n = 0; n != 10000; ++n) {
      sprintf(b, "KEY%d", n);
      if (setenv(b, "", 1) != 0) {
        abort();
      }
    }
    exit(EXIT_SUCCESS);
  }
---
 stdlib/envlock.h | 10 ++++++++++
 stdlib/getenv.c  | 19 ++++++++++++++++---
 stdlib/setenv.c  | 10 ++--------
 3 files changed, 28 insertions(+), 11 deletions(-)
 create mode 100644 stdlib/envlock.h

diff --git a/stdlib/envlock.h b/stdlib/envlock.h
new file mode 100644
index 0000000..8a1fd05
--- /dev/null
+++ b/stdlib/envlock.h
@@ -0,0 +1,10 @@
+#if _LIBC
+/* This lock protects against simultaneous modifications of `environ'.  */
+# include <bits/libc-lock.h>
+__libc_lock_define (extern, __envlock)
+# define LOCK	__libc_lock_lock (__envlock)
+# define UNLOCK	__libc_lock_unlock (__envlock)
+#else
+# define LOCK
+# define UNLOCK
+#endif
diff --git a/stdlib/getenv.c b/stdlib/getenv.c
index 8398dab..5c14455 100644
--- a/stdlib/getenv.c
+++ b/stdlib/getenv.c
@@ -22,6 +22,7 @@
 #include <string.h>
 #include <unistd.h>
 
+#include "envlock.h"
 
 /* Return the value of the environment variable NAME.  This implementation
    is tuned a bit in that it assumes no environment variable has an empty
@@ -37,8 +38,13 @@ getenv (name)
   char **ep;
   uint16_t name_start;
 
+  LOCK;
+
   if (__environ == NULL || name[0] == '\0')
-    return NULL;
+    {
+      UNLOCK;
+      return NULL;
+    }
 
   if (name[1] == '\0')
     {
@@ -63,7 +69,10 @@ getenv (name)
 			       | (((unsigned char *) *ep)[1] << 8));
 #endif
 	  if (name_start == ep_start)
-	    return &(*ep)[2];
+	    {
+	      UNLOCK;
+	      return &(*ep)[2];
+	    }
 	}
     }
   else
@@ -88,10 +97,14 @@ getenv (name)
 
 	  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;
 }
 libc_hidden_def (getenv)
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index d1db356..e6901d3 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -44,15 +44,9 @@ extern char **environ;
 # endif
 #endif
 
+#include "envlock.h"
 #if _LIBC
-/* This lock protects against simultaneous modifications of `environ'.  */
-# include <bits/libc-lock.h>
-__libc_lock_define_initialized (static, envlock)
-# define LOCK	__libc_lock_lock (envlock)
-# define UNLOCK	__libc_lock_unlock (envlock)
-#else
-# define LOCK
-# define UNLOCK
+__libc_lock_define_initialized (, __envlock)
 #endif
 
 /* In the GNU C library we must keep the namespace clean.  */
-- 
1.7.11.4


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]