PATCH: Rimmerworld pipeline race

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

OK, this is *much* closer and possibly correct.

When adding a new process to the list in the main shell, we looked to
see if the group leader for that job was empty and if so marked the new
process as the group leader.

This conflicts with entersubsh(), where there are hairy list-pipe tests
to determine what will actually be used as the process group leader
(look in the block controlled by the ESUB_PGRP test --- that's where I
was fiddling in the last patch, but this bit isn't the one that's
wrong).

Most of the time this isn't a problem, but if we decide in the main
shell that that process exited while that group was in control of the terminal,
the main shell will think it's time for it to reclaim the terminal.
Whatever happens to be running in the foreground will then find it's
been pushed into the background.

This showed up in this case

  echo help | { cat | more }

because

- The echo had set the process group in a list_pipe_job fashion.

- The cat was in that process group and the bogus logic made the parent
shell think it had its own process group.

- The more put itself in the same process group, too.

- The cat exited.  The following logic

		if (WIFEXITED(status) &&
		    pn->pid == jn->gleader &&
		    killpg(pn->pid, 0) == -1) {
		    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);
		    }
		}

  then triggered, grabbing back the terminal.  The more found itself
  without a terminal.

The fix is to second-guess the test the process itself makes when
setting the group leader and to add some clear comments.  Then
jn->gleader in the hunk above correctly reflects the actual foreground
process group, so the shell doesn't reclaim the terminal yet.

Neither of the two related bits, in entersubsh() or addproc(), is new,
increasing my suspicion that the problem isn't fundamentally new but a
newly exposed race.

It's hard to think how fixing this can make things worse (although I
always say this).  How about committing it, making a new test build,
encouraging EVERYONE to try it out, and seeing if that is releasable?

pws


diff --git a/Src/exec.c b/Src/exec.c
index 09ee132..8062f0c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -994,6 +994,31 @@ enum {
     ESUB_JOB_CONTROL = 0x40
 };
 
+
+/*
+ * If the list_pipe magic is in effect, its process group controls
+ * the terminal: return this.  Otherwise, return -1.
+ *
+ * C.f. the test in entersubsh() for process group of forked subshell.
+ */
+
+/**/
+int
+get_list_pipe_process_group(void)
+{
+    int gleader;
+    if (!list_pipe && !list_pipe_child)
+	return -1;
+    gleader = jobtab[list_pipe_job].gleader;
+
+    if (gleader &&
+	killpg(gleader, 0) != -1 &&
+	gettygrp() == gleader)
+	return gleader;
+
+    return -1;
+}
+
 /**/
 static void
 entersubsh(int flags)
diff --git a/Src/jobs.c b/Src/jobs.c
index 38b3d89..c841150 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1392,10 +1392,21 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime)
     if (!aux)
     {
 	pn->bgtime = *bgtime;
-	/* if this is the first process we are adding to *
-	 * the job, then it's the group leader.          */
-	if (!jobtab[thisjob].gleader)
-	    jobtab[thisjob].gleader = pid;
+	/*
+	 * If this is the first process we are adding to
+	 * the job, then it's the group leader.
+	 *
+	 * Exception: if the list_pipe_job pipeline madness is in
+	 * effect, we need to use the process group that's already
+	 * controlling the terminal or we'll attach to something
+	 * bogus when that process exits.  This test mimics the
+	 * test the process itself makes in entersubsh(), so the
+	 * two should be kept in sync.
+	 */
+	if (!jobtab[thisjob].gleader) {
+	    int gleader = get_list_pipe_process_group();
+	    jobtab[thisjob].gleader = (gleader == -1) ? pid : gleader;
+	}
 	/* attach this process to end of process list of current job */
 	pnlist = &jobtab[thisjob].procs;
     }