[BUG] Issue with set built-in in 5.8 (?)

[BUG] Issue with set built-in in 5.8 (?)

[dana]

I think there is arguably a regression in the way the set built-in works in
5.8, or at least a change in behaviour that we might consider further. The
implications hadn't occurred to me until just now, sorry :/

Unlike setopt, when set gets an error back from dosetopt(), it aborts the
shell. Before 5.8, the only errors you were likely to see in the real world
were usage-related ones (e.g., `set -b` or `set -o fakeoption`). It *was*
possible to get an error if setuid() or setgid() failed, but this should be
very rare.

Now, due to the extra error checks we do, it's much more likely that
dosetopt() can return non-zero when unsetting PRIVILEGED: the functions might
not be available, the user might not have permission to do initgroups(), the
sanity checks at the end might find that we're able to restore privileges, &c.

Most of these errors are useful, but i'm not sure they should unconditionally
abort the shell.

Possibilities:

1. We could reconsider which errors should be reported and/or make dosetopt()
   return non-zero. The main one i have doubts about is the initgroups()
   permission one. It is expected that unprivileged users can't update their
   supplementary groups, so warning the user about this condition, let alone
   treating it as an error, may be excessive

2. We could do something like have dosetopt() return <0 for halting errors and
   >0 for ones non-halting, and have bin_set() handle accordingly

3. I'm over-thinking it

@Daniel, we talked about the first one before, but this particular concern
didn't come up at the time — what do you reckon?

dana


% sudo perl -e '$< = 1; $> = 2; exec("zsh", "-fc", "id; unsetopt privileged; echo still here");'
uid=1(daemon) gid=1(daemon) euid=2 egid=0(wheel) groups=...
zsh:unsetopt:1: PRIVILEGED: supplementary group list not changed due to lack of permissions: EUID=2
zsh:unsetopt:1: can't change option: privileged
still here

% sudo perl -e '$< = 1; $> = 2; exec("zsh", "-fc", "id; set +p; echo still here");'
uid=1(daemon) gid=1(daemon) euid=2 egid=0(wheel) groups=...
zsh:unsetopt:1: PRIVILEGED: supplementary group list not changed due to lack of permissions: EUID=2
zsh:set:1: can't change option: -p

Re: [BUG] Issue with set built-in in 5.8 (?)

[Daniel Shahaf]

dana wrote on Sun, 16 Feb 2020 20:19 -0600:
> Most of these errors are useful, but i'm not sure they should unconditionally
> abort the shell.
⋮
> @Daniel, we talked about the first one before, but this particular concern
> didn't come up at the time — what do you reckon?

Considerations:

- Compatibility with what zshoptions(1) has promised would work, though
  that didn't work before 5.8.

- Consistency with POSIX.  (POSIX doesn't specify -p, but still.)

- Consistency with other shells.

- Consistency between «set» and «setopt».

- On the one hand, "Errors should never pass silently".  On the other
  hand, in the shell language there are few other cases of aborting the
  shell just because a syscall returned an error.

- Regardless of what we choose, the other behaviour is achievable: if
  we make the error fatal people can use «eval» to make it non-fatal,
  and if we make the error non-fatal people can use «… || exit 1» to
  make it fatal.

I'm not sure what these add up to, but these are the addends I have
in mind.

Cheers,

Daniel


> % sudo perl -e '$< = 1; $> = 2; exec("zsh", "-fc", "id; unsetopt privileged; echo still here");'
> uid=1(daemon) gid=1(daemon) euid=2 egid=0(wheel) groups=...
> zsh:unsetopt:1: PRIVILEGED: supplementary group list not changed due to lack of permissions: EUID=2
> zsh:unsetopt:1: can't change option: privileged
> still here
> 
> % sudo perl -e '$< = 1; $> = 2; exec("zsh", "-fc", "id; set +p; echo still here");'
> uid=1(daemon) gid=1(daemon) euid=2 egid=0(wheel) groups=...
> zsh:unsetopt:1: PRIVILEGED: supplementary group list not changed due to lack of permissions: EUID=2
> zsh:set:1: can't change option: -p
> 

Re: [BUG] Issue with set built-in in 5.8 (?)

[dana]

On 17 Feb 2020, at 03:02, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> - Compatibility with what zshoptions(1) has promised would work, though
>   that didn't work before 5.8.

zshoptions(1) doesn't say anything about set aborting the shell, AFAIK. Only
zshbuiltins(1) does (under setopt, strangely), and it only states that a 'bad
option name' does it — nothing about any implicit errors that might occur when
changing a valid option.

On 17 Feb 2020, at 03:02, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> - Consistency with POSIX.  (POSIX doesn't specify -p, but still.)
> - Consistency with other shells.

I can't even find where in POSIX it says that set should abort the shell. dash
and ksh93 do work that way, but bash and yash don't.

On 17 Feb 2020, at 03:02, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> - Regardless of what we choose, the other behaviour is achievable: if
>   we make the error fatal people can use «eval» to make it non-fatal,
>   and if we make the error non-fatal people can use «… || exit 1» to
>   make it fatal.

Of the two, the latter seems more intuitive

dana

Re: [BUG] Issue with set built-in in 5.8 (?)

[Peter Stephenson]

On Tue, 2020-02-18 at 14:01 -0600, dana wrote:
> On 17 Feb 2020, at 03:02, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > 
> > - Regardless of what we choose, the other behaviour is achievable: if
> >   we make the error fatal people can use «eval» to make it non-fatal,
> >   and if we make the error non-fatal people can use «… || exit 1» to
> >   make it fatal.
> Of the two, the latter seems more intuitive

I would say so, too.  We don't have *that* many fatal errors for settings,
I don't think.

Tracing through POSIX to get an exact answer can be a bit of a pain;
some wording somewhere implies some particular case at another
point not explicitly cross-referenced etc. etc.

pws

Re: [BUG] Issue with set built-in in 5.8 (?)

[dana]

On 19 Feb 2020, at 03:37, Peter Stephenson <p.stephenson@samsung.com> wrote:
> Tracing through POSIX to get an exact answer can be a bit of a pain

I found it:

  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_01

When encountering a 'special built-in utility error', a non-interactive shell
'shall exit'. The documentation for the set built-in doesn't seem to
anticipate any errors besides those related to option-parsing, but i guess the
'letter of the law' is clear; if we were going to follow it strictly, we'd
leave set the way it is.

idk. On balance, maybe we should just let it be until someone complains (which
probably won't happen). Otherwise, with all of the weird variables in this
code path, we could be tinkering with it until next February...

dana

Re: [BUG] Issue with set built-in in 5.8 (?)

[Peter Stephenson]

On Wed, 2020-02-19 at 13:25 -0600, dana wrote:
> On 19 Feb 2020, at 03:37, Peter Stephenson <p.stephenson@samsung.com> wrote:
> > 
> > Tracing through POSIX to get an exact answer can be a bit of a pain
> I found it:
> 
>   https://protect2.fireeye.com/url?k=ce2973a8-93fdceec-ce28f8e7-0cc47a31381a-9cb7538620366aa8&u=https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_01
> 
> When encountering a 'special built-in utility error', a non-interactive shell
> 'shall exit'. The documentation for the set built-in doesn't seem to
> anticipate any errors besides those related to option-parsing, but i guess the
> 'letter of the law' is clear; if we were going to follow it strictly, we'd
> leave set the way it is.
> 
> idk. On balance, maybe we should just let it be until someone complains (which
> probably won't happen). Otherwise, with all of the weird variables in this
> code path, we could be tinkering with it until next February...

If this is behaviour of set that's not itself covered by POSIX, it's
definitely not so clear, and yes, I agree in practice we very likely
get away with it.

We have the possibility of covering variant behaviour with the
POSIX_BUILTINS option if it's important enough.

pws