>
> 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?
>