From 9f0d2c0ee6c728643fcf9a4879e9f20f5e45ce5f Mon Sep 17 00:00:00 2001 From: Arjun Shankar Date: Fri, 18 Oct 2024 16:03:25 +0200 Subject: [PATCH] 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 --- libio/Makefile | 1 + libio/iopopen.c | 20 +++++++++++ libio/libioP.h | 6 ++++ libio/tst-popen-fork.c | 80 ++++++++++++++++++++++++++++++++++++++++++ posix/fork.c | 3 ++ 5 files changed, 110 insertions(+) create mode 100644 libio/tst-popen-fork.c diff --git a/libio/Makefile b/libio/Makefile index ae704d8767..018c26d971 100644 --- 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 d01cb0648e..352513a291 100644 --- 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 616253fcd0..a83a411fdf 100644 --- 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 index 0000000000..1df30fc6c0 --- /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 + . */ + +#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 diff --git a/posix/fork.c b/posix/fork.c index c2b476ff2d..bd6371a9f4 100644 --- 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. */ -- 2.43.5