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

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Tue, 10 Apr 2018 12:45:45 +0100
Peter Stephenson <p.stephenson@xxxxxxxxxxx> wrote:

> On Sat, 24 Mar 2018 15:09:44 -0700
> Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> > On Mar 23, 10:05pm, Joey Pabalinas wrote:
> > }
> > } > It could be I just haven't constructed the right test.
> > } 
> > } It's kind of an *incredibly* unlikely edge-case but FWIW:
> > } 
> > } > $ echo | ps -j $(: $(cat)) | cat | cat | cat
> > } 
> > } The shell _seems_ to completely lock up after applying your patch;
> > on } my zsh 5.4.2 release version there are no problems (aside from
> > the } original process group leader issues).
> 
>... [Stuff about the process group aspects I'm not discussing here] ...

I think all this still applies, but I've nothing new to add, so I've
edited it down to keep the messages manageable.

> But looking at ps output while ^C is failing to have any effect
> suggests things are worse than that.  I think at this point the
> parent shell hasn't actually reaped the echo (it's <defunct>), so
> hasn't executed this chunk of code.  That's got something to do with
> the fact that it's got child reaping blocked while trying to read
> output from the the $(: ...) process --- it's executing the same code
> as the forked shell waiting for the cat.  However, even simple-minded
> child unblocking didn't seem to cause the zombie to go away, at which
> point I got stuck.

OK, the following change --- also too simple for the real world, so
treat as proof of concept --- *does* make this case interruptible.  I
was forgetting that child_unblock() wasn't enough, you need to ensure
signals aren't queued.

So I suppose we need a way of making this safe(r)...

pws

diff --git a/Src/exec.c b/Src/exec.c
index e154d12..c22d37d 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4543,11 +4543,15 @@ getoutput(char *cmd, int qt)
 	return NULL;
     } else if (pid) {
 	LinkList retval;
+	int q = queue_signal_level();
 
 	zclose(pipes[1]);
+	dont_queue_signals();
+	child_unblock();
 	retval = readoutput(pipes[0], qt, NULL);
 	fdtable[pipes[0]] = FDT_UNUSED;
 	waitforpid(pid, 0);		/* unblocks */
+	restore_queue_signals(q);
 	lastval = cmdoutval;
 	return retval;
     }
diff --git a/Src/signals.c b/Src/signals.c
index 94f379e..f86ae54 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -537,6 +537,11 @@ wait_for_processes(void)
 #else
 		update_process(pn, status);
 #endif
+		if (pn->pid == jn->gleader) {
+		    jn->gleader = 0;
+		    /* HERE: check this was actually in foreground... */
+		    attachtty(mypgrp);
+		}
 	    }
 	    update_job(jn);
 	} else if (findproc(pid, &jn, &pn, 1)) {