From 50db00e1366677dc4d5138ff2cdd5f54fa0fba3c Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Mon, 13 Jun 2022 19:41:36 +0100 Subject: [PATCH] Fix subshell trap integrity, e.g. re-trapping a signal in subshell Ksh handles local traps in virtual subshells the same way as local traps in ksh-style shell functions, which can cause incorrect operation. Reproducer script: trap 'echo "parent shell trap"; exit 0' USR1 (trap 'echo "subshell trap"' USR1; kill -USR1 $$) echo wrong Output on every ksh93 version: 'wrong' Output on every other shell: 'parent shell trap' The ksh93 output is wrong because $$ is the PID of the main shell, therefore 'kill -USR1 $$' from a subshell needs to issue SIGUSR1 to the main shell and trigger the 'echo SIGUSR1' trap. This is an inevitable consequence of processing signals in a virtual subshell. Signals are a process-wide property, but a virtual subshell and the parent shell share the same process. Therefore it is not possible to distinguish between the parent shell and subshell trap. This means virtual subshells are fundamentally incompatible with receiving signals. No workaround can make this work properly. Ksh could either assume the signal needs to be caught by the subshell trap (wrong in this case, but right in others) or by the parent shell trap. But it does neither and just gives up and does nothing, which I suppose is the least bad way of doing it wrong. As another example, consider a subshell that traps a signal, then passes its own process ID (as of 9de65210, that's ${.sh.pid}) to another process to say "here is where to signal me". A virtual subshell will send it the PID that it shares with the the parent shell. Even if a virtual subshell receives the signal correctly, it may fork mid-execution afterwards, depending on the commands that it runs (and this varies by implementation as we fix or work around bugs). So its PID could be invalidated at any time. Forking a virtual subshell at the time of trapping a signal is the only way to ensure a persistent PID and correct operation. src/cmd/ksh93/bltins/trap.c: b_trap(): - Fork when trapping (or ignoring) a signal in a virtual subshell. (There's no need to fork when trapping a pseudosignal.) src/cmd/ksh93/tests/signal.sh: - Add tests. These are simplified versions of tests already there, which issued 'kill' as a background job. Currently, running a background job causes a virtual subshell to fork before forking the 'kill' background job (or *any* background job, see e3d7bf1d) -- an ugly partial workaround that I believe just became redundant and which I will remove in the next commit. --- NEWS | 9 +++++++++ src/cmd/ksh93/bltins/trap.c | 7 +++++++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/tests/signal.sh | 21 ++++++++++++++++++--- 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 079109f00..13561aa66 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,15 @@ For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0 Any uppercase BUG_* names are modernish shell bug IDs. +2022-06-13: + +- Trapping a signal that is not a pseudosignal will now cause a virtual + subshell to fork into a real one. This ensures a persistent PID where + other processes can signal the subshell (the PID of a virtual subshell may + change as other commands cause it to fork), and fixes a bug where a signal + could not be correctly or effectively trapped in a subshell if the same + signal was also trapped in the main shell environment. + 2022-06-12: - The POSIX mode now disables zero-padding of seconds in 'time'/'times' output. diff --git a/src/cmd/ksh93/bltins/trap.c b/src/cmd/ksh93/bltins/trap.c index c19d1ef65..76e134de8 100644 --- a/src/cmd/ksh93/bltins/trap.c +++ b/src/cmd/ksh93/bltins/trap.c @@ -166,6 +166,13 @@ int b_trap(int argc,char *argv[],Shbltin_t *context) } else { + /* + * Trap or ignore a real signal. A virtual subshell needs to fork in + * order to receive signals correctly and (because other commands + * may cause a virtual subshell to fork) to ensure a persistent PID. + */ + if(sh.subshell && !sh.subshare) + sh_subfork(); if(sig >= sh.st.trapmax) sh.st.trapmax = sig+1; arg = sh.st.trapcom[sig]; diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 1896808d6..cfa451acd 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -23,7 +23,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-beta.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2022-06-12" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2022-06-13" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/tests/signal.sh b/src/cmd/ksh93/tests/signal.sh index 3dc23fd4b..9d4b34ae8 100755 --- a/src/cmd/ksh93/tests/signal.sh +++ b/src/cmd/ksh93/tests/signal.sh @@ -273,22 +273,37 @@ PATH=${PATH#:} if [[ ${SIG[USR1]} ]] then float s=$SECONDS exp=SIGUSR1 + got=$(LC_ALL=C $SHELL -c ' trap "print SIGUSR1 ; exit 0" USR1 (trap "" USR1 ; exec kill -USR1 $$ & sleep .5) print done') [[ $got == "$exp" ]] || err_exit 'subshell ignoring signal does not send signal to parent' \ - "(expected '$exp', got '$got')" + "(expected $(printf %q "$exp"), got $(printf %q "$got"))" (( (SECONDS-s) < .4 )) && err_exit 'parent does not wait for child to complete before handling signal' ((s = SECONDS)) - exp=SIGUSR1 + + : >out + trap 'echo SIGUSR1 >out; exit 0' USR1 + (trap '' USR1; kill -USR1 $$) + got=$(out + trap 'echo SIGUSR1 >out; exit 0' USR1 + (trap 'echo wrong' USR1; kill -USR1 $$) + got=$(