make sure to kill background process if set -m fails - #853
Conversation
talaria0101
left a comment
There was a problem hiding this comment.
Diagnosis confirmed, and no regressions where set -m works, but the fallback path has a bug that partially defeats the fix.
Reproduction
Built dash from the parent of 74085cc2 and from the commit itself, then ran a copy of the _lib4bin_collect_strace pattern (set -m, background job, the old kill block) under setsid with stdin from /dev/null (no tty, like the runner):
pre-fix dash (ubuntu-24.04 class):
set: can't access tty; job control turned off
shell pid=4005 pgid=4005 job pid=4006 pgid=4005 cmd=sleep
RESULT: job still alive after old kill code, wait would block forever
The job stays in the shell's process group, so kill -TERM -$pid targets a pgrp that does not exist, || : swallows the error, and nothing before wait $pid can kill the job. Exactly as described.
dash at the fix commit, same script: job pid=3994 pgid=3994, old code kills it.
Regressions on systems where set -m works: none
There kill -0 -$pid succeeds and the branch taken is exactly what main already does (kill -TERM -$pid; sleep 1; kill -KILL -$pid). The only addition is one signal 0 probe, which delivers no signal. Verified branch selection with an instrumented copy of _kill_traced on both shells: post-fix dash takes the group path, pre-fix dash takes the fallback. pid=$1 inside _kill_traced does rewrite the caller's global, but with the value it was called with, so nothing observable changes on that path.
Bug: the recursion clobbers pid, so the root of the tree is never signaled
dash has no local. pid=$2 in each recursive call overwrites the caller's frame, so after the for loop returns, $pid no longer holds this frame's node, it holds the last visited descendant. Every node that has children therefore signals its last child twice and never itself.
Measured with the PR functions verbatim, pre-fix dash, no tty, background job with a root, a leaf child, and an internal node holding two children of its own:
survivors after _kill_traced:
ROOT pid=4023: STILL ALIVE <- $pid already clobbered, label shows a grandchild
sleep 583: dead <- leaf
sleep 584: dead <- leaf
sleep 585: STILL ALIVE <- internal node, never signaled
sleep 597: STILL ALIVE <- root, never signaled
It also corrupts the caller. After _kill_traced $pid returns, the script's $pid is the last grandchild, so the wait $pid at line 1574 waits on a process that is not a child of the shell, errors under || :, and the run continues while the app root is still alive and appending to $dlopened. Simulated that flow with a root writing to a log every 0.2s: 10 lines right after _kill_traced returned, 15 lines one second later.
So the fallback only works for jobs without children (_simple_test_appimage, most strace targets). For forking roots it does not fix the hang: _test_appimage backgrounds xvfb-run, which is itself a node with children. And in strace mode it converts the hang into silent breakage, an incomplete dlopen list parsed from a log that is still being written. Note build-demos does not run on PRs, so this would first show up after merge.
Fix
Use only positional parameters inside the recursion (they are per invocation in POSIX sh) and do not assign pid in _kill_traced at all:
_kill_tree() {
for c in $(cat /proc/$2/task/$2/children 2>/dev/null); do
_kill_tree "$1" "$c"
done
kill -s "$1" "$2" 2>/dev/null || :
}
_kill_traced() {
if kill -0 -$1 2>/dev/null; then
kill -TERM -$1 2>/dev/null || :
sleep 1
kill -KILL -$1 2>/dev/null || :
else
_kill_tree TERM "$1"
sleep 1
_kill_tree KILL "$1"
fi
}The loop variable c can stay global: each iteration reassigns it from the word list expanded before the loop, and the other for c in at line 2004 runs in a later phase anyway.
Same tree test with this version, pre-fix dash, no tty: root, internal node and all leaves dead, caller $pid intact, log frozen.
kill -s TERM was also verified against dash's builtin kill, the leaves above died through exactly that call. The remaining TOCTOU (a child forking between the walk and the kill can escape) is the same class of race the group path has, fine for CI.
talaria0101
left a comment
There was a problem hiding this comment.
Re-ran the full matrix against c1e050a with the functions copied verbatim from the file, both dashes still built from 74085cc2 and its parent, no tty:
- forking tree (root + internal node + 2 leaves), pre-fix dash, fallback path: all 5 processes dead. Before this commit the root and the internal node survived.
- strace flow (root writing to its log every 0.2s), pre-fix dash: no survivors, log frozen, caller
$pidunchanged after_kill_traced, andwait $pidreaps with rc=143 (died by TERM) instead of erroring on a non-child. - post-fix dash:
kill -0 -$pidprobe selects the group path and everything dies, identical to main's behavior there.
That closes everything from my review. From my side this is good to merge.
Apparently ubuntu 24 version of dash doesn't enable
set -mwhen ran without atty.This doesn't happen on archlinux since newer versions of dash don't have this problem.
So this hack is the best we can do that won't introduce a new dependency lol