Re: [Bug] Exiting shell from function called by trap handler always produces status 0

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

iOn Tue, 2018-10-09 at 12:46 +0100, Martijn Dekker wrote:
> Op 09-10-18 om 09:49 schreef Peter Stephenson:
> > On Mon, 2018-10-08 at 14:02 +0100, Martijn Dekker wrote:
> > > 
> > > When a trap handler exits the shell using the 'exit' command within a
> > > function, the shell's exit status is zero even if another exit status
> > > was given as an argument to the 'exit' command.
> > >   
> > > $ Src/zsh -c 'fn() { exit 13; }; trap "fn" EXIT'
> > > $ echo $?
> > > 0
> > > (expected output: 13)
> > diff --git a/Src/builtin.c b/Src/builtin.c
> > index c5b319b..b81acdb 100644
> [...]
> 
> This still doesn't fix it. Output identical to above.

Fair enough, it was me that added the explicit "exit" at the end, not you.

Grumble.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index ca3ef23..905edd9 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5647,8 +5647,9 @@ bin_break(char *name, char **argv, UNUSED(Options ops),
int func)
 	    if (stopmsg || (zexit(0,2), !stopmsg)) {
 		retflag = 1;
 		breaks = loops;
-		exit_pending = (num << 1) | 1;
+		exit_pending = 1;
 		exit_level = locallevel;
+		exit_val = num;
 	    }
 	} else
 	    zexit(num, 0);
@@ -5698,6 +5699,42 @@ checkjobs(void)
 /**/
 int shell_exiting;
 
+/*
+ * Exit status if explicitly set by an exit command.
+ * This is complicated by the fact the exit command may be within
+ * a function whose state we need to unwind (exit_pending set
+ * and the exit will happen up the stack), or we may need to execute
+ * additional code such as a trap after we are committed to exiting
+ * (shell_exiting and the exit will happen down the stack).
+ *
+ * It's lucky this is all so obvious there is no possibility of any
+ * bugs.  (C.f. the entire rest of the shell.)
+ */
+/**/
+int exit_val;
+
+/*
+ * Actually exit the shell, working out the status locally.
+ * This is exit_val if "exit" has explicitly been called in the shell,
+ * else lastval.
+ */
+
+/**/
+void
+realexit(void)
+{
+    exit(exit_val ? exit_val : lastval);
+}
+
+/* As realexit(), but call _exit instead */
+
+/**/
+void
+_realexit(void)
+{
+    _exit(exit_val ? exit_val : lastval);
+}
+
 /* exit the shell.  val is the return value of the shell.  *
  * from_where is
  *   1   if zexit is called because of a signal
@@ -5709,7 +5746,6 @@ int shell_exiting;
 mod_export void
 zexit(int val, int from_where)
 {
-    static int exit_val;
     /*
      * Don't do anything recursively:  see below.
      * Do, however, update exit status --- there's no nesting,
diff --git a/Src/exec.c b/Src/exec.c
index 1d537af..c4a2740 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -738,7 +738,7 @@ execute(LinkList args, int flags, int defpath)
 
 	if (!search_defpath(arg0, pbuf, PATH_MAX)) {
 	    if (commandnotfound(arg0, args) == 0)
-		_exit(lastval);
+		_realexit();
 	    zerr("command not found: %s", arg0);
 	    _exit(127);
 	}
@@ -802,7 +802,7 @@ execute(LinkList args, int flags, int defpath)
     if (eno)
 	zerr("%e: %s", eno, arg0);
     else if (commandnotfound(arg0, args) == 0)
-	_exit(lastval);
+	_realexit();
     else
 	zerr("command not found: %s", arg0);
     _exit((eno == EACCES || eno == ENOEXEC) ? 126 : 127);
@@ -1012,6 +1012,7 @@ entersubsh(int flags, struct entersubsh_ret *retp)
 		unsettrap(sig);
     monitor = isset(MONITOR);
     job_control_ok = monitor && (flags & ESUB_JOB_CONTROL) &&
isset(POSIXJOBS);
+    exit_val = 0; 		/* parent exit status is irrelevant */
     if (flags & ESUB_NOMONITOR)
 	opts[MONITOR] = 0;
     if (!isset(MONITOR)) {
@@ -1535,9 +1536,9 @@ sublist_done:
 		    if (sigtrapped[SIGEXIT])
 			dotrap(SIGEXIT);
 		    if (mypid != getpid())
-			_exit(lastval);
+			_realexit();
 		    else
-			exit(lastval);
+			realexit();
 		}
 		if (errreturn) {
 		    retflag = 1;
@@ -2934,7 +2935,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		/* autoload the builtin if necessary */
 		if (!(hn = resolvebuiltin(cmdarg, hn))) {
 		    if (forked)
-			_exit(lastval);
+			_realexit();
 		    return;
 		}
 		if (type != WC_TYPESET)
@@ -3115,7 +3116,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			    lastval = 1;
 			    errflag |= ERRFLAG_ERROR;
 			    if (forked)
-				_exit(lastval);
+				_realexit();
 			    return;
 			}
 		    }
@@ -3210,7 +3211,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			lastval = 1;
 			errflag |= ERRFLAG_ERROR;
 			if (forked)
-			    _exit(lastval);
+			    _realexit();
 			return;
 		    } else if (!nullcmd || !*nullcmd || opts[SHNULLCMD]) {
 			if (!args)
@@ -3230,7 +3231,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		} else if ((cflags & BINF_PREFIX) && (cflags & BINF_COMMAND))
{
 		    lastval = 0;
 		    if (forked)
-			_exit(lastval);
+			_realexit();
 		    return;
 		} else {
 		    /*
@@ -3242,7 +3243,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			zerr("no match");
 			lastval = 1;
 			if (forked)
-			    _exit(lastval);
+			    _realexit();
 			return;
 		    }
 		    cmdoutval = use_cmdoutval ? lastval : 0;
@@ -3260,7 +3261,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			fflush(xtrerr);
 		    }
 		    if (forked)
-			_exit(lastval);
+			_realexit();
 		    return;
 		}
 	    } else if (isset(RESTRICTED) && (cflags & BINF_EXEC) && do_exec)
{
@@ -3268,7 +3269,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			(char *) getdata(firstnode(args)));
 		lastval = 1;
 		if (forked)
-		    _exit(lastval);
+		    _realexit();
 		return;
 	    }
 
@@ -3304,7 +3305,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		    if (oautocont >= 0)
 			opts[AUTOCONTINUE] = oautocont;
 		    if (forked)
-			_exit(lastval);
+			_realexit();
 		    return;
 		}
 		break;
@@ -3315,7 +3316,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		/* autoload the builtin if necessary */
 		if (!(hn = resolvebuiltin(cmdarg, hn))) {
 		    if (forked)
-			_exit(lastval);
+			_realexit();
 		    return;
 		}
 		break;
@@ -3333,7 +3334,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	if (oautocont >= 0)
 	    opts[AUTOCONTINUE] = oautocont;
 	if (forked)
-	    _exit(lastval);
+	    _realexit();
 	return;
     }
 
@@ -3412,7 +3413,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		if (oautocont >= 0)
 		    opts[AUTOCONTINUE] = oautocont;
 		if (forked)
-		    _exit(lastval);
+		    _realexit();
 		return;
 	    }
 	}
@@ -3442,7 +3443,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	if (oautocont >= 0)
 	    opts[AUTOCONTINUE] = oautocont;
 	if (forked)
-	    _exit(lastval);
+	    _realexit();
 	return;
     }
 
@@ -4118,13 +4119,13 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 
 	    if (do_exec) {
 		if (subsh)
-		    _exit(lastval);
+		    _realexit();
 
 		/* If we are exec'ing a command, and we are not in a
subshell, *
 		 * then check if we should save the history
file.              */
 		if (isset(RCS) && interact && !nohistsave)
 		    savehistfile(NULL, 1, HFILE_USE_OPTIONS);
-		exit(lastval);
+		realexit();
 	    }
 	    if (restorelist)
 		restore_params(restorelist, removelist);
@@ -4215,7 +4216,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	closem(FDT_UNUSED, 1);
 	if (thisjob != -1)
 	    waitjobs();
-	_exit(lastval);
+	_realexit();
     }
     fixfds(save);
 
@@ -4631,7 +4632,7 @@ getoutput(char *cmd, int qt)
     execode(prog, 0, 1, "cmdsubst");
     cmdpop();
     close(1);
-    _exit(lastval);
+    _realexit();
     zerr("exit returned in child!!");
     kill(getpid(), SIGKILL);
     return NULL;
@@ -4825,7 +4826,7 @@ getoutputfile(char *cmd, char **eptr)
     execode(prog, 0, 1, "equalsubst");
     cmdpop();
     close(1);
-    _exit(lastval);
+    _realexit();
     zerr("exit returned in child!!");
     kill(getpid(), SIGKILL);
     return NULL;
@@ -4938,7 +4939,7 @@ getproc(char *cmd, char **eptr)
     execode(prog, 0, 1, out ? "outsubst" : "insubst");
     cmdpop();
     zclose(out);
-    _exit(lastval);
+    _realexit();
     return NULL;
 #endif   /* HAVE_FIFOS and PATH_DEV_FD not defined */
 }
@@ -4986,7 +4987,7 @@ getpipe(char *cmd, int nullexec)
     cmdpush(CS_CMDSUBST);
     execode(prog, 0, 1, out ? "outsubst" : "insubst");
     cmdpop();
-    _exit(lastval);
+    _realexit();
     return 0;
 }
 
@@ -5927,7 +5928,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int
noreturnval)
 	     * exit command was handled.
 	     */
 	    stopmsg = 1;
-	    zexit(exit_pending >> 1, 0);
+	    zexit(exit_val, 0);
 	}
     }
 
diff --git a/Src/init.c b/Src/init.c
index e9e6be9..838c2c2 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -157,7 +157,7 @@ loop(int toplevel, int justonce)
 		 * Handle that now.
 		 */
 		stopmsg = 1;
-		zexit(exit_pending >> 1, 0);
+		zexit(exit_val, 0);
 	    }
 	    if (tok == LEXERR && !lastval)
 		lastval = 1;
@@ -215,14 +215,14 @@ loop(int toplevel, int justonce)
 	    clearerr(stderr);
 	}
 	if (subsh)		/* how'd we get this far in a subshell? */
-	    exit(lastval);
+	    realexit();
 	if (((!interact || sourcelevel) && errflag) || retflag)
 	    break;
 	if (isset(SINGLECOMMAND) && toplevel) {
 	    dont_queue_signals();
 	    if (sigtrapped[SIGEXIT])
 		dotrap(SIGEXIT);
-	    exit(lastval);
+	    realexit();
 	}
 	if (justonce)
 	    break;
@@ -1358,7 +1358,7 @@ init_misc(char *cmd, char *zsh_name)
 	bshin = fdopen(SHIN, "r");
 	execstring(cmd, 0, 1, "cmdarg");
 	stopmsg = 1;
-	zexit(lastval, 0);
+	zexit(exit_val ? exit_val : lastval, 0);
     }
 
     if (interact && isset(RCS))
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index eab01e5..bab0b0a 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -869,6 +869,10 @@ F:Must be tested with a top-level script rather than
source or function
   $ZTST_testdir/../Src/zsh -fc 'fn() { exit $?+8; }; trap fn EXIT; exit 7'
 15:Progated exit status through exit trap
 
+  $ZTST_testdir/../Src/zsh -fc 'fn() { exit 13; }; trap fn EXIT'
+13:Explicit exit in exit trap overrides implicit exit status
+
 %clean
 
   rm -f TRAPEXIT
+