[PATCH] jobs: Block signals during tcsetpgrp

[Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>]

Harald van Dijk <harald@xxxxxxxxxxx> wrote:
> On 19/12/2020 22:21, Steffen Nurpmeso wrote:
>> Steffen Nurpmeso wrote in
>>   <20201219172838.1B-WB%steffen@xxxxxxxxxx>:
>>   |Long story short, after falsely accusing BSD make of not working
>> 
>> After dinner i shortened it a bit more, and attach it again, ok?
>> It is terrible, but now less redundant than before.
>> Sorry for being so terse, that problem crosses my head for about
>> a week, and i was totally mislead and if you bang your head
>> against the wall so many hours bugs or misbehaviours in a handful
>> of other programs is not the expected outcome.
> 
> I think a minimal test case is simply
> 
> all:
>         $(SHELL) -c 'trap "echo TTOU" TTOU; set -m; echo all good'
> 
> unless I accidentally oversimplified.
> 
> The SIGTTOU is caused by setjobctl's xtcsetpgrp(fd, pgrp) call to make 
> its newly started process group the foreground process group when job 
> control is enabled, where xtcsetpgrp is a wrapper for tcsetpgrp. (That's 
> in dash, the other variants may have some small differences.) tcsetpgrp 
> has this little bit in its specification:
> 
>        Attempts to use tcsetpgrp() from a process which is a member of
>        a background process group on a fildes associated with its con‐
>        trolling  terminal  shall  cause the process group to be sent a
>        SIGTTOU signal. If the calling thread is blocking SIGTTOU  sig‐
>        nals  or  the  process is ignoring SIGTTOU signals, the process
>        shall be allowed to perform the operation,  and  no  signal  is
>        sent.
> 
> Ordinarily, when job control is enabled, SIGTTOU is ignored. However, 
> when a trap action is specified for SIGTTOU, the signal is not ignored, 
> and there is no blocking in place either, so the tcsetpgrp() call is not 
> allowed.
> 
> The lowest impact change to make here, the one that otherwise preserves 
> the existing shell behaviour, is to block signals before calling 
> tcsetpgrp and unblocking them afterwards. This ensures SIGTTOU does not 
> get raised here, but also ensures that if SIGTTOU is sent to the shell 
> for another reason, there is no window where it gets silently ignored.
> 
> Another way to fix this is by not trying to make the shell start a new 
> process group, or at least not make it the foreground process group. 
> Most other shells appear to not try to do this.

This patch implements the blocking of SIGTTOU (and everything else)
while we call tcsetpgrp.

Reported-by: Steffen Nurpmeso <steffen@xxxxxxxxxx>
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>

diff --git a/src/jobs.c b/src/jobs.c
index 516786f..809f37c 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -1512,7 +1512,13 @@ showpipe(struct job *jp, struct output *out)
 STATIC void
 xtcsetpgrp(int fd, pid_t pgrp)
 {
-	if (tcsetpgrp(fd, pgrp))
+	int err;
+
+	sigblockall(NULL);
+	err = tcsetpgrp(fd, pgrp);
+	sigclearmask();
+
+	if (err)
 		sh_error("Cannot set tty process group (%s)", strerror(errno));
 }
 #endif
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt