diff mbox series

[v4] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells

Message ID 20240518084105.111682-1-aclopte@gmail.com (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show
Series [v4] Allow trap to un-ignore SIGINT/SIGQUIT in async subshells | expand

Commit Message

Johannes Altmanninger May 18, 2024, 8:38 a.m. UTC
Unlike in Bash or Zsh, this asynchronous job ignores SIGINT, despite
builtin trap explicitly resetting the SIGINT handler.

	dash -c '( trap - INT; sleep inf ) & read _'

POSIX Section 2.11 on [Signals] and Error Handling says about
background execution:

> If job control is disabled (see the description of set -m) when
> the shell executes an asynchronous list, the commands in the list
> shall inherit from the shell a signal action of ignored (SIG_IGN)
> for the SIGINT and SIGQUIT signals.

Builtin [trap] has this requirement:

> Signals that were ignored on entry to a non-interactive shell
> cannot be trapped or reset, although no error need be reported when
> attempting to do so.

Apparently this only applies to signals that were inherited as ignored,
not to the special case of SIGINT/SIGQUIT begin ignored in asynchronous
subshells.

Make it so. This means that either of

	trap - INT; trap - QUIT
	set -i

in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.

[Signals]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
[trap]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap

{{{ Test cases:

shell=src/dash

set -e

SubshellWith() {
	parent_pid=$(setsid "$shell" -c "( $1; sleep 99 ) </dev/null >/dev/null 2>&1 & echo \$\$")
	sleep 1
	subshell_pid=$(ps -o pid= -$parent_pid | tail -n 1)
}

trap 'kill -TERM -$parent_pid 2>/dev//null ||:' EXIT # Tear down after a failure.

echo Scenario 0: '"set -i"' makes a subshell un-ignore SIGINT.
SubshellWith 'set -i'
kill -INT $subshell_pid
! ps -p $subshell_pid | grep sleep || exit 1
kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.

echo Scenario 1: resetting SIGINT handler.
SubshellWith 'trap - INT'
kill -INT -$parent_pid # kill the whole process group since that's the my use case
! ps -p $subshell_pid | grep sleep || exit 1
kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.

echo Scenario 2: ignoring SIGINT.
SubshellWith 'trap "" INT'
kill -INT $subshell_pid
ps -p $subshell_pid | grep sleep || exit 1
kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.

}}}

{{{ Backstory/motivation:

The Kakoune[1] editor likes to run noninteractive shell commands that
boil down to

	mkfifo /tmp/fifo
	(
		trap - INT
		make
	) >/tmp/fifo 2>&1 &

On Control-C, the editor sends SIGINT to its process group,
which should terminate the subshell running make[2].

We experimented with sending SIGTERM instead but found issues,
specifically if the editor is invoked (without exec) from a wrapper
script, sending SIGTERM to the whole process group would kill the
wrapper script, which in turn makes it send SIGTERM to the editor,
which then terminates.

[1]: https://kakoune.org/
[2]: https://lists.sr.ht/~mawww/kakoune/%3C20240307135831.1967826-3-aclopte@gmail.com%3E

}}}
---
 src/trap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Herbert Xu May 26, 2024, 10:27 a.m. UTC | #1
On Sat, May 18, 2024 at 10:38:30AM +0200, Johannes Altmanninger wrote:
> Unlike in Bash or Zsh, this asynchronous job ignores SIGINT, despite
> builtin trap explicitly resetting the SIGINT handler.
> 
> 	dash -c '( trap - INT; sleep inf ) & read _'
> 
> POSIX Section 2.11 on [Signals] and Error Handling says about
> background execution:
> 
> > If job control is disabled (see the description of set -m) when
> > the shell executes an asynchronous list, the commands in the list
> > shall inherit from the shell a signal action of ignored (SIG_IGN)
> > for the SIGINT and SIGQUIT signals.
> 
> Builtin [trap] has this requirement:
> 
> > Signals that were ignored on entry to a non-interactive shell
> > cannot be trapped or reset, although no error need be reported when
> > attempting to do so.
> 
> Apparently this only applies to signals that were inherited as ignored,
> not to the special case of SIGINT/SIGQUIT begin ignored in asynchronous
> subshells.
> 
> Make it so. This means that either of
> 
> 	trap - INT; trap - QUIT
> 	set -i
> 
> in a backgrounded subshell will now un-ignore SIGINT and SIGQUIT.
> 
> [Signals]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
> [trap]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap
> 
> {{{ Test cases:
> 
> shell=src/dash
> 
> set -e
> 
> SubshellWith() {
> 	parent_pid=$(setsid "$shell" -c "( $1; sleep 99 ) </dev/null >/dev/null 2>&1 & echo \$\$")
> 	sleep 1
> 	subshell_pid=$(ps -o pid= -$parent_pid | tail -n 1)
> }
> 
> trap 'kill -TERM -$parent_pid 2>/dev//null ||:' EXIT # Tear down after a failure.
> 
> echo Scenario 0: '"set -i"' makes a subshell un-ignore SIGINT.
> SubshellWith 'set -i'
> kill -INT $subshell_pid
> ! ps -p $subshell_pid | grep sleep || exit 1
> kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.
> 
> echo Scenario 1: resetting SIGINT handler.
> SubshellWith 'trap - INT'
> kill -INT -$parent_pid # kill the whole process group since that's the my use case
> ! ps -p $subshell_pid | grep sleep || exit 1
> kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.
> 
> echo Scenario 2: ignoring SIGINT.
> SubshellWith 'trap "" INT'
> kill -INT $subshell_pid
> ps -p $subshell_pid | grep sleep || exit 1
> kill -TERM -$parent_pid 2>/dev//null ||: # Tear down.
> 
> }}}
> 
> {{{ Backstory/motivation:
> 
> The Kakoune[1] editor likes to run noninteractive shell commands that
> boil down to
> 
> 	mkfifo /tmp/fifo
> 	(
> 		trap - INT
> 		make
> 	) >/tmp/fifo 2>&1 &
> 
> On Control-C, the editor sends SIGINT to its process group,
> which should terminate the subshell running make[2].
> 
> We experimented with sending SIGTERM instead but found issues,
> specifically if the editor is invoked (without exec) from a wrapper
> script, sending SIGTERM to the whole process group would kill the
> wrapper script, which in turn makes it send SIGTERM to the editor,
> which then terminates.
> 
> [1]: https://kakoune.org/
> [2]: https://lists.sr.ht/~mawww/kakoune/%3C20240307135831.1967826-3-aclopte@gmail.com%3E
> 
> }}}
> ---
>  src/trap.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Patch applied.  Thanks.
diff mbox series

Patch

diff --git a/src/trap.c b/src/trap.c
index f871656..90e9607 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -296,11 +296,11 @@  setsignal(int signo)
 void
 ignoresig(int signo)
 {
-	if (sigmode[signo - 1] != S_IGN && sigmode[signo - 1] != S_HARD_IGN) {
-		signal(signo, SIG_IGN);
-	}
+	if (sigmode[signo - 1] == S_IGN || sigmode[signo - 1] == S_HARD_IGN)
+        	return;
+	signal(signo, SIG_IGN);
 	if (!vforked)
-		sigmode[signo - 1] = S_HARD_IGN;
+		sigmode[signo - 1] = S_IGN;
 }