On Sun, Jun 24, 2007 at 08:43:30PM +0100, David Laight wrote:
> On Sun, Jun 24, 2007 at 11:30:10AM -0700, Bill Stouder-Studenmund wrote:
> >
> > Setting groups:
>
> How about:
I think this is a good step in the right direction. I like it.
> @@ -131,6 +131,28 @@ kauth_cred_alloc(void)
> return (cred);
> }
>
> +kauth_cred_t
> +kauth_cred_alloc1(int ngroups, gid_t **grbuf)
My instinct is to use a different name, but I'll be honest I've started to
drink the Objective C naming kool-aid. "kauth_cred_alloc_with_ngroups"
comes to mind. But with good doco, kauth_cred_alloc1 can work.
> +{
> + kauth_cred_t cred;
> +
> + if (ngroups < 0 || ngroups > NGROUPS)
> + return NULL;
> +
> + cred = kauth_cred_alloc();
> + cred->cr_ngroups = ngroups;
> + if (grbuf != NULL)
> + *grbuf = cred->cr_groups;
> +
> + return cred;
> +}
Minor nit, check for failure of kauth_cred_alloc().
> +void
> +kauth_cred_alloc_finalise_grouplist(kauth_cred_t cred)
> +{
> + KASSERT(cred->cr_refcnt == 1);
> +}
> +
How about kauth_cred_alloc1_finalize() (changing alloc1 if we change the
allocation routine's name as above)? The idea is that allocation is a two
step process, and this finishes the second step.
Another nit, I think it'd be good to pass the gid array in. While our
current credential structure doesn't have to do any work here, if we
changed how we handle groups (say GUIDS), we could have a lot of work to
do here.
Phrased another way, I think that we aren't violating kauth's opacity if
the code doesn't depend on the fact group membership is stored as a set of
gid_t's. That the code happens to work fast if it is is ok as long as the
external code doesn't need to change if we change the implementation.
As a test, the calling code should work just as well if we change the
alloc1()/finalize() implementation to malloc() a buffer and then memcpy()
it in finalize(). I think things are easiest then if we pass the array
back in.
The only other issue I see is I'd like it if we could make it explicit
that you have to call kauth_cred_alloc1()/
kauth_cred_alloc_finalise_grouplist() as a pair. As in something we could
KASSERT(). I realize I'm now raising the bar some and making more work, so
I'm not going to press hard at the moment. But we have a partially-set-up
kauth_cred_t, and I'd like to make sure we can't do anything else with it
until we are done.
I think what I'm thinking of is some sort of "initializing" flag in the
kauth_cred_t and KASSERTS to make sure it's not set.
> @@ -567,19 +567,17 @@ sys_setgroups(struct lwp *l, void *v, re
> int error;
> gid_t *grbuf;
>
> - ncred = kauth_cred_alloc();
> + ncred = kauth_cred_alloc1(SCARG(uap, gidsetsize), &grbuf);
> + if (ncred == NULL)
> + return EINVAL;
>
> - grbuf = kauth_cred_setngroups(ncred, SCARG(uap, gidsetsize));
> - if (grbuf == NULL)
> - error = EINVAL;
> - else {
> - error = copyin(SCARG(uap, gidset), grbuf,
> - SCARG(uap, gidsetsize) * sizeof(gid_t));
> - }
> + error = copyin(SCARG(uap, gidset), grbuf,
> + SCARG(uap, gidsetsize) * sizeof(gid_t));
> if (error != 0) {
> kauth_cred_free(ncred);
> return error;
> }
> + kauth_cred_alloc_finalise_grouplist(ncred);
>
> return kauth_proc_setgroups(l, ncred);
> }
Thank you for making these changes!
Take care,
Bill
Attachment:
pgpNotNR_pqf9.pgp
Description: PGP signature