Page MenuHomeFreeBSD

chroot: slightly cleanup
ClosedPublic

Authored by kevans on Fri, Jul 25, 5:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Aug 8, 6:14 PM
Unknown Object (File)
Sun, Aug 3, 4:26 AM
Unknown Object (File)
Tue, Jul 29, 12:27 AM
Unknown Object (File)
Tue, Jul 29, 12:20 AM
Unknown Object (File)
Tue, Jul 29, 12:20 AM
Unknown Object (File)
Mon, Jul 28, 9:21 PM
Unknown Object (File)
Mon, Jul 28, 5:49 PM
Unknown Object (File)
Mon, Jul 28, 2:35 PM
Subscribers

Details

Summary

Highlights:

  • Pull resolve_user() and resolve_group() out to make the main flow a bit easier to read
  • Fix some edge-cases in user/group resolution: you can have fully numeric usernames, and they may or may not live within the valid ID range. Switch to just trying to resolve every specified group/user as a name, first, with a fallback to converting it to a numeric type and trying to resolve it as an ID.
  • Constify locals in main() that don't need to be mutable, re-sort

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

usr.sbin/chroot/chroot.c
50–51

Why not have resolve_group() and resolve_user() just return the found ID as the return value, given that not finding one leads to exiting right away through errx()?

55–56

I'm all for this change!

This is what we are doing in several other utilities, and we really should do that since for some utilities like chown(8), this is explicitly prescribed by POSIX.

Actually, we should probably have some common utility function for that (not as part of this review).

usr.sbin/chroot/chroot.c
50–51

Whoops; originally I had this return an error (which I didn't want to try to do through a gid_t/uid_t because a lot of our utilities get ported to platform(s) that have signed [gu]id_t types, and (uid_t)-1 is a valid user), but then realized it was silly to not just have them always-succeed or abort. Will fix.

usr.sbin/chroot/chroot.c
50–51

No worries. This isn't a bug (nothing functional), just some slight quality-of-life improvement (eases reviewing).

Return uid_t/gid_t since they cannot fail

usr.sbin/chroot/chroot.c
63

Forgot this. Calling getgrgid() wasn't present initially, and POSIX's text for chown(8) arguably can be interpreted as saying that any numerical ID is supposed to be accepted as is.

Same in resolve_user().

usr.sbin/chroot/chroot.c
63

Hmm, I agree, with the caveat that we also need to range-check it against [UG]ID_MAX if we're not doing a round-trip through the database.

Looking at the spec again and the platform I'm aware of with a signed type, it occurs to me that the assigned negative groups are nobody and nogroup, which make sense to be out of the technically-valid range (which POSIX explicitly describes as positive values)

usr.sbin/chroot/chroot.c
63

You're right. This has the unfortunate consequence that some implementation's choice to make nobody/nogroup negative (and thus uid_t/gid_t signed) determines whether this user/group can be accepted, introducing non-portability. This is arguably a flaw in POSIX.

Accept any numerical uid/gid within valid range

This notably means only accepting positive values on systwms that have a
signed [ug]id_t type. We contibue to try and resolve as a nane first, just.
in case the system has numeric usernames.

This revision is now accepted and ready to land.Thu, Jul 31, 2:35 PM
This revision was automatically updated to reflect the committed changes.