Re: "echo | ps -j $(:) | cat | cat | cat" runs components in different process groups

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Wed, 11 Apr 2018 15:10:24 -0700
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> On Apr 10,  2:59pm, Peter Stephenson wrote:
> } Subject: Re: "echo | ps -j $(:) | cat | cat | cat" runs components
> in diff }
> } +		if (pn->pid == jn->gleader) {
> } +		    jn->gleader = 0;
> } +		    /* HERE: check this was actually in
> foreground... */ } +		    attachtty(mypgrp);
> } +		}
> 
> I think attachtty() will simply fail here if mypgrp is not already in
> the foreground, but I confess not to be 100% certain.

The man page suggests it's SIGTTOU time, though this isn't my natural
territory either.

I think the right test is for STAT_NOSTTY, which is the flag associated
with running in the background.  (Paranoid checK: we set this with
"bg" as well as "&".)

Moving the unqueuing inside readoutput() both catches other similar
cases and allows us to continue protecting memory allocation while
allowing signals around the blocking read.

So I think this is a definite improvement, though it's probably not safe
enough to sneak into 5.5.1.

pws

diff --git a/Src/exec.c b/Src/exec.c
index e154d12..65ce348 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4576,10 +4576,20 @@ readoutput(int in, int qt, int *readerror)
     char *buf, *ptr;
     int bsiz, c, cnt = 0;
     FILE *fin;
+    int q = queue_signal_level();
 
     fin = fdopen(in, "r");
     ret = newlinklist();
     ptr = buf = (char *) hcalloc(bsiz = 64);
+    /*
+     * We need to be sensitive to SIGCHLD else we can be
+     * stuck forever with important processes unreaped.
+     * The case that triggered this was where the exiting
+     * process is group leader of the foreground process and we need
+     * to reclaim the terminal else ^C doesn't work.
+     */
+    dont_queue_signals();
+    child_unblock();
     while ((c = fgetc(fin)) != EOF || errno == EINTR) {
 	if (c == EOF) {
 	    errno = 0;
@@ -4592,13 +4602,18 @@ readoutput(int in, int qt, int *readerror)
 	    cnt++;
 	}
 	if (++cnt >= bsiz) {
-	    char *pp = (char *) hcalloc(bsiz *= 2);
+	    char *pp;
+	    queue_signals();
+	    pp = (char *) hcalloc(bsiz *= 2);
+	    dont_queue_signals();
 
 	    memcpy(pp, buf, cnt - 1);
 	    ptr = (buf = pp) + cnt - 1;
 	}
 	*ptr++ = c;
     }
+    child_block();
+    restore_queue_signals(q);
     if (readerror)
 	*readerror = ferror(fin) ? errno : 0;
     fclose(fin);
diff --git a/Src/signals.c b/Src/signals.c
index 94f379e..2a6aa3f 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -537,6 +537,19 @@ wait_for_processes(void)
 #else
 		update_process(pn, status);
 #endif
+		if (pn->pid == jn->gleader) {
+		    jn->gleader = 0;
+		    if (!(jn->stat & STAT_NOSTTY)) {
+			/*
+			 * This PID was in control of the terminal;
+			 * reclaim terminal now it has exited.
+			 * It's still possible some future forked
+			 * process of this job will become group
+			 * leader, however.
+			 */
+			attachtty(mypgrp);
+		    }
+		}
 	    }
 	    update_job(jn);
 	} else if (findproc(pid, &jn, &pn, 1)) {