Call stack issues when running trap handler

Call stack issues when running trap handler

[Sebastian Reuße]

I’ve noticed that under certain conditions, shell functions called from
inside an exit trap handler appear to never return. E.g., running the
following code will only yield «a» as output, indicating that callee
«echoa» never returns control to the caller «handler».

#+begin_src shell
trap handler EXIT

handler() {
    echoa
    echo b
}

echoa() {
    echo a
}

exit0() {
    exit
}

main() {
    exit0
}

main

#+end_src

When directly calling «exit0» or «exit» instead of «main», this issue
does not arise.

I was able to reproduce this both in Zsh 5.3.1 as well as today’s HEAD.

Kind regards,
SR

Re: Call stack issues when running trap handler

[Bart Schaefer]

On Apr 21, 10:32pm, Sebastian Reusse wrote:
}
} I've noticed that under certain conditions, shell functions called from
} inside an exit trap handler appear to never return.

Whatever this is dates back a long time.  In zsh-3, the sample script
doesn't appear to call the exit trap at all via "main" or "exit0"
though it does if "exit" is called directly.  In zsh-4 the behavior
is the same as in current zsh-5.

Re: Call stack issues when running trap handler

[Peter Stephenson]

On Fri, 21 Apr 2017 22:32:46 +0200
Sebastian Reuße <seb@wirrsal.net> wrote:
> I’ve noticed that under certain conditions, shell functions called from
> inside an exit trap handler appear to never return. E.g., running the
> following code will only yield «a» as output, indicating that callee
> «echoa» never returns control to the caller «handler».

This is working OK for me, in my normal configuration or starting
from scratch, so there appears to be some other ingredient to add.

pws

Re: Call stack issues when running trap handler

[Bart Schaefer]

On Mon, Apr 24, 2017 at 2:00 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Fri, 21 Apr 2017 22:32:46 +0200
>
> This is working OK for me, in my normal configuration or starting
> from scratch, so there appears to be some other ingredient to add.

I am able to reproduce Sebastian's symptom exactly when starting any
version from current back to 4.3.17.

So if there's an ingredient, it's yours to make it work correctly.

Re: Call stack issues when running trap handler

[Peter Stephenson]

On Mon, 24 Apr 2017 09:16:55 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Mon, Apr 24, 2017 at 2:00 AM, Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> > On Fri, 21 Apr 2017 22:32:46 +0200
> >
> > This is working OK for me, in my normal configuration or starting
> > from scratch, so there appears to be some other ingredient to add.
> 
> I am able to reproduce Sebastian's symptom exactly when starting any
> version from current back to 4.3.17.
> 
> So if there's an ingredient, it's yours to make it work correctly.

I'm aware others can reproduce it; my only point is I can't so won't be
looking at it.

pws

Re: Call stack issues when running trap handler

[Bart Schaefer]

On Mon, Apr 24, 2017 at 10:17 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> I'm aware others can reproduce it; my only point is I can't so won't be
> looking at it.

OK.  Perhaps I should know this from previous exchanges, but could you
describe the environment in which it works?  E.g. it fails for me on
all of MacOS 10.9.5 and Ubuntu 12.04.5 and on an embarrassingly old
CentOS virtual machine, all built from latest git and started from
scratch with "zsh -f".

Re: Call stack issues when running trap handler

[Peter Stephenson]

On Fri, 21 Apr 2017 22:32:46 +0200
Sebastian Reuße <seb@wirrsal.net> wrote:
> I’ve noticed that under certain conditions, shell functions called from
> inside an exit trap handler appear to never return. E.g., running the
> following code will only yield «a» as output, indicating that callee
> «echoa» never returns control to the caller «handler».

I've just re-read this and compared with the evidence and obviously I've
misinterpreted it.  When you said "never return", you meant it exited
from that point (so never printed "b" but did leave the shell
nonetheless).  I interpreted this as saying it got stuck in that
function, but that obviously isn't what you mean.

This I can fix.

If anyone knows whether I need to distinguish in any of the following
between EXIT traps for functions and the shell as a whole, feel free to
tell me.  I'm assuming it's going to be too arcane to worry about.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index b2e552d..ff07b04 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5509,8 +5509,11 @@ 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* in an EXIT trap... give this all up as
+	     * a bad job.
 	     */
-	    if (stopmsg || (zexit(0,2), !stopmsg)) {
+	    if ((stopmsg || (zexit(0,2), !stopmsg)) && !in_exit_trap) {
 		retflag = 1;
 		breaks = loops;
 		exit_pending = (num << 1) | 1;
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);
 }

Re: Call stack issues when running trap handler

[Peter Stephenson]

On Wed, 26 Apr 2017 20:27:29 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> 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

Re: Call stack issues when running trap handler

[Bart Schaefer]

On Apr 26,  8:52pm, Peter Stephenson wrote:
}
} I think I like this better.

I like it better, too, tho mostly because it means I can stop looking at it.

Re: Call stack issues when running trap handler

[Sebastian Reuße]

> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:

>> This I can fix.

Only got to follow up on this now. Working fine for me on HEAD. Thank
you kindly!

Best regards,

-- 
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay