zsh-workers
 help / color / mirror / code / Atom feed
* [Bug] Exiting shell from function called by trap handler always produces status 0
@ 2018-10-08 13:02 ` Martijn Dekker
  2018-10-09  8:49   ` Peter Stephenson
  0 siblings, 1 reply; 11+ messages in thread
From: Martijn Dekker @ 2018-10-08 13:02 UTC (permalink / raw)
  To: Zsh hackers list

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)

I was able to reproduce this in zsh 5.4.1 and up, and not in 5.3.1 and 
earlier. Every other shell also exits with status 13.

Looping through git commits starting from zsh-5.3.1, I found the commit 
that introduced this: d7110d8 (41012: Fix premature exit from nested 
function in EXIT trap).

Thanks,

- M.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-08 13:02 ` [Bug] Exiting shell from function called by trap handler always produces status 0 Martijn Dekker
@ 2018-10-09  8:49   ` Peter Stephenson
  2018-10-09  9:58     ` Mikael Magnusson
  2018-10-09 11:46     ` Martijn Dekker
  0 siblings, 2 replies; 11+ messages in thread
From: Peter Stephenson @ 2018-10-09  8:49 UTC (permalink / raw)
  To: zsh-workers

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
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5709,7 +5709,13 @@ int shell_exiting;
 mod_export void
 zexit(int val, int from_where)
 {
-    /* Don't do anything recursively:  see below */
+    static int exit_val;
+    /*
+     * Don't do anything recursively:  see below.
+     * Do, however, update exit status --- there's no nesting,
+     * a later value always overrides an earlier.
+     */
+    exit_val = val;
     if (shell_exiting == -1)
 	return;
 
@@ -5757,7 +5763,7 @@ zexit(int val, int from_where)
 #endif
 	}
     }
-    lastval = val;
+    lastval = exit_val;
     /*
      * Now we are committed to exiting any previous state
      * is irrelevant.  Ensure trap can run.
@@ -5771,9 +5777,9 @@ zexit(int val, int from_where)
        release_pgrp();
     }
     if (mypid != getpid())
-	_exit(val);
+	_exit(exit_val);
     else
-	exit(val);
+	exit(exit_val);
 }
 
 /* . (dot), source */
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index dce263f..3b5d4c6 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -863,6 +863,9 @@ F:Must be tested with a top-level script rather than
source or function
 >a
 >b
 
+  $ZTST_testdir/../Src/zsh -fc 'fn() { exit 13; }; trap fn EXIT; exit'
+13:Explicit exit in exit trap overrides status
+
 %clean
 
   rm -f TRAPEXIT


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09  8:49   ` Peter Stephenson
@ 2018-10-09  9:58     ` Mikael Magnusson
  2018-10-09 11:46     ` Martijn Dekker
  1 sibling, 0 replies; 11+ messages in thread
From: Mikael Magnusson @ 2018-10-09  9:58 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: zsh-workers

On 10/9/18, Peter Stephenson <p.stephenson@samsung.com> wrote:
> 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
> --- a/Src/builtin.c
> +++ b/Src/builtin.c
> @@ -5709,7 +5709,13 @@ int shell_exiting;
>  mod_export void
>  zexit(int val, int from_where)
>  {
> -    /* Don't do anything recursively:  see below */
> +    static int exit_val;
> +    /*
> +     * Don't do anything recursively:  see below.
> +     * Do, however, update exit status --- there's no nesting,
> +     * a later value always overrides an earlier.
> +     */
> +    exit_val = val;
>      if (shell_exiting == -1)
>  	return;
>
> @@ -5757,7 +5763,7 @@ zexit(int val, int from_where)
>  #endif
>  	}
>      }
> -    lastval = val;
> +    lastval = exit_val;
>      /*
>       * Now we are committed to exiting any previous state
>       * is irrelevant.  Ensure trap can run.
> @@ -5771,9 +5777,9 @@ zexit(int val, int from_where)
>         release_pgrp();
>      }
>      if (mypid != getpid())
> -	_exit(val);
> +	_exit(exit_val);
>      else
> -	exit(val);
> +	exit(exit_val);
>  }
>
>  /* . (dot), source */
> diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
> index dce263f..3b5d4c6 100644
> --- a/Test/C03traps.ztst
> +++ b/Test/C03traps.ztst
> @@ -863,6 +863,9 @@ F:Must be tested with a top-level script rather than
> source or function
>  >a
>  >b
>
> +  $ZTST_testdir/../Src/zsh -fc 'fn() { exit 13; }; trap fn EXIT; exit'
> +13:Explicit exit in exit trap overrides status
> +
>  %clean
>
>    rm -f TRAPEXIT

We might as well make this test
+  $ZTST_testdir/../Src/zsh -fc 'fn() { exit $?+7; }; trap fn EXIT; exit 6'
+13:Explicit exit in exit trap overrides status

-- 
Mikael Magnusson

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09  8:49   ` Peter Stephenson
  2018-10-09  9:58     ` Mikael Magnusson
@ 2018-10-09 11:46     ` Martijn Dekker
  2018-10-09 13:16       ` Peter Stephenson
  1 sibling, 1 reply; 11+ messages in thread
From: Martijn Dekker @ 2018-10-09 11:46 UTC (permalink / raw)
  To: zsh-workers

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.

> +  $ZTST_testdir/../Src/zsh -fc 'fn() { exit 13; }; trap fn EXIT; exit'

The extra '; exit' at the end makes it work correctly, but should not be 
necessary.

Thanks,

- M.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09 11:46     ` Martijn Dekker
@ 2018-10-09 13:16       ` Peter Stephenson
  2018-10-09 13:39         ` Daniel Shahaf
  2018-10-09 20:09         ` Peter Stephenson
  0 siblings, 2 replies; 11+ messages in thread
From: Peter Stephenson @ 2018-10-09 13:16 UTC (permalink / raw)
  To: zsh-workers

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
+




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09 13:16       ` Peter Stephenson
@ 2018-10-09 13:39         ` Daniel Shahaf
  2018-10-09 13:43           ` Peter Stephenson
       [not found]           ` <1539092591.3286.12.camel@samsung.com>
  2018-10-09 20:09         ` Peter Stephenson
  1 sibling, 2 replies; 11+ messages in thread
From: Daniel Shahaf @ 2018-10-09 13:39 UTC (permalink / raw)
  To: Peter Stephenson, zsh-workers

Peter Stephenson wrote on Tue, 09 Oct 2018 14:16 +0100:
> +++ b/Src/builtin.c
> @@ -5698,6 +5699,42 @@ checkjobs(void)
> +/*
> + * 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;

Shouldn't this variable be initialized?

A function-local 'static' (below) is implicitly initialized to 0, but
a non-static global is not implicitly initialized.

> +
> +/*
> + * Actually exit the shell, working out the status locally.
> + * This is exit_val if "exit" has explicitly been called in the shell,
> + * else lastval.
> + */
> +
> @@ -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,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09 13:39         ` Daniel Shahaf
@ 2018-10-09 13:43           ` Peter Stephenson
       [not found]           ` <1539092591.3286.12.camel@samsung.com>
  1 sibling, 0 replies; 11+ messages in thread
From: Peter Stephenson @ 2018-10-09 13:43 UTC (permalink / raw)
  To: Daniel Shahaf, zsh-workers

On Tue, 2018-10-09 at 13:39 +0000, Daniel Shahaf wrote:
> Peter Stephenson wrote on Tue, 09 Oct 2018 14:16 +0100:
> A function-local 'static' (below) is implicitly initialized to 0, but
> a non-static global is not implicitly initialized.

If that's really the case then the entire shell (look around at other
cases), and pretty much every other non-trivial programme I've ever
written, is fundamentally broken.  I'm not going to try to fix that now.

I can't quote chapter and verse but in all interesting cases they're
initialised to zero.

pws





^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
       [not found]           ` <1539092591.3286.12.camel@samsung.com>
@ 2018-10-09 13:50             ` Peter Stephenson
  0 siblings, 0 replies; 11+ messages in thread
From: Peter Stephenson @ 2018-10-09 13:50 UTC (permalink / raw)
  To: zsh-workers

On Tue, 2018-10-09 at 14:43 +0100, Peter Stephenson wrote:
> On Tue, 2018-10-09 at 13:39 +0000, Daniel Shahaf wrote:
> > Peter Stephenson wrote on Tue, 09 Oct 2018 14:16 +0100:
> > A function-local 'static' (below) is implicitly initialized to 0, but
> > a non-static global is not implicitly initialized.
> If that's really the case then the entire shell (look around at other
> cases), and pretty much every other non-trivial programme I've ever
> written, is fundamentally broken.  I'm not going to try to fix that now.
> 
> I can't quote chapter and verse but in all interesting cases they're
> initialised to zero.

Looking around at some discussions of this subject on websites...

I wonder if you're getting confused between static and automatic
*allocation* vs. global and static *linkage*?  Linkage has no effect
here: global variables are statically allocated.

pws


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09 13:16       ` Peter Stephenson
  2018-10-09 13:39         ` Daniel Shahaf
@ 2018-10-09 20:09         ` Peter Stephenson
  2018-10-10  8:56           ` Daniel Shahaf
  1 sibling, 1 reply; 11+ messages in thread
From: Peter Stephenson @ 2018-10-09 20:09 UTC (permalink / raw)
  To: zsh-workers

Here's a slight improvement --- we can "exit 0" even if the last command
status is non-zero.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index e01e035cc..8dcdcc024 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5723,7 +5723,7 @@ int exit_val;
 void
 realexit(void)
 {
-    exit(exit_val ? exit_val : lastval);
+    exit((shell_exiting || exit_pending) ? exit_val : lastval);
 }
 
 /* As realexit(), but call _exit instead */
@@ -5732,7 +5732,7 @@ realexit(void)
 void
 _realexit(void)
 {
-    _exit(exit_val ? exit_val : lastval);
+    _exit((shell_exiting || exit_pending) ? exit_val : lastval);
 }
 
 /* exit the shell.  val is the return value of the shell.  *

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-09 20:09         ` Peter Stephenson
@ 2018-10-10  8:56           ` Daniel Shahaf
  2018-10-10  9:31             ` Peter Stephenson
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Shahaf @ 2018-10-10  8:56 UTC (permalink / raw)
  To: zsh-workers

Peter Stephenson wrote on Tue, 09 Oct 2018 21:09 +0100:
> Here's a slight improvement --- we can "exit 0" even if the last command
> status is non-zero.

Can we add a test for this?  I tried to write one but couldn't nail down
the behaviour that differed.

FWIW:

% Src/zsh -fc 'fn() { exit 0 }; trap fn EXIT; false'  ; echo $?
1
% /usr/bin/zsh -fc 'fn() { exit 0 }; trap fn EXIT; false' ; echo $?
0
% 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug] Exiting shell from function called by trap handler always produces status 0
  2018-10-10  8:56           ` Daniel Shahaf
@ 2018-10-10  9:31             ` Peter Stephenson
  0 siblings, 0 replies; 11+ messages in thread
From: Peter Stephenson @ 2018-10-10  9:31 UTC (permalink / raw)
  To: zsh-workers

On Wed, 2018-10-10 at 08:56 +0000, Daniel Shahaf wrote:
> Peter Stephenson wrote on Tue, 09 Oct 2018 21:09 +0100:
> > 
> > Here's a slight improvement --- we can "exit 0" even if the last command
> > status is non-zero.
> Can we add a test for this?  I tried to write one but couldn't nail down
> the behaviour that differed.
> 
> FWIW:
> 
> % Src/zsh -fc 'fn() { exit 0 }; trap fn EXIT; false'  ; echo $?
> 1
> % /usr/bin/zsh -fc 'fn() { exit 0 }; trap fn EXIT; false' ; echo $?
> 0
> % 

I missed another case for implicit exit that was masking the effect.  We
go through zexit() at this point as it's a standard end-of-command-list
case, though whether that's actually necessary (running additional tests
mostly associated with interactive shells) I don't know.

It's also a bit confusing that the exit trap has already happened in
that point --- in execlist(), I think --- even though it's more sensibly
associated in this case with the code inside zexit() which will also run
it (and if it had done the exit status would have been overridden and we
wouldn't have seen this).  But I don't think there are any real world
consequences for this with this change.

I don't think an exit_pending can actually be picked up at that point,
but it's easier and safer to be consistent.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index e01e035..8dcdcc0 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5723,7 +5723,7 @@ int exit_val;
 void
 realexit(void)
 {
-    exit(exit_val ? exit_val : lastval);
+    exit((shell_exiting || exit_pending) ? exit_val : lastval);
 }
 
 /* As realexit(), but call _exit instead */
@@ -5732,7 +5732,7 @@ realexit(void)
 void
 _realexit(void)
 {
-    _exit(exit_val ? exit_val : lastval);
+    _exit((shell_exiting || exit_pending) ? exit_val : lastval);
 }
 
 /* exit the shell.  val is the return value of the shell.  *
diff --git a/Src/init.c b/Src/init.c
index 838c2c2..cec9143 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1358,7 +1358,7 @@ init_misc(char *cmd, char *zsh_name)
 	bshin = fdopen(SHIN, "r");
 	execstring(cmd, 0, 1, "cmdarg");
 	stopmsg = 1;
-	zexit(exit_val ? exit_val : lastval, 0);
+	zexit((exit_pending || shell_exiting) ? exit_val : lastval, 0);
     }
 
     if (interact && isset(RCS))
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index bab0b0a..57daf8d 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -872,6 +872,9 @@ F:Must be tested with a top-level script rather than source or function
   $ZTST_testdir/../Src/zsh -fc 'fn() { exit 13; }; trap fn EXIT'
 13:Explicit exit in exit trap overrides implicit exit status
 
+  $ZTST_testdir/../Src/zsh -fc 'fn() { exit 0; }; trap fn EXIT; false'
+0:Explicit exit status 0 in exit trap overrides implicit non-zero status
+
 %clean
 
   rm -f TRAPEXIT




^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-10-10  9:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20181009012624epcas1p44f2ae223f663713a980af4be735e5a3f@epcas1p4.samsung.com>
2018-10-08 13:02 ` [Bug] Exiting shell from function called by trap handler always produces status 0 Martijn Dekker
2018-10-09  8:49   ` Peter Stephenson
2018-10-09  9:58     ` Mikael Magnusson
2018-10-09 11:46     ` Martijn Dekker
2018-10-09 13:16       ` Peter Stephenson
2018-10-09 13:39         ` Daniel Shahaf
2018-10-09 13:43           ` Peter Stephenson
     [not found]           ` <1539092591.3286.12.camel@samsung.com>
2018-10-09 13:50             ` Peter Stephenson
2018-10-09 20:09         ` Peter Stephenson
2018-10-10  8:56           ` Daniel Shahaf
2018-10-10  9:31             ` Peter Stephenson

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).