libio: Fix a deadlock after fork in popen

popen modifies its file handler book-keeping under a lock that wasn't
being taken during fork.  This meant that a concurrent popen and fork
could end up copying the lock in a "locked" state into the fork child,
where subsequently calling popen would lead to a deadlock due to the
already (spuriously) held lock.

This commit fixes the deadlock by appropriately taking the lock before
fork, and releasing/resetting it in the parent/child after the fork.

A new test for concurrent popen and fork is also added.  It consistently
hangs (and therefore fails via timeout) without the fix applied.
Reviewed-by: Florian Weimer <fweimer@redhat.com>

diff --git a/libio/Makefile b/libio/Makefile
index ae704d8767eceb3a3b9bf443b98ef024e607f74e..018c26d971d17b9d40b45edb0c77187a9a2a0956 100644 (file)
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -119,6 +119,7 @@ tests = \
   tst-mmap-offend \
   tst-mmap-setvbuf \
   tst-mmap2-eofsync \
+  tst-popen-fork \
   tst-popen1 \
   tst-setvbuf1 \
   tst-sprintf-chk-ub \

diff --git a/libio/iopopen.c b/libio/iopopen.c
index d01cb0648e3aac54acf28e69db138f79475536b5..352513a2914a9d36463533fa03404a6c7927f51e 100644 (file)
--- a/libio/iopopen.c
+++ b/libio/iopopen.c
@@ -57,6 +57,26 @@ unlock (void *not_used)
 }
 #endif
 
+/* These lock/unlock/resetlock functions are used during fork.  */
+
+void
+_IO_proc_file_chain_lock (void)
+{
+  _IO_lock_lock (proc_file_chain_lock);
+}
+
+void
+_IO_proc_file_chain_unlock (void)
+{
+  _IO_lock_unlock (proc_file_chain_lock);
+}
+
+void
+_IO_proc_file_chain_resetlock (void)
+{
+  _IO_lock_init (proc_file_chain_lock);
+}
+
 /* POSIX states popen shall ensure that any streams from previous popen()
    calls that remain open in the parent process should be closed in the new
    child process.

diff --git a/libio/libioP.h b/libio/libioP.h
index 616253fcd00f04db0253343aa6d32790862e2bb7..a83a411fdf7d93c9a81b6150b0dd4e06dba01667 100644 (file)
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -429,6 +429,12 @@ libc_hidden_proto (_IO_list_resetlock)
 extern void _IO_enable_locks (void) __THROW;
 libc_hidden_proto (_IO_enable_locks)
 
+/* Functions for operating popen's proc_file_chain_lock during fork.  */
+
+extern void _IO_proc_file_chain_lock (void) __THROW attribute_hidden;
+extern void _IO_proc_file_chain_unlock (void) __THROW attribute_hidden;
+extern void _IO_proc_file_chain_resetlock (void) __THROW attribute_hidden;
+
 /* Default jumptable functions. */
 
 extern int _IO_default_underflow (FILE *) __THROW;

diff --git a/libio/tst-popen-fork.c b/libio/tst-popen-fork.c
new file mode 100644 (file)
index 0000000..1df30fc
--- /dev/null
+++ b/libio/tst-popen-fork.c
@@ -0,0 +1,80 @@
+/* Test concurrent popen and 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/>.  */
+
+#include <stdio.h>
+#include <stdatomic.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+#include <support/check.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+
+static void
+popen_and_pclose (void)
+{
+  FILE *f = popen ("true", "r");
+  TEST_VERIFY_EXIT (f != NULL);
+  pclose (f);
+  return;
+}
+
+static atomic_bool done = ATOMIC_VAR_INIT (0);
+
+static void *
+popen_and_pclose_forever (__attribute__ ((unused))
+                          void *arg)
+{
+  while (!atomic_load_explicit (&done, memory_order_acquire))
+    popen_and_pclose ();
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+
+  /* Repeatedly call popen in a loop during the entire test.  */
+  pthread_t t = xpthread_create (NULL, popen_and_pclose_forever, NULL);
+
+  /* Repeatedly fork off and reap child processes one-by-one.
+     Each child calls popen once, then exits, leading to the possibility
+     that a child forks *during* our own popen call, thus inheriting any
+     intermediate popen state, possibly including lock state(s).  */
+  for (int i = 0; i < 100; i++)
+    {
+      int cpid = xfork ();
+
+      if (cpid == 0)
+        {
+          popen_and_pclose ();
+          _exit (0);
+        }
+      else
+        xwaitpid (cpid, NULL, 0);
+    }
+
+  /* Stop calling popen.  */
+  atomic_store_explicit (&done, 1, memory_order_release);
+  xpthread_join (t);
+
+  return 0;
+}
+
+#include <support/test-driver.c>

diff --git a/posix/fork.c b/posix/fork.c
index c2b476ff2d502504deaaec5cbc1c726c5ad95c6d..bd6371a9f469fcffb14ef665ac272febbad20804 100644 (file)
--- a/posix/fork.c
+++ b/posix/fork.c
@@ -62,6 +62,7 @@ __libc_fork (void)
       call_function_static_weak (__nss_database_fork_prepare_parent,
                                 &nss_database_data);
 
+      _IO_proc_file_chain_lock ();
       _IO_list_lock ();
 
       /* Acquire malloc locks.  This needs to come last because fork
@@ -94,6 +95,7 @@ __libc_fork (void)
 
          /* Reset locks in the I/O code.  */
          _IO_list_resetlock ();
+         _IO_proc_file_chain_resetlock ();
 
          call_function_static_weak (__nss_database_fork_subprocess,
                                     &nss_database_data);
@@ -123,6 +125,7 @@ __libc_fork (void)
 
          /* We execute this even if the 'fork' call failed.  */
          _IO_list_unlock ();
+         _IO_proc_file_chain_unlock ();
        }
 
       /* Run the handlers registered for the parent.  */