Re: Call stack issues when running trap handler

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

On Wed, 26 Apr 2017 20:27:29 +0100
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx> wrote:
> This I can fix.

I think I like this better.

shell_exiting looks like a good way of ensuring we don't tie ourselves
in knots with exits, independent of in_exit_trap which we still need for
the original fix.  The interaction is that when we commit to exiting
shell_exiting goes to -1; then we won't set exit_pending in zexit();
that means we don't hit the case that caused us to exit early.  Or
something.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index b2e552d..063644e 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5500,7 +5500,7 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func)
 	}
 	/*FALLTHROUGH*/
     case BIN_EXIT:
-	if (locallevel > forklevel) {
+	if (locallevel > forklevel && shell_exiting != -1) {
 	    /*
 	     * We don't exit directly from functions to allow tidying
 	     * up, in particular EXIT traps.  We still need to perform
@@ -5509,6 +5509,9 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func)
 	     *
 	     * If we are forked, we exit the shell at the function depth
 	     * at which we became a subshell, hence the comparison.
+	     *
+	     * If we are already exiting... give this all up as
+	     * a bad job.
 	     */
 	    if (stopmsg || (zexit(0,2), !stopmsg)) {
 		retflag = 1;
@@ -5555,6 +5558,14 @@ checkjobs(void)
     }
 }
 
+/*
+ * -1 if the shell is already committed to exit.
+ * positive if zexit() was already called.
+ */
+
+/**/
+int shell_exiting;
+
 /* exit the shell.  val is the return value of the shell.  *
  * from_where is
  *   1   if zexit is called because of a signal
@@ -5566,10 +5577,8 @@ checkjobs(void)
 mod_export void
 zexit(int val, int from_where)
 {
-    static int in_exit;
-
     /* Don't do anything recursively:  see below */
-    if (in_exit == -1)
+    if (shell_exiting == -1)
 	return;
 
     if (isset(MONITOR) && !stopmsg && from_where != 1) {
@@ -5582,14 +5591,14 @@ zexit(int val, int from_where)
 	}
     }
     /* Positive in_exit means we have been here before */
-    if (from_where == 2 || (in_exit++ && from_where))
+    if (from_where == 2 || (shell_exiting++ && from_where))
 	return;
 
     /*
-     * We're now committed to exiting.  Set in_exit to -1 to
+     * We're now committed to exiting.  Set shell_exiting to -1 to
      * indicate we shouldn't do any recursive processing.
      */
-    in_exit = -1;
+    shell_exiting = -1;
     /*
      * We want to do all remaining processing regardless of preceding
      * errors, even user interrupts.
diff --git a/Src/exec.c b/Src/exec.c
index 978a32d..e0fc544 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5688,8 +5688,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
      * the only likely case where we need that second test is
      * when we have an "always" block.  The endparamscope() has
      * already happened, hence the "+1" here.
+     *
+     * If we are in an exit trap, finish it first... we wouldn't set
+     * exit_pending if we were already in one.
      */
-    if (exit_pending && exit_level >= locallevel+1) {
+    if (exit_pending && exit_level >= locallevel+1 && !in_exit_trap) {
 	if (locallevel > forklevel) {
 	    /* Still functions to return: force them to do so. */
 	    retflag = 1;
diff --git a/Src/signals.c b/Src/signals.c
index 68a7ae3..cad40f4 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -55,6 +55,11 @@ mod_export Eprog siglists[VSIGCOUNT];
 /**/
 mod_export int nsigtrapped;
 
+/* Running an exit trap? */
+
+/**/
+int in_exit_trap;
+
 /*
  * Flag that exit trap has been set in POSIX mode.
  * The setter's expectation is therefore that it is run
@@ -1435,7 +1440,13 @@ dotrap(int sig)
 
     dont_queue_signals();
 
+    if (sig == SIGEXIT)
+	++in_exit_trap;
+
     dotrapargs(sig, sigtrapped+sig, funcprog);
 
+    if (sig == SIGEXIT)
+	--in_exit_trap;
+
     restore_queue_signals(q);
 }
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index 7bc0b48..7594012 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -756,6 +756,27 @@ F:Must be tested with a top-level script rather than source or function
 >''
 >hello
 
+  $ZTST_testdir/../Src/zsh -f =(<<<"
+    trap handler EXIT
+    handler() {
+      echoa
+      echo b
+    }
+    echoa() {
+      echo a
+    }
+    exit0() {
+      exit
+    }
+    main() {
+      exit0
+    }
+    main
+  ")
+0:No early exit from nested function in EXIT trap.
+>a
+>b
+
 %clean
 
   rm -f TRAPEXIT