commit 9f0d2c0ee6c728643fcf9a4879e9f20f5e45ce5f Author: Arjun Shankar Date: Fri Oct 18 16:03:25 2024 +0200 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 Conflicts: libio/Makefile (Usual conflict when adding tests due to missing backports.) posix/fork.c (reworked due to file rename upstream and libpthread integration) diff -Nrup a/libio/Makefile b/libio/Makefile --- a/libio/Makefile 2025-04-23 15:51:08.439579602 -0400 +++ b/libio/Makefile 2025-04-23 15:11:32.124866600 -0400 @@ -62,7 +62,7 @@ tests = tst_swprintf tst_wprintf tst_sws tst-memstream1 tst-memstream2 tst-memstream3 \ tst-wmemstream1 tst-wmemstream2 tst-wmemstream3 \ bug-memstream1 bug-wmemstream1 \ - tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ + tst-setvbuf1 tst-popen-fork tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ tst-wfile-sync diff -Nrup a/libio/iopopen.c b/libio/iopopen.c --- a/libio/iopopen.c 2025-04-23 15:51:09.145583660 -0400 +++ b/libio/iopopen.c 2025-04-23 15:07:45.167549822 -0400 @@ -60,6 +60,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 -Nrup a/libio/libioP.h b/libio/libioP.h --- a/libio/libioP.h 2025-04-23 15:51:08.012577147 -0400 +++ b/libio/libioP.h 2025-04-23 15:07:45.167549822 -0400 @@ -423,6 +423,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 -Nrup a/libio/tst-popen-fork.c b/libio/tst-popen-fork.c --- a/libio/tst-popen-fork.c 1969-12-31 19:00:00.000000000 -0500 +++ b/libio/tst-popen-fork.c 2025-04-23 15:07:45.168549828 -0400 @@ -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 + . */ + +#include +#include +#include +#include +#include + +#include +#include +#include + +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 --- a/sysdeps/nptl/fork.c 2025-04-23 16:33:49.283403134 -0400 +++ b/sysdeps/nptl/fork.c 2025-04-23 16:33:12.240190356 -0400 @@ -65,6 +65,7 @@ __libc_fork (void) not matter if fork was called from a signal handler. */ if (multiple_threads) { + _IO_proc_file_chain_lock (); _IO_list_lock (); /* Acquire malloc locks. This needs to come last because fork @@ -121,6 +122,7 @@ __libc_fork (void) /* Reset locks in the I/O code. */ _IO_list_resetlock (); + _IO_proc_file_chain_resetlock (); } /* Reset the lock the dynamic loader uses to protect its data. */ @@ -142,6 +144,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. */