Re: Bug in ZSH 5.0.0 - Segmentation fault after sending process to background

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On 11 October 2012 14:46, BlueC0re <bluecore90@xxxxxxxxxxxxxx> wrote:
> zsh version 5.0.0 crashes with a segfault if you have a custom precmd
> specified (containing an additional command block), and sending a
> process to the background.

Yes, it does.  Thanks for the reproduction information.

That's bad, right?

I think I understand much of this.  Certain very simple commands are
optimised to be run by execsimple(), bypassing job control, so "thisjob"
won't be set up the way you expect.  This causes the observed mayhem if
the code actually does look at job control.

I've added some tests to ensure thisjob is valid where necessary, and
also ensured that it's not valid for code called from execsimple(),
since it's never been set up --- or if it has we have no reason to
suppose it's a job we're interested in.  While I can't guarantee
invalidating thisjob in execsimple is harmless, the case here suggests
that if it causes anything else that's probably something we want to
find out about rather than sweeping under the carpet.

Using my own initialisation files I can see an execshfunc() being called
from execsimple().  This doesn't strike me as being particularly
"simple", although execsimple says the requirement is the code runs
entirely within the shell, which is true (code run from the function
that doesn't will cause its own jobs to be created).

The change in hasprocs() is just there to make debugging a little
easier.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.212
diff -p -u -r1.212 exec.c
--- Src/exec.c	7 Oct 2012 19:46:46 -0000	1.212
+++ Src/exec.c	11 Oct 2012 14:19:34 -0000
@@ -404,7 +404,17 @@ execcursh(Estate state, int do_exec)
     /* Skip word only used for try/always */
     state->pc++;

-    if (!list_pipe && thisjob != list_pipe_job && !hasprocs(thisjob))
+    /*
+     * The test thisjob != -1 was added because sometimes thisjob
+     * can be invalid at this point.  The case in question was
+     * in a precmd function after operations involving background
+     * jobs.
+     *
+     * This is because sometimes we bypass job control to execute
+     * very simple functions via execssimple().
+     */
+    if (!list_pipe && thisjob != -1 && thisjob != list_pipe_job &&
+	!hasprocs(thisjob))
 	deletejob(jobtab + thisjob, 0);
     cmdpush(CS_CURSH);
     execlist(state, 1, do_exec);
@@ -1064,7 +1074,7 @@ static int
 execsimple(Estate state)
 {
     wordcode code = *state->pc++;
-    int lv;
+    int lv, otj;

     if (errflag)
 	return (lastval = 1);
@@ -1075,6 +1085,13 @@ execsimple(Estate state)

     code = wc_code(*state->pc++);

+    /*
+     * Because we're bypassing job control, ensure the called
+     * code doesn't see the current job.
+     */
+    otj = thisjob;
+    thisjob = -1;
+
     if (code == WC_ASSIGN) {
 	cmdoutval = 0;
 	addvars(state, state->pc - 1, 0);
@@ -1086,6 +1103,8 @@ execsimple(Estate state)
     } else
 	lv = (execfuncs[code - WC_CURSH])(state, 0);

+    thisjob = otj;
+
     return lastval = lv;
 }

@@ -4313,7 +4332,9 @@ execshfunc(Shfunc shf, LinkList args)
     if (errflag)
 	return;

-    if (!list_pipe && thisjob != list_pipe_job && !hasprocs(thisjob)) {
+    /* thisjob may be invalid if we're called via execsimple: see execcursh */
+    if (!list_pipe && thisjob != -1 && thisjob != list_pipe_job &&
+	!hasprocs(thisjob)) {
 	/* Without this deletejob the process table *
 	 * would be filled by a recursive function. */
 	last_file_list = jobtab[thisjob].filelist;
Index: Src/jobs.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/jobs.c,v
retrieving revision 1.91
diff -p -u -r1.91 jobs.c
--- Src/jobs.c	21 Sep 2012 12:45:29 -0000	1.91
+++ Src/jobs.c	11 Oct 2012 14:19:34 -0000
@@ -209,7 +209,13 @@ findproc(pid_t pid, Job *jptr, Process *
 int
 hasprocs(int job)
 {
-    Job jn = jobtab + job;
+    Job jn;
+
+    if (job < 0) {
+	DPUTS(1, "job number invalid in hasprocs");
+	return 0;
+    }
+    jn = jobtab + job;

     return jn->procs || jn->auxprocs;
 }

pws