Hello,
I just stumbled upon a bug today and this is my suggested fix.
Greetings Markus
Configuration Information [Automatically generated, do not change]:
Machine: x86_64
OS: linux-gnu
Compiler: gcc
Compilation CFLAGS: -march=x86-64 -mtune=generic -O2 -pipe -fno-plt
-fexceptions -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security
-fstack-clash-protection -fcf-protection -fno-omit-frame-pointer
-mno-omit-leaf-frame-pointer -g -flto=auto
uname output: Linux 7.2.6-arch2-1 x86_64
Machine Type: x86_64-pc-linux-gnu
Bash Version: 5.3
Patch Level: 15
Release Status: release
Description:
With lastpipe enabled and job control off, execute_pipeline()
unblocks SIGCHLD (execute_cmd.c, UNBLOCK_CHILD just before the
lastpipe_flag block) and then calls append_process() to link a
dummy PROCESS for the last element into jobs[jid]->pipe.
append_process() does that with SIGCHLD deliverable and in this
order:
p->next = t; /* t reachable, t->next == NULL */
t->next = jobs[jid]->pipe;
alloc_process() sets t->next to NULL. If SIGCHLD arrives between
the two stores, sigchld_handler -> waitchld ->
set_job_status_and_cleanup walks the ring with child = child->next,
reaches the NULL link, and faults reading child->running
(si_addr 0x10).
The other ring mutators in jobs.c already run with SIGCHLD
blocked (add_process under BLOCK_CHILD; rotate_the_pipeline and
reverse_the_pipeline document "Must be called with SIGCHLD
blocked"). append_process is the exception. It also does
js.c_reaped++ without the signal blocked, and waitchld updates
the same counter.
This is not only theoretical. A shell-based scanner that runs
many `x=$(sha256sum "$f" | awk '{print $1}')` under
`shopt -s lastpipe` crashed intermittently in long parallel runs.
Three cores from the distribution /usr/bin/bash 5.3.15 were all
interrupted at the same instruction: the second store above
(append_process+126 in that binary). The crashing frame was
waitchld reading child->running through the NULL link.
The code is unchanged in the current devel branch (jobs.c
append_process; the UNBLOCK_CHILD before it is in execute_cmd.c).
Repeat-By:
The window is two instructions wide, so a plain loop rarely hits
it. This gdb recipe makes it deterministic. Use a -O0 -g build so
the line breakpoint lands between the stores. The line number is
that of `t->next = jobs[jid]->pipe;` in jobs.c (1660 in 5.3).
printf 'shopt -s lastpipe\nsleep 30 | :\necho survived\n' > lp.sh
gdb -q -batch \
-ex 'set startup-with-shell off' \
-ex 'handle SIGCHLD nostop noprint pass' \
-ex 'break jobs.c:1660' -ex run \
-ex 'print *t' \
-ex 'call (int) kill (jobs[jid]->pipe->pid, 15)' \
-ex 'shell sleep 1' \
-ex continue -ex 'bt 6' \
--args ./bash lp.sh
At the breakpoint, `print *t` shows next = 0x0 with t already
linked from p. Killing the `sleep` child makes a real SIGCHLD
pending, and continuing gives:
Program received signal SIGSEGV, Segmentation fault.
set_job_status_and_cleanup (job=0) at jobs.c:4310
4310 job_state |= PRUNNING (child);
#1 waitchld (wpid=-1, block=0) at jobs.c:4216
#2 sigchld_handler (sig=17) at jobs.c:4044
#3 <signal handler called>
#4 append_process (...) at jobs.c:1660
#5 execute_pipeline (...) at execute_cmd.c:2777
Without a debugger, the crash is easy to see if the window is
widened by inserting a short busy loop between the two stores.
The following script then segfaults within a few hundred
iterations:
shopt -s lastpipe
for ((i=0;i<500;i++)); do
s=$((RANDOM % 8000))
( for ((j=0;j<s;j++)); do :; done ) &
( exit 0 ) | :
done
With SIGCHLD blocked across the same widened window (the store
order left as it is, so the mask is the only difference), it does
not. Across 80 such runs of 500 iterations, the unmasked build
crashed in 67 and the masked build in none.
Fix:
Block SIGCHLD across the whole update, as the other ring
mutators do, and close the ring before publishing the new node.
This also covers js.c_reaped++.
Reordering the two stores alone would not be enough. Nothing
orders them for the compiler, and it would leave the counter
update racing.
Checked with the patch applied to 5.3.15:
- the gdb recipe above completes and prints "survived";
- the signal mask on return from append_process equals the
mask on entry, including when SIGCHLD was already blocked
by the caller;
- `make tests` output matches unpatched except for the number
of CHLD lines from trap8.sub. That test is timing-dependent,
uses set -m, and never reaches append_process; its count
varies on both builds. run-lastpipe is clean;
- the cost is two sigprocmask calls per lastpipe pipeline;
wall time was unchanged within noise.
QUEUE_SIGCHLD/UNQUEUE_SIGCHLD would be an alternative. I used
the mask because it keeps the change local to append_process and
does not defer reaping across the wait_for() that follows it.
--- a/jobs.c
+++ b/jobs.c
@@ -1645,6 +1645,10 @@
append_process (char *name, pid_t pid, int status, int jid)
{
PROCESS *t, *p;
+ sigset_t set, oset;
+
+ /* SIGCHLD traverses this ring; keep it blocked until the update is
complete. */
+ BLOCK_CHILD (set, oset);
t = alloc_process (name, pid);
@@ -1656,8 +1660,10 @@
for (p = jobs[jid]->pipe; p->next != jobs[jid]->pipe; p = p->next)
;
- p->next = t;
t->next = jobs[jid]->pipe;
+ p->next = t;
+
+ UNBLOCK_CHILD (oset);
}
#if 0
