Page MenuHomeFreeBSD

crsetgroups(): Improve and factor out groups normalization
Needs ReviewPublic

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

Details

Reviewers
mhorne
mjg
emaste
Summary

The groups array has been sorted (not including the first element, which
is always the effective GID) to enable performing a binary search for
determining if some group is part of the supplementary groups set.

Factor out this sorting operation into an internal normalization
function (groups_normalize()), adding to it the removal of duplicates
after the sort.

Separating groups normalization code allows to perform it in advance,
and in particular before calling MAC hooks which need the groups array
to be sorted to perform better. This also enables sorting input arrays
ahead of acquiring the process lock (which is not necessary for this
operation).

kern_setgroups() has been changed accordingly, so MAC modules
implementing the mac_cred_check_setgroups() hook now can assume
a normalized groups array (and also that it has at least one element, as
if kern_setgroups() is passed no groups, the hook is called with an
array of one element being the current effective GID, as this is
effectively the effect of such a call to kern_setgroups()). Further
commits introducing the setcred() system call and associated MAC hooks
will also guarantee a normalized groups array to MAC modules
implementing these hooks.

Rename crsetgroups_locked() into crsetgroups_internal(), as it is no
more "locked" than crsetgroups() itself. However, it can be called
under any lock (as needed), whereas the second may sleep to allocate
memory. Update their herald comments to make that explicit.

In passing, using qsort() instead of the old open-coded insertion sort
(in crsetgroups_locked()) fixes the performance concern about the latter
when using a large number of groups. Also, our qsort() falls back to
insertion sort for small arrays and in case the array is likely to be
mostly sorted, so this shouldn't cause concerns for the small number of
groups common case.

While here, add assertions in inner modification routines to check that
the passed credentials object has a reference count of exactly 1 (in
particular, it must not be shared). Remove a redundant one from some
outer routine.

Diff Detail

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