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>
This commit is contained in:
Arjun Shankar 2024-10-18 16:03:25 +02:00
parent 81439a116c
commit 9f0d2c0ee6
5 changed files with 110 additions and 0 deletions

View File

@ -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 \

View File

@ -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.

View File

@ -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;

80
libio/tst-popen-fork.c Normal file
View File

@ -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>

View File

@ -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. */