From 0b87551bdfb74ac411caa335d8ad0b89a7f139c6 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Sat, 24 Oct 2020 13:34:29 -0400 Subject: lift sigaction abort locking to fix posix_spawn child deadlock commit 25ea9f712c30c32957de493d4711ee39d0bbb024 introduced a deadlock to the posix_spawn child whereby, if abort was called in the parent and ended up taking the abort lock to terminate the process, the __libc_sigaction calls in the child would wait forever to obtain a lock that would not be released. this could be fixed by having abort set the abort lock as the exit futex address, but it's cleaner to just remove the SIGABRT special handling from the internal __libc_sigaction and lift it to the public sigaction function. nothing but the posix_spawn child calls __libc_sigaction on SIGABRT, and since commit b7bc966522d73e1dc420b5ee6fc7a2e78099a08c the abort lock is held at the time of __clone, which precludes the child inheriting a kernel-level signal disposition inconsistent with the disposition on the abstract machine. this means it's fine to inspect and modify the disposition in the child without a lock. --- src/signal/sigaction.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/signal/sigaction.c b/src/signal/sigaction.c index 7232d4b9..2203471b 100644 --- a/src/signal/sigaction.c +++ b/src/signal/sigaction.c @@ -20,14 +20,6 @@ volatile int __eintr_valid_flag; int __libc_sigaction(int sig, const struct sigaction *restrict sa, struct sigaction *restrict old) { struct k_sigaction ksa, ksa_old; - unsigned long set[_NSIG/(8*sizeof(long))]; - /* Doing anything with the disposition of SIGABRT requires a lock, - * so that it cannot be changed while abort is terminating the - * process and so any change made by abort can't be observed. */ - if (sig == SIGABRT) { - __block_all_sigs(&set); - LOCK(__abort_lock); - } if (sa) { if ((uintptr_t)sa->sa_handler > 1UL) { a_or_l(handler_set+(sig-1)/(8*sizeof(long)), @@ -57,10 +49,6 @@ int __libc_sigaction(int sig, const struct sigaction *restrict sa, struct sigact memcpy(&ksa.mask, &sa->sa_mask, _NSIG/8); } int r = __syscall(SYS_rt_sigaction, sig, sa?&ksa:0, old?&ksa_old:0, _NSIG/8); - if (sig == SIGABRT) { - UNLOCK(__abort_lock); - __restore_sigs(&set); - } if (old && !r) { old->sa_handler = ksa_old.handler; old->sa_flags = ksa_old.flags; @@ -71,11 +59,26 @@ int __libc_sigaction(int sig, const struct sigaction *restrict sa, struct sigact int __sigaction(int sig, const struct sigaction *restrict sa, struct sigaction *restrict old) { + unsigned long set[_NSIG/(8*sizeof(long))]; + if (sig-32U < 3 || sig-1U >= _NSIG-1) { errno = EINVAL; return -1; } - return __libc_sigaction(sig, sa, old); + + /* Doing anything with the disposition of SIGABRT requires a lock, + * so that it cannot be changed while abort is terminating the + * process and so any change made by abort can't be observed. */ + if (sig == SIGABRT) { + __block_all_sigs(&set); + LOCK(__abort_lock); + } + int r = __libc_sigaction(sig, sa, old); + if (sig == SIGABRT) { + UNLOCK(__abort_lock); + __restore_sigs(&set); + } + return r; } weak_alias(__sigaction, sigaction); -- cgit v1.2.1