PATCH signal queueing and deadlocks and interrupts

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

This consolidates or replaces all of the patches in 35987, 35992, 35997,
36004, and 36007 (see 36010).

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; fast multiprocessing means we can no longer rely on
operating system process switching contstraints to guarantee order of
some operations.  (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.

I have chosen, possibly unwisely, to exclude parameter assignments (e.g,
assignment-only commands, assignments prefixing commands, and prefork
substitutions) from (2) except when command or process substitution is
involved, because those actions operate exclusively on internal shell
memory.  If someone can come up with an example (e.g., a side-effect)
where a signal trap is not executed in the expected order with respect
to an assignment, we can revisit this.

It was already the case that signal queueing is disabled whenever ZLE
is waiting for input, so I made that the case for non-ZLE shell input
as well [mirroring what's done with winch_unblock() in shingetline()].

However, there's an odd situation (which is not new) wherein ^C at the
prompt when ZLE is disabled does not reprint the prompt; instead the
read() system call is simply restarted even though the input up to
that point is correctly discarded.  Surprisingly, this is NOT because
of the "&& errno == EINTR" test in shingetline(), which must be there
solely to simulate restarting the system call in case that's not
supported by the OS.  I think instead there's an assumption that
shingetline() is reading a script file, and therefore zhandler() is
not doing its job when the input is a terminal.  This patch does not
address this (though not for lack of my trying to figure it out).

The most controversial part of this patch is probably the change to
loop() in init.c.  Calling queue_signals() here means that most of the
shell code is executed with signals queued.  However, because signals
are re-enabled any time the shell is reading input or executing a
command, most of the *time* it should be operating with signals not
queued.  Individual builtins are still responsible for keeping their
own memory state signal-safe, for example, and I haven't gone to any
effort to fix that.

Finally, note all the stuff with simple_pline in loop.c.  This fixes
a long-standing bug where a loop like this:

    while x=y; do y=x; done

was not interruptible.  Similarly with

    for ((;1;)); do y=x; done
    repeat $(( reallylargenumber )); do x=y; done

It's possible that all of this simple_pline frobbing should be lifted
out of loop.c and added to exec.c around "execfuncs[code - WC_CURSH]"
(see first hunk below), but I wasn't sure that was correct.


diff --git a/Src/exec.c b/Src/exec.c
index 7612d43..a635c18 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 2ef9099..f2021f0 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 1efabad..eb968ea 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 e4e8e2d..4def9b6 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 09567fd..1a74164 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 3950ad1..697c4c5 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. */