On Tue, Jun 25, 2019 at 04:55:10PM -0700, Michael Forney wrote:
> On 2019-06-25, Richard Ipsum <[email protected]> wrote:
> > On Tue, Jun 25, 2019 at 03:44:57PM -0700, Michael Forney wrote:
> >> On 2019-06-25, Richard Ipsum <[email protected]> wrote:
> >> > This fixes an error on OpenBSD,
> >> >
> >> > % chown 1000:1000 yes.c
> >> > ./chown: getgrnam 1000: No such file or directory
> >>
> >> POSIX says that if the entry cannot be found, errno shall not be
> >> changed. Are you saying that OpenBSD doesn't behave this way?
> >
> > Yeah when I run this on OpenBSD with uid:gid values I get ENOENT.
> 
> Do you think that's something they'd be willing to fix?
> 
> I think the error is coming from
> https://github.com/openbsd/src/blob/f84583fe5d7899ab1504dc1d58a04742db4a3058/lib/libc/gen/getgrent.c#L217.
> If I run `touch /var/run/ypbind.lock` in my OpenBSD VM, it fixes the
> issue.

I see, so to preserve the existing behaviour you'd want to save errno
before calling access, then restore afterward.

> 
> I think this is unintended, since there appears to be some logic to
> save and restore errno in grscan(). Looks like it was introduced in
> https://github.com/openbsd/src/commit/b971f1acd7c34a49359ccefbe512e06f3826a939
> (first released in OpenBSD 5.9).

Right, exactly. I think the reason this doesn't break OpenBSD chown is
because it uses `gid_from_group` and the implementation[1] for that
actually ignores errno, if it didn't they might have spotted this issue
themselves.

Do you want to raise this with upstream or shall I?

> 
> >>
[snip]
> >>
> >> It looks like there is no error with getpwnam, only with getgrname,
> >> right?
> >>
> >
> > Yes, but I would argue that realistically the only time this function errors
> > is
> > when the entry doesn't exist, we don't want to exit in that case,
> > instead try parsing it as a numeric uid and if that fails then exit.
> 
> The POSIX getgrnam specification also lists a couple other possible
> errors: I/O error and out of file descriptors.
> 
> Also, the chown specification specifically says
> 
>       If a numeric group operand exists in the group database as a group
> name, the group ID number associated with that group name shall be
> used as the group ID.
> 
> If we aren't able to read /etc/group for some reason and the the group
> name for a number exists, we would end up using the wrong group ID.
> 

Yes I can see why you would rather not ignore errno to be fair.
Maybe the better option is to see if the fix can be made in OpenBSD
and then I could rework these patches?

Thanks,
Richard

[1]: https://github.com/openbsd/src/blob/master/lib/libc/gen/pwcache.c#L384 

Reply via email to