errexit and (Z)ERR trap regression

errexit and (Z)ERR trap regression

[Martijn Dekker]

Two manifestations of the same bug in the current dev version:

     $ zsh -e -c 'true && false; echo NOT REACHED'         # no output; correct
     $ zsh -c 'true && (set -e; false; echo NOT REACHED)'  # incorrect output
     NOT REACHED

and

     $ zsh -c 'trap "echo Trapped!" ERR; true && false'     # correct output
     Trapped!
     $ zsh -c 'true && (trap "print Trapped!" ERR; false)'  # no output; bug

The -e option and the (Z)ERR trap should be disabled for the left hand side of 
&&/|| but not for the right hand side (or, as POSIX puts it, set -e should be 
disabled for "any command of an AND-OR list other than the last"). zsh 5.8.x 
works correctly.

A 'git bisect' revealed that this bug was introduced in:

commit 259f1e944b96715fda25f7ba227da05bdb7e600f
Author: Philippe Altherr <philippe.altherr@gmail.com>
Date:   Sat Dec 3 21:03:36 2022 -0800

     51071: fix ERR_RETURN for functions in conditional statements

-- 
||	modernish -- harness the shell
||	https://github.com/modernish/modernish
||
||	KornShell lives!
||	https://github.com/ksh93/ksh

Re: errexit and (Z)ERR trap regression

[Bart Schaefer]

On Thu, Jun 20, 2024 at 10:12 AM Martijn Dekker <martijn@inlv.org> wrote:
>
>      $ zsh -c 'true && (set -e; false; echo NOT REACHED)'  # incorrect output
>      $ zsh -c 'true && (trap "print Trapped!" ERR; false)'  # no output; bug

These two examples can be fixed by resetting the "noerrs"-related
flags on entry to the subshell, although I'm not sure that's correct
when the subshell appears earlier in the chain.  Of course even then,
exit from the subshell won't be treated as anything other than part of
the boolean condition in the parent.

However, given this --

> disabled for "any command of an AND-OR list other than the last"

-- the question is whether the behavior should be the same when using
brace commands instead of subshells:

% zsh -c 'true && { set -e; false; echo NOT REACHED }'
% zsh -c 'true && { trap "print Trapped!" ERR; false }'

Certainly we don't want to reset the flag even on the last command
when (for example) the AND-OR list appears in the condition part of an
"if" or similar construct.

Is my recollection incorrect, that there's a change related to this in
the as-yet-unpublished next revision of POSIX?

Re: errexit and (Z)ERR trap regression

[Lawrence Velázquez]

On Fri, Jun 21, 2024, at 2:46 PM, Bart Schaefer wrote:
> Is my recollection incorrect, that there's a change related to this in
> the as-yet-unpublished next revision of POSIX?

There is a change, but it doesn't seem to be relevant here:
https://www.austingroupbugs.net/view.php?id=1150#c4184

The full description of "set -e" in the fresh-out-of-the-oven
POSIX.1-2024 is:

-e	When this option is on, when any command fails (for any of
	the reasons listed in Section 2.8.1 or by returning an exit
	status greater than zero), the shell immediately shall exit,
	as if by executing the `exit' special built-in utility with
	no arguments, with the following exceptions:

	1.	The failure of any individual command in a multi-command
		pipeline, or of any subshell environments in which
		command substitution was performed during word
		expansion, shall not cause the shell to exit.  Only
		the failure of the pipeline itself shall be considered.

	2.	The `-e' setting shall be ignored when executing
		the compound list following the `while', `until',
		`if', or `elif' reserved word, a pipeline beginning
		with the `!' reserved word, or any command of an
		AND-OR list other than the last.

	3.	If the exit status of a compound command other than
		a subshell command was the result of a failure while
		`-e' was being ignored, then `-e' shall not apply
		to this command.

	This requirement applies to the shell environment and each
	subshell environment separately.  For example, in:

		set -e; (false; echo one) | cat; echo two

	the `false' command causes the subshell to exit without
	executing `echo one'; however, `echo two' is executed because
	the exit status of the pipeline `(false; echo one) | cat'
	is zero.

	In

		set -e; echo $(false; echo one) two

	the `false' command causes the subshell in which the command
	substitution is performed to exit without executing `echo
	one'; the exit status of the subshell is ignored and the
	shell then executes the word-expanded command `echo two'.

-- 
vq

Re: errexit and (Z)ERR trap regression

[Bart Schaefer]

On Fri, Jun 21, 2024 at 1:03 PM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> The full description of "set -e" in the fresh-out-of-the-oven
> POSIX.1-2024 is:
>
>         This requirement applies to the shell environment and each
>         subshell environment separately.  For example, in:
>
>                 set -e; (false; echo one) | cat; echo two
>
>         the `false' command causes the subshell to exit without
>         executing `echo one'

Hm, that doesn't directly address something like

  set -e; true && (false; echo one) || echo two

If the subshell should execute "echo one" then the parent should not "echo two".

Unfortunately I don't follow how any of the POSIX.1-2024 text is meant
to apply to

   set -e; true && { false; echo one; } || echo two

Is "false" there considered to be "any command of an AND-OR list" ?

Re: errexit and (Z)ERR trap regression

[Lawrence Velázquez]

On Fri, Jun 21, 2024, at 4:36 PM, Bart Schaefer wrote:
> On Fri, Jun 21, 2024 at 1:03 PM Lawrence Velázquez <larryv@zsh.org> wrote:
>>
>> The full description of "set -e" in the fresh-out-of-the-oven
>> POSIX.1-2024 is:
>>
>>         This requirement applies to the shell environment and each
>>         subshell environment separately.  For example, in:
>>
>>                 set -e; (false; echo one) | cat; echo two
>>
>>         the `false' command causes the subshell to exit without
>>         executing `echo one'
>
> Hm, that doesn't directly address something like
>
>   set -e; true && (false; echo one) || echo two
>
> If the subshell should execute "echo one" then the parent should not "echo two".

Right, because early exit is suppressed in the non-final portions
of the AND-OR list and is not reenabled in the subshell.  All of
bash, dash, ksh, mksh, yash, and the Solaris 10 Bourne shell print
"one" here.

> Unfortunately I don't follow how any of the POSIX.1-2024 text is meant
> to apply to
>
>    set -e; true && { false; echo one; } || echo two
>
> Is "false" there considered to be "any command of an AND-OR list" ?

This ought to work the same as the subshell version: early exit is
suppressed for the commands "true" and "{ false; echo one; }", so
the overall output is still "one".  The aforementioned shells behave
this way.

-- 
vq

Re: errexit and (Z)ERR trap regression

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Fri, Jun 21, 2024 at 10:50 PM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> >    set -e; true && { false; echo one; } || echo two
> >
> > Is "false" there considered to be "any command of an AND-OR list" ?
>
> This ought to work the same as the subshell version: early exit is
> suppressed for the commands "true" and "{ false; echo one; }", so
> the overall output is still "one".

The following seems to fix all six of the mentioned cases from this
thread, then.

[-- Attachment #2: noerrexit-sublist-last.txt --]
[-- Type: text/plain, Size: 1244 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index e955e85df..3f0afa3d5 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1486,6 +1486,8 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    /* suppress errexit for commands before && and || and after ! */
 	    if (isandor || isnot)
 		noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;
+	    else if (!oldnoerrexit)
+		noerrexit = 0;
 	    switch (WC_SUBLIST_TYPE(code)) {
 	    case WC_SUBLIST_END:
 		/* End of sublist; just execute, ignoring status. */
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index de57765a0..47a15bf7f 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -995,6 +995,18 @@ F:Must be tested with a top-level script rather than source or function
 ?loop 0
 ?loop 1
 
+  ( true && (set -e; false; echo NOT REACHED) )
+  ( true && (trap "print Trapped!" ERR; false) )
+  ( true && { set -e; false; echo NOT REACHED } )
+  ( true && { trap "print Trapped!" ERR; false } )
+  ( set -e; true && (false; echo one) || echo two )
+  ( set -e; true && { false; echo one; } || echo two )
+0:ERR_EXIT is triggered by last command in an AND-OR list
+>Trapped!
+>Trapped!
+>one
+>one
+
   if zmodload zsh/system 2>/dev/null; then
   (
     trap 'echo TERM; exit 2' TERM

Re: errexit and (Z)ERR trap regression

[Philippe Altherr]

[-- Attachment #1.1: Type: text/plain, Size: 2060 bytes --]

Note that just adding braces is enough to trigger the bug:

     $ zsh -e -c 'true && false; echo NOT REACHED'     # no output; correct
     $ zsh -e -c 'true && {false; echo NOT REACHED}'  # incorrect output
     NOT REACHED

and

     $ zsh -c 'trap "echo Trapped!" ERR; true && false'     # correct output
     Trapped!
     $ zsh -c 'trap "echo Trapped!" ERR; true && {false}'  # no output; bug

I think that the correct fix is the following:

    if (isandor || isnot)
        noerrexit = oldnoerrexit | NOERREXIT_EXIT | NOERREXIT_RETURN;
    else
        noerrexit = oldnoerrexit;

For reminder, here is the current code:

    if (isandor || isnot)
        noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;


In other words, every sub-command in a list should only depend on the
noerrexit that was in effect at the start of the evaluation of the list. It
should in no way depend on the noerrexit that results from the evaluation
of proceeding sub-commands as is currently the case. The current code only
works for lists where the last sub-command is a simple command, thanks to
the "noerrexit = oldnoerrexit;" performed after the last sub-command just
before checking whether an ERREXIT should be triggered.

Here are two other examples fixed by this patch:

    zsh -c 'trap "echo Trapped!" ERR; true && if true; then false; fi'
    zsh -c 'trap "echo Trapped!" ERR; true && {false} always {true}'

Philippe




On Tue, Jun 25, 2024 at 1:03 AM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Fri, Jun 21, 2024 at 10:50 PM Lawrence Velázquez <larryv@zsh.org>
> wrote:
> >
> > >    set -e; true && { false; echo one; } || echo two
> > >
> > > Is "false" there considered to be "any command of an AND-OR list" ?
> >
> > This ought to work the same as the subshell version: early exit is
> > suppressed for the commands "true" and "{ false; echo one; }", so
> > the overall output is still "one".
>
> The following seems to fix all six of the mentioned cases from this
> thread, then.
>

[-- Attachment #1.2: Type: text/html, Size: 3022 bytes --]

[-- Attachment #2: noerrexit-list-subcommands.txt --]
[-- Type: text/plain, Size: 1611 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index e955e85df..916a3567d 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1485,7 +1485,9 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    next = state->pc + WC_SUBLIST_SKIP(code);
 	    /* suppress errexit for commands before && and || and after ! */
 	    if (isandor || isnot)
-		noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;
+		noerrexit = oldnoerrexit | NOERREXIT_EXIT | NOERREXIT_RETURN;
+	    else
+		noerrexit = oldnoerrexit;
 	    switch (WC_SUBLIST_TYPE(code)) {
 	    case WC_SUBLIST_END:
 		/* End of sublist; just execute, ignoring status. */
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index de57765a0..9fefdf713 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -995,6 +995,25 @@ F:Must be tested with a top-level script rather than source or function
 ?loop 0
 ?loop 1
 
+  ( set -e; true && {false; echo NOT REACHED} )
+  ( trap "print Trapped!" ERR; true && {false} )
+  ( trap "print Trapped!" ERR; true && if true; then false; fi )
+  ( trap "print Trapped!" ERR; true && {false} always {true} )
+  ( true && (set -e; false; echo NOT REACHED) )
+  ( true && (trap "print Trapped!" ERR; false) )
+  ( true && { set -e; false; echo NOT REACHED } )
+  ( true && { trap "print Trapped!" ERR; false } )
+  ( set -e; true && (false; echo one) || echo two )
+  ( set -e; true && { false; echo one; } || echo two )
+0:ERR_EXIT is triggered by last command in an AND-OR list
+>Trapped!
+>Trapped!
+>Trapped!
+>Trapped!
+>Trapped!
+>one
+>one
+
   if zmodload zsh/system 2>/dev/null; then
   (
     trap 'echo TERM; exit 2' TERM

Re: errexit and (Z)ERR trap regression

[Bart Schaefer]

On Wed, Jun 26, 2024 at 5:43 AM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> I think that the correct fix is the following:
>
>     if (isandor || isnot)
>         noerrexit = oldnoerrexit | NOERREXIT_EXIT | NOERREXIT_RETURN;
>     else
>         noerrexit = oldnoerrexit;

This doesn't seem necessary to me.  For one thing, it's equivalent to:

  noerrexit = oldnoerrexit;
  if (isandor || isnot)
    noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;

But assigning (noerrexit = oldnoerrexit) presumes that noerrexit was
(improperly?) cleared at or after the point where olderrexit was
recorded.  If that were the situation, then --

> For reminder, here is the current code:
>
>     if (isandor || isnot)
>         noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;

-- would be insufficient for the existing test cases, I think.  That
is, either (olderrexit == noerrexit), or it's not necessary to
OR-together olderrexit with the new values.

> Here are two other examples fixed by this patch:
>
>     zsh -c 'trap "echo Trapped!" ERR; true && if true; then false; fi'
>     zsh -c 'trap "echo Trapped!" ERR; true && {false} always {true}'

These two cases also pass with my patch from workers/52973.  Do you
have an example where my patch doesn't work?

Re: errexit and (Z)ERR trap regression

[Philippe Altherr]

[-- Attachment #1.1: Type: text/plain, Size: 4937 bytes --]

>
> This doesn't seem necessary to me.  For one thing, it's equivalent to:


>   noerrexit = oldnoerrexit;
>   if (isandor || isnot)
>     noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;


I agree that it's equivalent but I disagree that it (or something similar)
isn't necessary ;-)

But assigning (noerrexit = oldnoerrexit) presumes that noerrexit was
> (improperly?) cleared at or after the point where olderrexit was
> recorded.


Which is indeed the problem. To see it, let's take a wider view of the code
at hand:

    int oldnoerrexit = noerrexit;
    // ...
    while (wc_code(code) == WC_SUBLIST) {
        // ...
*        noerrexit = oldnoerrexit;*
*        if (isandor || isnot)*
*            noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;*
        switch (WC_SUBLIST_TYPE(code)) {
            case WC_SUBLIST_END: // ...
            case WC_SUBLIST_AND: // ...
            case WC_SUBLIST_OR: // ...
        }
        // ...
    }
    sublist_done:
    noerrexit = oldnoerrexit;

It's true that in the first iteration of the while loop, the noerrexit =
oldnoerrexit is always a nop. However, that isn't the case for subsequent
iterations and in particular for the last iteration where it's important to
always restore noerrexit to its original value (i.e., oldnoerrexit) even if
that value isn't equal to 0.

Now that I see this code under this angle, I think that a better version is
one where noerrexit = oldnoerrexit is placed after the switch statement.
This way it's easier to grasp why it's necessary (or why it's not a nop).
Ideally we would then also drop the noerrexit = oldnoerrexit after the
sublist_done label. Unfortunately, that's not possible because the switch
statement contains multiple goto statements that jump to the label and that
bypass any noerrexit = oldnoerrexit after the switch statement.

> Here are two other examples fixed by this patch:
> >
> >     zsh -c 'trap "echo Trapped!" ERR; true && if true; then false; fi'
> >     zsh -c 'trap "echo Trapped!" ERR; true && {false} always {true}'


> These two cases also pass with my patch from workers/52973.  Do you
> have an example where my patch doesn't work?


Yes, these two cases were just meant to highlight slightly different code
paths that also trigger the bug. And yes, I managed to build an example
that fails with your patch. I think that the following is a minimal
example, i.e., both && and the braces around the false are needed to
trigger the bug:

zsh -c 'set -o ERR_RETURN; f() { true && { false }; echo "NOT REACHED";  };
f && true'

In this example "f" in "f && true" is evaluated with
"errnoexit = NOERREXIT_EXIT | NOERREXIT_RETURN". The call to "f" clears
the NOERREXIT_RETURN bit. Thus "true && { false }" is evaluated
with "errnoexit = NOERREXIT_EXIT" and "olderrnoexit" is initialized to that
value in "execlist". The evaluation of "true" sets "errnoexit" to
"NOERREXIT_EXIT | NOERREXIT_RETURN". Now, if "errnoexit" is left unchanged,
as is the case in version 5.9, or set to 0 only if "olderrnoexit" is equal
to 0, then "{ false }" is evaluated with "errnoexit = NOERREXIT_EXIT |
NOERREXIT_RETURN" and fails to trigger ERR_RETURN. With my path "errnoexit"
is restored to "NOERREXIT_EXIT", which enables "{ false }" to trigger
ERR_RETURN.

Below is an updated patch with the cleaner and simpler fix described above
and a new test case for the ERR_RETURN example.

Philippe


On Wed, Jun 26, 2024 at 11:43 PM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Wed, Jun 26, 2024 at 5:43 AM Philippe Altherr
> <philippe.altherr@gmail.com> wrote:
> >
> > I think that the correct fix is the following:
> >
> >     if (isandor || isnot)
> >         noerrexit = oldnoerrexit | NOERREXIT_EXIT | NOERREXIT_RETURN;
> >     else
> >         noerrexit = oldnoerrexit;
>
> This doesn't seem necessary to me.  For one thing, it's equivalent to:
>
>   noerrexit = oldnoerrexit;
>   if (isandor || isnot)
>     noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;
>
> But assigning (noerrexit = oldnoerrexit) presumes that noerrexit was
> (improperly?) cleared at or after the point where olderrexit was
> recorded.  If that were the situation, then --
>
> > For reminder, here is the current code:
> >
> >     if (isandor || isnot)
> >         noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN;
>
> -- would be insufficient for the existing test cases, I think.  That
> is, either (olderrexit == noerrexit), or it's not necessary to
> OR-together olderrexit with the new values.
>
> > Here are two other examples fixed by this patch:
> >
> >     zsh -c 'trap "echo Trapped!" ERR; true && if true; then false; fi'
> >     zsh -c 'trap "echo Trapped!" ERR; true && {false} always {true}'
>
> These two cases also pass with my patch from workers/52973.  Do you
> have an example where my patch doesn't work?
>

[-- Attachment #1.2: Type: text/html, Size: 7320 bytes --]

[-- Attachment #2: noerrexit-list-subcommands.txt --]
[-- Type: text/plain, Size: 1658 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index e955e85df..a473938ec 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1568,6 +1568,7 @@ execlist(Estate state, int dont_change_job, int exiting)
 	    }
 	    state->pc = next;
 	    code = *state->pc++;
+	    noerrexit = oldnoerrexit;
 	}
 	state->pc--;
 sublist_done:
diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst
index de57765a0..87b7fd1f7 100644
--- a/Test/C03traps.ztst
+++ b/Test/C03traps.ztst
@@ -995,6 +995,33 @@ F:Must be tested with a top-level script rather than source or function
 ?loop 0
 ?loop 1
 
+  ( set -e; true && {false; echo NOT REACHED} )
+  ( trap "print Trapped!" ERR; true && {false} )
+  ( trap "print Trapped!" ERR; true && if true; then false; fi )
+  ( trap "print Trapped!" ERR; true && {false} always {true} )
+  ( true && (set -e; false; echo NOT REACHED) )
+  ( true && (trap "print Trapped!" ERR; false) )
+  ( true && { set -e; false; echo NOT REACHED } )
+  ( true && { trap "print Trapped!" ERR; false } )
+  ( set -e; true && (false; echo one) || echo two )
+  ( set -e; true && { false; echo one; } || echo two )
+0:ERR_EXIT is triggered by last command in an AND-OR list
+>Trapped!
+>Trapped!
+>Trapped!
+>Trapped!
+>Trapped!
+>one
+>one
+
+  ( set -o ERR_RETURN; f() { false; echo NOT REACHED;  }; f || true; echo OK )
+  ( set -o ERR_RETURN; f() { true && false; echo NOT REACHED;  }; f || true; echo OK )
+  ( set -o ERR_RETURN; f() { true && { false }; echo NOT REACHED;  }; f || true; echo OK )
+0:ERR_RETURN is triggered in function calls on the left of an AND-OR
+>OK
+>OK
+>OK
+
   if zmodload zsh/system 2>/dev/null; then
   (
     trap 'echo TERM; exit 2' TERM

Re: errexit and (Z)ERR trap regression

[Bart Schaefer]

On Wed, Jun 26, 2024 at 7:01 PM Philippe Altherr
<philippe.altherr@gmail.com> wrote:
>
> It's true that in the first iteration of the while loop, the noerrexit = oldnoerrexit is always a nop. However, that isn't the case for subsequent iterations and in particular for the last iteration where it's important to always restore noerrexit to its original value (i.e., oldnoerrexit) even if that value isn't equal to 0.
>
> Now that I see this code under this angle, I think that a better version is one where noerrexit = oldnoerrexit is placed after the switch statement.

Ok, this makes much more sense to me than the previous patch.  Thanks
for digging in.

Re: errexit and (Z)ERR trap regression

[Philippe Altherr]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

>
> Ok, this makes much more sense to me than the previous patch.  Thanks
> for digging in.


To me too :-) It's definitely cleaner and easier to follow.

Philippe


On Thu, Jun 27, 2024 at 6:44 PM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Wed, Jun 26, 2024 at 7:01 PM Philippe Altherr
> <philippe.altherr@gmail.com> wrote:
> >
> > It's true that in the first iteration of the while loop, the noerrexit =
> oldnoerrexit is always a nop. However, that isn't the case for subsequent
> iterations and in particular for the last iteration where it's important to
> always restore noerrexit to its original value (i.e., oldnoerrexit) even if
> that value isn't equal to 0.
> >
> > Now that I see this code under this angle, I think that a better version
> is one where noerrexit = oldnoerrexit is placed after the switch statement.
>
> Ok, this makes much more sense to me than the previous patch.  Thanks
> for digging in.
>
>

[-- Attachment #2: Type: text/html, Size: 1542 bytes --]