36022 fix bug that some loop constructs could not be interrupted, revise signal queueing

There are two underlying ideas here:  (1) Keeping signals queued around
anything that's doing memory management (including push/pop of the heap)
has become crucial.  (2) Anytime the shell is going to run a command, be
it buitin or external, it must be both safe and necessary to process any
queued signals, so that the apparent order of signal arrival and command
execution is preserved.

7 files changed, 119 insertions, 14 deletions

diff --git a/ChangeLog b/ChangeLog
index 4bb73515f..eada38b91 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -14,6 +14,16 @@
 	understanding what the command context code is attempting to do
 	after a command separator.
 
+2015-08-09  Barton E. Schaefer  <schaefer@zsh.org>
+
+	* 36022: Src/loop.c: fix bug that some loop constructs could
+	not be interrupted if all they did was variable assignments or
+	math expressions
+
+	* 36022: Src/exec.c, Src/init.c, Src/input.c, Src/parse.c,
+	Src/signals.c: revise signal queueing to better control the
+	circumtances in which trap handlers are executed
+
 2015-08-08  Daniel Shahaf  <d.s@daniel.shahaf.name>
 
 	* 36008: Src/builtin.c: trap: Fix listing of traps created
diff --git a/Src/exec.c b/Src/exec.c
index 7612d4303..a635c18ed 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1121,10 +1121,14 @@ execsimple(Estate state)
 	    fflush(xtrerr);
 	}
 	lv = (errflag ? errflag : cmdoutval);
-    } else if (code == WC_FUNCDEF) {
-	lv = execfuncdef(state, NULL);
     } else {
-	lv = (execfuncs[code - WC_CURSH])(state, 0);
+	int q = queue_signal_level();
+	dont_queue_signals();
+	if (code == WC_FUNCDEF)
+	    lv = execfuncdef(state, NULL);
+	else
+	    lv = (execfuncs[code - WC_CURSH])(state, 0);
+	restore_queue_signals(q);
     }
 
     thisjob = otj;
@@ -1158,6 +1162,8 @@ execlist(Estate state, int dont_change_job, int exiting)
      */
     int oldnoerrexit = noerrexit;
 
+    queue_signals();
+
     cj = thisjob;
     old_pline_level = pline_level;
     old_list_pipe = list_pipe;
@@ -1428,6 +1434,8 @@ sublist_done:
 	/* Make sure this doesn't get executed again. */
 	sigtrapped[SIGEXIT] = 0;
     }
+
+    unqueue_signals();
 }
 
 /* Execute a pipeline.                                                *
@@ -1456,6 +1464,14 @@ execpline(Estate state, wordcode slcode, int how, int last1)
     else if (slflags & WC_SUBLIST_NOT)
 	last1 = 0;
 
+    /* If trap handlers are allowed to run here, they may start another
+     * external job in the middle of us starting this one, which can
+     * result in jobs being reaped before their job table entries have
+     * been initialized, which in turn leads to waiting forever for
+     * jobs that no longer exist.  So don't do that.
+     */
+    queue_signals();
+
     pj = thisjob;
     ipipe[0] = ipipe[1] = opipe[0] = opipe[1] = 0;
     child_block();
@@ -1468,6 +1484,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
      */
     if ((thisjob = newjob = initjob()) == -1) {
 	child_unblock();
+	unqueue_signals();
 	return 1;
     }
     if (how & Z_TIMED)
@@ -1523,6 +1540,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 	else
 	    spawnjob();
 	child_unblock();
+	unqueue_signals();
 	/* Executing background code resets shell status */
 	return lastval = 0;
     } else {
@@ -1580,15 +1598,18 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 		}
 		if (!(jn->stat & STAT_LOCKED)) {
 		    updated = hasprocs(thisjob);
-		    waitjobs();
+		    waitjobs();		/* deals with signal queue */
 		    child_block();
 		} else
 		    updated = 0;
 		if (!updated &&
 		    list_pipe_job && hasprocs(list_pipe_job) &&
 		    !(jobtab[list_pipe_job].stat & STAT_STOPPED)) {
+		    int q = queue_signal_level();
 		    child_unblock();
+		    dont_queue_signals();
 		    child_block();
+		    restore_queue_signals(q);
 		}
 		if (list_pipe_child &&
 		    jn->stat & STAT_DONE &&
@@ -1672,6 +1693,7 @@ execpline(Estate state, wordcode slcode, int how, int last1)
 		    break;
 	    }
 	    child_unblock();
+	    unqueue_signals();
 
 	    if (list_pipe && (lastval & 0200) && pj >= 0 &&
 		(!(jn->stat & STAT_INUSE) || (jn->stat & STAT_DONE))) {
@@ -3391,6 +3413,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    fflush(xtrerr);
 	}
     } else if (isset(EXECOPT) && !errflag) {
+	int q = queue_signal_level();
 	/*
 	 * We delay the entersubsh() to here when we are exec'ing
 	 * the current shell (including a fake exec to run a builtin then
@@ -3431,11 +3454,14 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    } else
 		redir_prog = NULL;
 
+	    dont_queue_signals();
 	    lastval = execfuncdef(state, redir_prog);
+	    restore_queue_signals(q);
 	}
 	else if (type >= WC_CURSH) {
 	    if (last1 == 1)
 		do_exec = 1;
+	    dont_queue_signals();
 	    if (type == WC_AUTOFN) {
 		/*
 		 * We pre-loaded this to get any redirs.
@@ -3444,6 +3470,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		lastval =  execautofn_basic(state, do_exec);
 	    } else
 		lastval = (execfuncs[type - WC_CURSH])(state, do_exec);
+	    restore_queue_signals(q);
 	} else if (is_builtin || is_shfunc) {
 	    LinkList restorelist = 0, removelist = 0;
 	    /* builtin or shell function */
@@ -3610,7 +3637,9 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		    }
 		    state->pc = opc;
 		}
+		dont_queue_signals();
 		lastval = execbuiltin(args, assigns, (Builtin) hn);
+		restore_queue_signals(q);
 		fflush(stdout);
 		if (save[1] == -2) {
 		    if (ferror(stdout)) {
@@ -4820,11 +4849,9 @@ execshfunc(Shfunc shf, LinkList args)
     if ((osfc = sfcontext) == SFC_NONE)
 	sfcontext = SFC_DIRECT;
     xtrerr = stderr;
-    unqueue_signals();
 
     doshfunc(shf, args, 0);
 
-    queue_signals();
     sfcontext = osfc;
     free(cmdstack);
     cmdstack = ocs;
@@ -5039,6 +5066,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
     static int funcdepth;
 #endif
 
+    queue_signals();	/* Lots of memory and global state changes coming */
+
     pushheap();
 
     oargv0 = NULL;
@@ -5261,6 +5290,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
     }
     popheap();
 
+    unqueue_signals();
+
     /*
      * Exit with a tidy up.
      * Only leave if we're at the end of the appropriate function ---
@@ -5299,6 +5330,8 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
     int cont, ouu;
     char *ou;
 
+    queue_signals();
+
     ou = zalloc(ouu = underscoreused);
     if (ou)
 	memcpy(ou, zunderscore, underscoreused);
@@ -5320,12 +5353,14 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
 	wrap = wrap->next;
     }
     startparamscope();
-    execode(prog, 1, 0, "shfunc");
+    execode(prog, 1, 0, "shfunc");	/* handles signal unqueueing */
     if (ou) {
 	setunderscore(ou);
 	zfree(ou, ouu);
     }
     endparamscope();
+
+    unqueue_signals();
 }
 
 /* Search fpath for an undefined function.  Finds the file, and returns the *
diff --git a/Src/init.c b/Src/init.c
index 2ef90992d..f2021f073 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -105,6 +105,7 @@ loop(int toplevel, int justonce)
     Eprog prog;
     int err, non_empty = 0;
 
+    queue_signals();
     pushheap();
     if (!toplevel)
 	zcontext_save();
@@ -126,7 +127,9 @@ loop(int toplevel, int justonce)
 		 * no matter what.
 		 */
 		errflag = 0;
+		unqueue_signals();
 		preprompt();
+		queue_signals();
 		if (stophist != 3)
 		    hbegin(1);
 		else
@@ -218,6 +221,7 @@ loop(int toplevel, int justonce)
 	if (((!interact || sourcelevel) && errflag) || retflag)
 	    break;
 	if (isset(SINGLECOMMAND) && toplevel) {
+	    dont_queue_signals();
 	    if (sigtrapped[SIGEXIT])
 		dotrap(SIGEXIT);
 	    exit(lastval);
@@ -229,6 +233,7 @@ loop(int toplevel, int justonce)
     if (!toplevel)
 	zcontext_restore();
     popheap();
+    unqueue_signals();
 
     if (err)
 	return LOOP_ERROR;
diff --git a/Src/input.c b/Src/input.c
index 1efabadeb..eb968ea72 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -141,16 +141,19 @@ shingetline(void)
     int c;
     char buf[BUFSIZ];
     char *p;
+    int q = queue_signal_level();
 
     p = buf;
-    winch_unblock();
     for (;;) {
+	winch_unblock();
+	dont_queue_signals();
 	do {
 	    errno = 0;
 	    c = fgetc(bshin);
 	} while (c < 0 && errno == EINTR);
 	if (c < 0 || c == '\n') {
 	    winch_block();
+	    restore_queue_signals(q);
 	    if (c == '\n')
 		*p++ = '\n';
 	    if (p > buf) {
@@ -167,12 +170,13 @@ shingetline(void)
 	    *p++ = c;
 	if (p >= buf + BUFSIZ - 1) {
 	    winch_block();
+	    queue_signals();
 	    line = zrealloc(line, ll + (p - buf) + 1);
 	    memcpy(line + ll, buf, p - buf);
 	    ll += p - buf;
 	    line[ll] = '\0';
 	    p = buf;
-	    winch_unblock();
+	    unqueue_signals();
 	}
     }
 }
@@ -377,6 +381,8 @@ inputline(void)
 static void
 inputsetline(char *str, int flags)
 {
+    queue_signals();
+
     if ((inbufflags & INP_FREE) && inbuf) {
 	free(inbuf);
     }
@@ -394,6 +400,8 @@ inputsetline(char *str, int flags)
     else
 	inbufct = inbufleft;
     inbufflags = flags;
+
+    unqueue_signals();
 }
 
 /*
diff --git a/Src/loop.c b/Src/loop.c
index e4e8e2df8..4def9b652 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -56,6 +56,10 @@ execfor(Estate state, int do_exec)
     char *name, *str, *cond = NULL, *advance = NULL;
     zlong val = 0;
     LinkList vars = NULL, args = NULL;
+    int old_simple_pline = simple_pline;
+
+    /* See comments in execwhile() */
+    simple_pline = 1;
 
     end = state->pc + WC_FOR_SKIP(code);
 
@@ -69,10 +73,12 @@ execfor(Estate state, int do_exec)
 	    fprintf(xtrerr, "%s\n", str2);
 	    fflush(xtrerr);
 	}
-	if (!errflag)
+	if (!errflag) {
 	    matheval(str);
+	}
 	if (errflag) {
 	    state->pc = end;
+	    simple_pline = old_simple_pline;
 	    return 1;
 	}
 	cond = ecgetstr(state, EC_NODUP, &ctok);
@@ -85,12 +91,14 @@ execfor(Estate state, int do_exec)
 
 	    if (!(args = ecgetlist(state, *state->pc++, EC_DUPTOK, &htok))) {
 		state->pc = end;
+		simple_pline = old_simple_pline;
 		return 0;
 	    }
 	    if (htok) {
 		execsubst(args);
 		if (errflag) {
 		    state->pc = end;
+		    simple_pline = old_simple_pline;
 		    return 1;
 		}
 	    }
@@ -198,6 +206,7 @@ execfor(Estate state, int do_exec)
     popheap();
     cmdpop();
     loops--;
+    simple_pline = old_simple_pline;
     state->pc = end;
     return lastval;
 }
@@ -214,6 +223,10 @@ execselect(Estate state, UNUSED(int do_exec))
     FILE *inp;
     size_t more;
     LinkList args;
+    int old_simple_pline = simple_pline;
+
+    /* See comments in execwhile() */
+    simple_pline = 1;
 
     end = state->pc + WC_FOR_SKIP(code);
     name = ecgetstr(state, EC_NODUP, NULL);
@@ -229,18 +242,21 @@ execselect(Estate state, UNUSED(int do_exec))
 
 	if (!(args = ecgetlist(state, *state->pc++, EC_DUPTOK, &htok))) {
 	    state->pc = end;
+	    simple_pline = old_simple_pline;
 	    return 0;
 	}
 	if (htok) {
 	    execsubst(args);
 	    if (errflag) {
 		state->pc = end;
+		simple_pline = old_simple_pline;
 		return 1;
 	    }
 	}
     }
     if (!args || empty(args)) {
 	state->pc = end;
+	simple_pline = old_simple_pline;
 	return 0;
     }
     loops++;
@@ -315,6 +331,7 @@ execselect(Estate state, UNUSED(int do_exec))
     popheap();
     fclose(inp);
     loops--;
+    simple_pline = old_simple_pline;
     state->pc = end;
     return lastval;
 }
@@ -382,6 +399,7 @@ execwhile(Estate state, UNUSED(int do_exec))
     Wordcode end, loop;
     wordcode code = state->pc[-1];
     int olderrexit, oldval, isuntil = (WC_WHILE_TYPE(code) == WC_WHILE_UNTIL);
+    int old_simple_pline = simple_pline;
 
     end = state->pc + WC_WHILE_SKIP(code);
     olderrexit = noerrexit;
@@ -396,8 +414,6 @@ execwhile(Estate state, UNUSED(int do_exec))
         /* This is an empty loop.  Make sure the signal handler sets the
         * flags and then just wait for someone hitting ^C. */
 
-        int old_simple_pline = simple_pline;
-
         simple_pline = 1;
 
         while (!breaks)
@@ -409,7 +425,14 @@ execwhile(Estate state, UNUSED(int do_exec))
         for (;;) {
             state->pc = loop;
             noerrexit = 1;
+
+	    /* In case the test condition is a functional no-op,
+	     * make sure signal handlers recognize ^C to end the loop. */
+	    simple_pline = 1;
+
             execlist(state, 1, 0);
+
+	    simple_pline = old_simple_pline;
             noerrexit = olderrexit;
             if (!((lastval == 0) ^ isuntil)) {
                 if (breaks)
@@ -421,7 +444,14 @@ execwhile(Estate state, UNUSED(int do_exec))
                 lastval = oldval;
                 break;
             }
+
+	    /* In case the loop body is also a functional no-op,
+	     * make sure signal handlers recognize ^C as above. */
+	    simple_pline = 1;
+
             execlist(state, 1, 0);
+
+	    simple_pline = old_simple_pline;
             if (breaks) {
                 breaks--;
                 if (breaks || !contflag)
@@ -452,6 +482,10 @@ execrepeat(Estate state, UNUSED(int do_exec))
     wordcode code = state->pc[-1];
     int count, htok = 0;
     char *tmp;
+    int old_simple_pline = simple_pline;
+
+    /* See comments in execwhile() */
+    simple_pline = 1;
 
     end = state->pc + WC_REPEAT_SKIP(code);
 
@@ -484,6 +518,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
     cmdpop();
     popheap();
     loops--;
+    simple_pline = old_simple_pline;
     state->pc = end;
     return lastval;
 }
diff --git a/Src/parse.c b/Src/parse.c
index 09567fde2..1a7416449 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -456,6 +456,8 @@ init_parse_status(void)
 void
 init_parse(void)
 {
+    queue_signals();
+
     if (ecbuf) zfree(ecbuf, eclen);
 
     ecbuf = (Wordcode) zalloc((eclen = EC_INIT_SIZE) * sizeof(wordcode));
@@ -466,6 +468,8 @@ init_parse(void)
     ecnfunc = 0;
 
     init_parse_status();
+
+    unqueue_signals();
 }
 
 /* Build eprog. */
@@ -488,6 +492,8 @@ bld_eprog(int heap)
     Eprog ret;
     int l;
 
+    queue_signals();
+
     ecadd(WCB_END());
 
     ret = heap ? (Eprog) zhalloc(sizeof(*ret)) : (Eprog) zalloc(sizeof(*ret));
@@ -511,6 +517,8 @@ bld_eprog(int heap)
     zfree(ecbuf, eclen);
     ecbuf = NULL;
 
+    unqueue_signals();
+
     return ret;
 }
 
diff --git a/Src/signals.c b/Src/signals.c
index 3950ad1a2..697c4c5ec 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -1207,6 +1207,8 @@ dotrapargs(int sig, int *sigtr, void *sigfn)
 	}
     }
 
+    queue_signals();	/* Any time we manage memory or global state */
+
     intrap++;
     *sigtr |= ZSIG_IGNORED;
 
@@ -1244,7 +1246,7 @@ dotrapargs(int sig, int *sigtr, void *sigfn)
 
 	sfcontext = SFC_SIGNAL;
 	incompfunc = 0;
-	doshfunc((Shfunc)sigfn, args, 1);
+	doshfunc((Shfunc)sigfn, args, 1);	/* manages signal queueing */
 	sfcontext = osc;
 	incompfunc= old_incompfunc;
 	freelinklist(args, (FreeFunc) NULL);
@@ -1254,7 +1256,7 @@ dotrapargs(int sig, int *sigtr, void *sigfn)
 	trap_state = TRAP_STATE_PRIMED;
 	trapisfunc = isfunc = 0;
 
-	execode((Eprog)sigfn, 1, 0, "trap");
+	execode((Eprog)sigfn, 1, 0, "trap");	/* manages signal queueing */
     }
     runhookdef(AFTERTRAPHOOK, NULL);
 
@@ -1321,6 +1323,8 @@ dotrapargs(int sig, int *sigtr, void *sigfn)
     if (*sigtr != ZSIG_IGNORED)
 	*sigtr &= ~ZSIG_IGNORED;
     intrap--;
+
+    unqueue_signals();
 }
 
 /* Standard call to execute a trap for a given signal. */