39359: Fix remaining race with orphaned subjob.

When shell is forked to run right hand side of pipieline it should
use its own PID as process group if the left hand side of the
pipeline has already exited.

4 files changed, 62 insertions, 5 deletions

diff --git a/ChangeLog b/ChangeLog
index 29796ed01..add2ad6a5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2016-09-16  Peter Stephenson  <p.stephenson@samsung.com>
 
+	* 39359: Src/exec.c, Src/jobs.c, Src/signals.c: Further fix on
+	top of 39331 for remaining race.  Ensure process group of forked
+	superjob is sane.
+
 	* 39331: Src/exec.c, Src/jobs.c, Src/zsh.h: Partially fix problem
 	occurring when a subjop in the RHS of a pipeline needs to be
 	picked up by a forked zsh after ^Z when the original superjob
diff --git a/Src/exec.c b/Src/exec.c
index 21270c82d..0755d55cd 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1652,7 +1652,13 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 		    int synch[2];
 		    struct timeval bgtime;
 
+		    /*
+		     * A pipeline with the shell handling the right
+		     * hand side was stopped.  We'll fork to allow
+		     * it to continue.
+		     */
 		    if (pipe(synch) < 0 || (pid = zfork(&bgtime)) == -1) {
+			/* Failure */
 			if (pid < 0) {
 			    close(synch[0]);
 			    close(synch[1]);
@@ -1666,6 +1672,18 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 			thisjob = newjob;
 		    }
 		    else if (pid) {
+			/*
+			 * Parent: job control is here.  If the job
+			 * started for the RHS of the pipeline is still
+			 * around, then its a SUBJOB and the job for
+			 * earlier parts of the pipeeline is its SUPERJOB.
+			 * The newly forked shell isn't recorded as a
+			 * separate job here, just as list_pipe_pid.
+			 * If the superjob exits (it may already have
+			 * done so, see child branch below), we'll use
+			 * list_pipe_pid to form the basis of a
+			 * replacement job --- see SUBLEADER code above.
+			 */
 			char dummy;
 
 			lpforked =
@@ -1692,9 +1710,33 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 			break;
 		    }
 		    else {
+			Job pjn = jobtab + list_pipe_job;
 			close(synch[0]);
 			entersubsh(ESUB_ASYNC);
-			if (jobtab[list_pipe_job].procs) {
+			/*
+			 * At this point, we used to attach this process
+			 * to the process group of list_pipe_job (the
+			 * new superjob) any time that was still available.
+			 * That caused problems when the fork happened
+			 * early enough that the subjob is in that
+			 * process group, since then we will stop this
+			 * job when we signal the subjob, and we have
+			 * no way here to know that we shouldn't also
+			 * send STOP to itself, resulting in two stops.
+			 * So don't do that if the original
+			 * list_pipe_job has exited.
+			 *
+			 * The choice here needs to match the assumption
+			 * made when handling SUBLEADER above that the
+			 * process group is our own PID.  I'm not sure
+			 * if there's a potential race for that.  But
+			 * setting up a new process group if the
+			 * superjob is still functioning seems the wrong
+			 * thing to do.
+			 */
+			if (pjn->procs &&
+			    (pjn->stat & STAT_INUSE) &&
+			    !(pjn->stat & STAT_DONE)) {
 			    if (setpgrp(0L, mypgrp = jobtab[list_pipe_job].gleader)
 				== -1) {
 				setpgrp(0L, mypgrp = getpid());
diff --git a/Src/jobs.c b/Src/jobs.c
index 9284c7124..d1b98ac4d 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -232,11 +232,12 @@ super_job(int sub)
 static int
 handle_sub(int job, int fg)
 {
+    /* job: superjob; sj: subjob. */
     Job jn = jobtab + job, sj = jobtab + jn->other;
 
     if ((sj->stat & STAT_DONE) || (!sj->procs && !sj->auxprocs)) {
 	struct process *p;
-		    
+
 	for (p = sj->procs; p; p = p->next) {
 	    if (WIFSIGNALED(p->status)) {
 		if (jn->gleader != mypgrp && jn->procs->next)
diff --git a/Src/signals.c b/Src/signals.c
index 2eefc07de..30dde713f 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -723,7 +723,7 @@ killjb(Job jn, int sig)
 {
     Process pn;
     int err = 0;
- 
+
     if (jobbing) {
         if (jn->stat & STAT_SUPERJOB) {
             if (sig == SIGCONT) {
@@ -731,11 +731,21 @@ killjb(Job jn, int sig)
                     if (killpg(pn->pid, sig) == -1)
 			if (kill(pn->pid, sig) == -1 && errno != ESRCH)
 			    err = -1;
- 
+
+		/*
+		 * Note this does not kill the last process,
+		 * which is assumed to be the one controlling the
+		 * subjob, i.e. the forked zsh that was originally
+		 * list_pipe_pid...
+		 */
                 for (pn = jn->procs; pn->next; pn = pn->next)
                     if (kill(pn->pid, sig) == -1 && errno != ESRCH)
 			err = -1;
 
+		/*
+		 * ...we only continue that once the external processes
+		 * currently associated with the subjob are finished.
+		 */
 		if (!jobtab[jn->other].procs && pn)
 		    if (kill(pn->pid, sig) == -1 && errno != ESRCH)
 			err = -1;
@@ -744,7 +754,7 @@ killjb(Job jn, int sig)
             }
             if (killpg(jobtab[jn->other].gleader, sig) == -1 && errno != ESRCH)
 		err = -1;
-		
+
 	    if (killpg(jn->gleader, sig) == -1 && errno != ESRCH)
 		err = -1;