Re: How to misplace an entire pipeline

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Aug 9,  9:19pm, Peter Stephenson wrote:
} Subject: Re: How to misplace an entire pipeline
}
} > } > +	jobtab[thisjob].stat |= STAT_CURSH;
} > } > +	if (!jobtab[thisjob].procs)
} > } > +	    jobtab[thisjob].stat |= STAT_NOPRINT;
} > 
} > Should that be "if (hasprocs(thisjob))" instead, do you think?
} 
} To a first approximation, the user isn't directly interested in the aux
} procs, but there might still be some knock-on effect.  So until we find
} otherwise, I would guess not.

Assuming that the aux procs are not subject to tty process group signals, 
it should be harmless to ignore them.

I believe the patch below would make it possible to back out the above,
because when the pipeline can't be stopped it's impossible to examine
the job table anyway.  However, I think it's not a problem to leave it,
and there may be a few cases where it affects $jobtexts et al. in a
useful way.

} > What needs to be tested in place of (!list_pipe) to determine that
} > the tail of the current pipeline is a simple shell builtin?
} 
} I don't think we've remembered that fact, but it's available (as
} is_builtin) in execcmd(), so could be stored before the builtin is
} called.

It appears sufficient to add this as another STAT_* bitflag in the
job structure.  It might actually make sense to make list_pipe into
a bitflag as well, and mark every job in the table for which it was
true at the time the job began, but I'm not prepared for that dive.

} > In fact even that may not be enough, maybe this needs to know if the
} > current job is a simple shell builtin -- it might be blocked on a
} > "while read; do ..." or on a "wait" in the middle of a loop, etc.
} 
} I suppose it depends on what cases the list_pipe logic would be
} triggered.

I fooled around with a few cases and I think I caught this one too.
At this point list_pipe is true but the job receiving the stop signal
is not in the process list of the current job (thisjob), so it's
necessary to check whether the current job is a builtin.  Unless you
can think of a case where thisjob would not correctly identify the
foreground job at the time the signal is handled?

I'm still a little nervous about coughing up that zwarn() in the
signal handler but I can't find a better place for it.  One drawback
here is that if the tail of the pipeline is a loop that's rapidly
alternating between a builtin and an external command, ^Z might stop
the loop sometimes and issue the warning other times, seemingly at
random.


diff -u ../zsh-forge/current/Src/exec.c ./Src/exec.c
--- ../zsh-forge/current/Src/exec.c	2011-08-09 20:01:14.000000000 -0700
+++ ./Src/exec.c	2011-08-13 11:00:36.000000000 -0700
@@ -2848,6 +2848,8 @@
 	jobtab[thisjob].stat |= STAT_CURSH;
 	if (!jobtab[thisjob].procs)
 	    jobtab[thisjob].stat |= STAT_NOPRINT;
+	if (is_builtin)
+	  jobtab[thisjob].stat |= STAT_BUILTIN;
     } else {
 	/* This is an exec (real or fake) for an external command.    *
 	 * Note that any form of exec means that the subshell is fake *
diff -u ../zsh-forge/current/Src/signals.c ./Src/signals.c
--- ../zsh-forge/current/Src/signals.c	2011-08-06 11:13:23.000000000 -0700
+++ ./Src/signals.c	2011-08-11 09:55:26.000000000 -0700
@@ -490,14 +490,21 @@
 	 * update it.
 	 */
 	if (findproc(pid, &jn, &pn, 0)) {
+	    if (((jn->stat & STAT_BUILTIN) ||
+		 (list_pipe && (jobtab[thisjob].stat & STAT_BUILTIN))) &&
+		WIFSTOPPED(status) && WSTOPSIG(status) == SIGTSTP) {
+		killjb(jn, SIGCONT);
+		zwarn("job can't be suspended");
+	    } else {
 #if defined(HAVE_WAIT3) && defined(HAVE_GETRUSAGE)
-	    struct timezone dummy_tz;
-	    gettimeofday(&pn->endtime, &dummy_tz);
-	    pn->status = status;
-	    pn->ti = ru;
+		struct timezone dummy_tz;
+		gettimeofday(&pn->endtime, &dummy_tz);
+		pn->status = status;
+		pn->ti = ru;
 #else
-	    update_process(pn, status);
+		update_process(pn, status);
 #endif
+	    }
 	    update_job(jn);
 	} else if (findproc(pid, &jn, &pn, 1)) {
 	    pn->status = status;
diff -u ../zsh-forge/current/Src/zsh.h ./Src/zsh.h
--- ../zsh-forge/current/Src/zsh.h	2011-05-13 21:08:04.000000000 -0700
+++ ./Src/zsh.h	2011-08-10 09:10:35.000000000 -0700
@@ -907,6 +907,8 @@
 #define STAT_ATTACH	(0x1000) /* delay reattaching shell to tty       */
 #define STAT_SUBLEADER  (0x2000) /* is super-job, but leader is sub-shell */
 
+#define STAT_BUILTIN    (0x4000) /* job at tail of pipeline is a builtin */
+
 #define SP_RUNNING -1		/* fake status for jobs currently running */
 
 struct timeinfo {