Page MenuHomeFreeBSD

Eliminate unnecessary loop in _cap_check()
ClosedPublic

Authored by ryan_freqlabs.com on Jul 30 2016, 7:49 AM.

Details

Summary

Calling cap_rights_contains() several times with the same inputs is not going to produce a different output.

For context, here is the definition of cap_rights_contains() from sys/kern/subr_capability.c:

bool
cap_rights_contains(const cap_rights_t *big, const cap_rights_t *little)
{
        unsigned int i, n;

        assert(CAPVER(big) == CAP_RIGHTS_VERSION_00);
        assert(CAPVER(little) == CAP_RIGHTS_VERSION_00);
        assert(CAPVER(big) == CAPVER(little));

        n = CAPARSIZE(big);
        assert(n >= CAPARSIZE_MIN && n <= CAPARSIZE_MAX);

        for (i = 0; i < n; i++) {
                if ((big->cr_rights[i] & little->cr_rights[i]) !=
                    little->cr_rights[i]) {
                        return (false);
                }
        }

        return (true);
}
Test Plan

Test early. Test often.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ryan_freqlabs.com retitled this revision from to Eliminate unnecessary loop in _cap_check().
ryan_freqlabs.com updated this object.
ryan_freqlabs.com edited the test plan for this revision. (Show Details)
ryan_freqlabs.com set the repository for this revision to rS FreeBSD src repository.
allanjude added a reviewer: ed.
ed accepted this revision.Jul 31 2016, 6:34 AM
ed edited edge metadata.

Interesting. I suspect an older version of this code hand-rolled cap_rights_contains() and was incorrectly refactored?

This revision is now accepted and ready to land.Jul 31 2016, 6:34 AM
oshogbo accepted this revision.Jul 31 2016, 3:56 PM
oshogbo edited edge metadata.
In D7369#153325, @ed wrote:

Interesting. I suspect an older version of this code hand-rolled cap_rights_contains() and was incorrectly refactored?

It sure looks that way, doesn't it. I dug through the logs to try to pinpoint where the refactor happened, but r255219 introduces the loop just like that. Lost artifact of development, I guess.

ed removed a reviewer: ed.Aug 10 2016, 9:27 PM
ryan_freqlabs.com edited edge metadata.

The variable i is not necessary, either.

This revision now requires review to proceed.Aug 30 2016, 1:48 AM
allanjude accepted this revision.Aug 31 2016, 5:51 PM
allanjude added a reviewer: allanjude.
This revision is now accepted and ready to land.Aug 31 2016, 5:51 PM
This revision was automatically updated to reflect the committed changes.