diff mbox series

[v2,12/13] posix: Acquire the environment lock during fork

Message ID 4896ad435ba63221db22c6430fbe99405e7753de.1722193092.git.fweimer@redhat.com
State New
Headers show
Series getenv/environ thread safety | expand

Commit Message

Florian Weimer July 28, 2024, 7:03 p.m. UTC 
This makes setenv, unsetenv etc. usable after fork with the
builtin-malloc (avoid deadlocks).  Deadlocks may still occur
with replacement mallocs.  If this turns out to be a problem,
a bit of refactoring should allow calling malloc without
the environment lock in unsetenv.
---
 posix/fork.c              |  9 +++++++++
 stdlib/Makefile           |  2 ++
 stdlib/getenv.c           |  2 ++
 stdlib/setenv.c           |  7 ++-----
 stdlib/setenv.h           |  4 ++++
 stdlib/tst-environ-fork.c | 38 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 57 insertions(+), 5 deletions(-)
 create mode 100644 stdlib/tst-environ-fork.c
diff mbox series

Patch

diff --git a/posix/fork.c b/posix/fork.c
index 298765a1ff..75d0910a94 100644
--- a/posix/fork.c
+++ b/posix/fork.c
@@ -25,6 +25,7 @@ 
 #include <stdio-lock.h>
 #include <sys/single_threaded.h>
 #include <unwind-link.h>
+#include <stdlib/setenv.h>
 
 static void
 fresetlockfiles (void)
@@ -64,6 +65,10 @@  __libc_fork (void)
 
       _IO_list_lock ();
 
+      /* The setenv function acquires this lock, and then the malloc
+	 locks.  Use the same lock order here.  */
+      __libc_lock_lock (__environ_lock);
+
       /* Acquire malloc locks.  This needs to come last because fork
 	 handlers may use malloc, and the libio list lock has an
 	 indirect malloc dependency as well (via the getdelim
@@ -87,6 +92,8 @@  __libc_fork (void)
 	  /* Release malloc locks.  */
 	  call_function_static_weak (__malloc_fork_unlock_child);
 
+	  __libc_lock_unlock (__environ_lock);
+
 	  /* Reset the file list.  These are recursive mutexes.  */
 	  fresetlockfiles ();
 
@@ -119,6 +126,8 @@  __libc_fork (void)
 	  /* Release malloc locks, parent process variant.  */
 	  call_function_static_weak (__malloc_fork_unlock_parent);
 
+	  __libc_lock_unlock (__environ_lock);
+
 	  /* We execute this even if the 'fork' call failed.  */
 	  _IO_list_unlock ();
 	}
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 5934b683b1..5613c34bcb 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -277,6 +277,7 @@  tests := \
   tst-canon-bz26341 \
   tst-cxa_atexit \
   tst-environ \
+  tst-environ-fork \
   tst-getenv-signal \
   tst-getenv-thread \
   tst-getenv-unsetenv \
@@ -639,6 +640,7 @@  $(objpfx)tst-setcontext3.out: tst-setcontext3.sh $(objpfx)tst-setcontext3
 
 $(objpfx)tst-qsort5: $(libm)
 
+$(objpfx)tst-environ-fork: $(shared-thread-library)
 $(objpfx)tst-getenv-signal: $(shared-thread-library)
 $(objpfx)tst-getenv-thread: $(shared-thread-library)
 $(objpfx)tst-getenv-unsetenv: $(shared-thread-library)
diff --git a/stdlib/getenv.c b/stdlib/getenv.c
index ad7f0d00c8..c581ecf951 100644
--- a/stdlib/getenv.c
+++ b/stdlib/getenv.c
@@ -19,7 +19,9 @@ 
 #include <setenv.h>
 #include <string.h>
 #include <unistd.h>
+#include <libc-lock.h>
 
+__libc_lock_define_initialized (, __environ_lock);
 struct environ_array *__environ_array_list;
 environ_counter __environ_counter;
 
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index 6bd0596b79..a97f6de4d4 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -55,11 +55,8 @@  extern char **environ;
 #endif
 
 #if _LIBC
-/* This lock protects against simultaneous modifications of `environ'.  */
-# include <libc-lock.h>
-__libc_lock_define_initialized (static, envlock)
-# define LOCK	__libc_lock_lock (envlock)
-# define UNLOCK	__libc_lock_unlock (envlock)
+# define LOCK	__libc_lock_lock (__environ_lock)
+# define UNLOCK	__libc_lock_unlock (__environ_lock)
 #else
 # define LOCK
 # define UNLOCK
diff --git a/stdlib/setenv.h b/stdlib/setenv.h
index 3e530f0bc0..1a184594ce 100644
--- a/stdlib/setenv.h
+++ b/stdlib/setenv.h
@@ -22,8 +22,12 @@ 
 /* Not actually usable in rtld, but it is required for symbol discovery.  */
 #if IS_IN (libc) || IS_IN (rtld)
 # include <atomic.h>
+# include <libc-lock.h>
 # include <stdbool.h>
 
+/* This lock protects against simultaneous modifications of `environ'.  */
+__libc_lock_define (extern, __environ_lock attribute_hidden);
+
 /* We use an exponential sizing policy for environment arrays.  The
    arrays are not deallocating during the lifetime of the process.
    This adds between one and two additional pointers per active
diff --git a/stdlib/tst-environ-fork.c b/stdlib/tst-environ-fork.c
new file mode 100644
index 0000000000..0e25b324e7
--- /dev/null
+++ b/stdlib/tst-environ-fork.c
@@ -0,0 +1,38 @@ 
+/* Test environment snapshots with fork.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* This test uses the process-creation skeleton, but does not performa
+   re-exec, so the use of PROGRAM is a bit misleading here.  */
+
+#define PROGRAM "tst-environ-fork"
+#include "tst-environ-snapshot-skeleton.c"
+
+static pid_t
+create_process (void)
+{
+  pid_t pid = xfork ();
+  if (pid == 0)
+    {
+      verify_environ ();
+      /* Verify that it is possible to obtain the environment lock.  */
+      TEST_COMPARE (setenv ("unrelated", "unrelated-set", 1), 0);
+      TEST_COMPARE_STRING (getenv ("unrelated"), "unrelated-set");
+      _exit (0);
+    }
+  return pid;
+}