diff mbox series

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

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

Commit Message

Johannes Altmanninger March 29, 2024, 3:39 p.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 ) &'

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Herbert Xu April 7, 2024, 11:18 a.m. UTC | #1 
On Fri, Mar 29, 2024 at 04:39:01PM +0100, Johannes Altmanninger wrote:
>
> diff --git a/src/trap.c b/src/trap.c
> index cd84814..dbf81ea 100644
> --- a/src/trap.c
> +++ b/src/trap.c
> @@ -272,7 +272,7 @@ ignoresig(int signo)
>  		signal(signo, SIG_IGN);
>  	}
>  	if (!vforked)
> -		sigmode[signo - 1] = S_HARD_IGN;
> +		sigmode[signo - 1] = S_IGN;

This is buggy if sigmode is already S_HARD_IGN.  You can fix
this by moving the if statement inside the previous one.

Please also add a comment stating that sigmode has already been
initialised by setinteractive, as otherwise we may also lose a
hard ignore.

Thanks,
Johannes Altmanninger May 18, 2024, 8:43 a.m. UTC | #2 
On Sun, Apr 07, 2024 at 07:18:52PM +0800, Herbert Xu wrote:
> On Fri, Mar 29, 2024 at 04:39:01PM +0100, Johannes Altmanninger wrote:
> >
> > diff --git a/src/trap.c b/src/trap.c
> > index cd84814..dbf81ea 100644
> > --- a/src/trap.c
> > +++ b/src/trap.c
> > @@ -272,7 +272,7 @@ ignoresig(int signo)
> >  		signal(signo, SIG_IGN);
> >  	}
> >  	if (!vforked)
> > -		sigmode[signo - 1] = S_HARD_IGN;
> > +		sigmode[signo - 1] = S_IGN;
> 
> This is buggy if sigmode is already S_HARD_IGN.  You can fix
> this by moving the if statement inside the previous one.
> 
> Please also add a comment stating that sigmode has already been
> initialised by setinteractive, as otherwise we may also lose a
> hard ignore.

I'm not really following the last part;
maybe it's no longer relevant with the bug fix.
Note that it works the same whether "set -i" or "trap - INT" is used,
also in noninteractive shells like

	dash -c '( trap - INT; sleep inf ) & read _'
Herbert Xu May 18, 2024, 9:02 a.m. UTC | #3 
On Sat, May 18, 2024 at 10:43:12AM +0200, Johannes Altmanninger wrote:
>
> I'm not really following the last part;
> maybe it's no longer relevant with the bug fix.
> Note that it works the same whether "set -i" or "trap - INT" is used,
> also in noninteractive shells like
> 
> 	dash -c '( trap - INT; sleep inf ) & read _'

All I meant was that if you call ignoresig before sigmode itself
is initialised it will do the wrong thing if the signal was supposed
to be a hard ignore.

However, this can't happen because we only call ignoresig for
SIGINT and SIGQUIT, and for those two signals, ignoresig is
always called after setinteractive which will initialise the
sigmode array to S_HARD_IGN if necessary.

I'll add a comment when applying your patch.

Thanks,
diff mbox series

Patch

diff --git a/src/trap.c b/src/trap.c
index cd84814..dbf81ea 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -272,7 +272,7 @@  ignoresig(int signo)
 		signal(signo, SIG_IGN);
 	}
 	if (!vforked)
-		sigmode[signo - 1] = S_HARD_IGN;
+		sigmode[signo - 1] = S_IGN;
 }