Page MenuHomeFreeBSD

mountd(8): parsecred(): Remove "duplicate compression"
Needs ReviewPublic

Authored by olce on Fri, Oct 4, 8:10 AM.

Details

Reviewers
rmacklem
Summary

No functional change (intended).

This code dates back to 4.4BSD, became wrong after some getgrouplist()
(nssswitch-related) change in 2007, was fixed only in 2020 and since
then underwent cosmetic changes.

It is likely that in fact it never served any useful purpose in FreeBSD,
except perhaps at the very beginning. It's certainly not the case
today: NFS credentials are only used to check for file accesses, whose
group is checked against all groups of a credentials indiscriminately
(except for the real GID). Consequently, having a single duplicate,
which the code would actually remove only if in the first supplementary
group slot, doesn't change behavior. Moreover, saving up one slot is
not a concern either as NGROUPS_MAX (1023) supplementary groups can be
passed (and see also next commit, regaining one slot). And if it ever
becomes one, the proper change is to have mountd(8) use
sysconf(_SC_NGROUPS_MAX) (and the administrator should raise the value
of the 'kern.ngroups' tunable).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59731
Build 56617: arc lint + arc unit

Event Timeline

olce requested review of this revision.Fri, Oct 4, 8:10 AM

I will look at the patches later, but just in case you are not aware of
why a lot of this weirdness exists in the code...

Sun RPC has a gid plus a list of additional groups (MAX 16) on the wire.
Broken code existed for a long time, that did the following:

cr_gid = gid
copy groups list

Step two overwrote the first gid, since cr_gid was just cr_groups[0] for BSD

The workaround was to put the same gid# in gid and element 0 of the Sun RPC
on-the-wire gid list.

Are any of these broken systems still out there (several very old variants of *BSD)?
If they are, you can easily break interoperability if you take this "put the same gid
in gid and element 0 of the gid list on the wire".
--> The code that looks for and gets rid of the duplicate on the receiving side

probably doesn't matter.

I will look at the patches later, rick
ps: If the gid is not duplicated on-the-wire, then there are up to 17 groups in

the RPC request.
usr.sbin/mountd/mountd.c
3616

This should now be

gid_t groups[NGROUPS_MAX];
3648

As noted for the other patch, this should now be

ngroups = NGROUPS_MAX;

since there is no chance it will be reduced by one.

3653

You might want to put a check here for

if (groups[0] == groups[1])
     syslog(LOG_ERR, "Group[0] == Group[1], pleas email freebsd-fs@freebsd.org");

so that any case where this occurs gets logged?
(I might do this as a first patch and see if anyone
reports the log message before moving to this patch.)

But this is up to you.