Subject: Re: CVS commit: src/sys
To: Bill Stouder-Studenmund <wrstuden@netbsd.org>
From: Bill Stouder-Studenmund <wrstuden@netbsd.org>
List: tech-kern
Date: 06/24/2007 14:05:22
--Qxx1br4bt0+wmkIi
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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:
> >=20
> > Setting groups:
>=20
> 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);
> }
>=20
> +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=
=20
drink the Objective C naming kool-aid. "kauth_cred_alloc_with_ngroups"=20
comes to mind. But with good doco, kauth_cred_alloc1 can work.
> +{ =20
> + kauth_cred_t cred;
> + =20
> + if (ngroups < 0 || ngroups > NGROUPS)
> + return NULL;
> +
> + cred =3D kauth_cred_alloc();
> + cred->cr_ngroups =3D ngroups;
> + if (grbuf !=3D NULL)
> + *grbuf =3D cred->cr_groups;
> + =20
> + return cred;
> +}
Minor nit, check for failure of kauth_cred_alloc().
> +void
> +kauth_cred_alloc_finalise_grouplist(kauth_cred_t cred)
> +{ =20
> + KASSERT(cred->cr_refcnt =3D=3D 1);
> +}
> +
How about kauth_cred_alloc1_finalize() (changing alloc1 if we change the=20
allocation routine's name as above)? The idea is that allocation is a two=
=20
step process, and this finishes the second step.
Another nit, I think it'd be good to pass the gid array in. While our=20
current credential structure doesn't have to do any work here, if we=20
changed how we handle groups (say GUIDS), we could have a lot of work to=20
do here.
Phrased another way, I think that we aren't violating kauth's opacity if=20
the code doesn't depend on the fact group membership is stored as a set of=
=20
gid_t's. That the code happens to work fast if it is is ok as long as the=
=20
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=20
alloc1()/finalize() implementation to malloc() a buffer and then memcpy()=
=20
it in finalize(). I think things are easiest then if we pass the array=20
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=
=20
KASSERT(). I realize I'm now raising the bar some and making more work, so=
=20
I'm not going to press hard at the moment. But we have a partially-set-up=
=20
kauth_cred_t, and I'd like to make sure we can't do anything else with it=
=20
until we are done.=20
I think what I'm thinking of is some sort of "initializing" flag in the=20
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;
>=20
> - ncred =3D kauth_cred_alloc();
> + ncred =3D kauth_cred_alloc1(SCARG(uap, gidsetsize), &grbuf);
> + if (ncred =3D=3D NULL)
> + return EINVAL;
> =20
> - grbuf =3D kauth_cred_setngroups(ncred, SCARG(uap, gidsetsize));
> - if (grbuf =3D=3D NULL)
> - error =3D EINVAL;
> - else {
> - error =3D copyin(SCARG(uap, gidset), grbuf,
> - SCARG(uap, gidsetsize) * sizeof(gid_t));
> - }
> + error =3D copyin(SCARG(uap, gidset), grbuf,
> + SCARG(uap, gidsetsize) * sizeof(gid_t));
> if (error !=3D 0) {
> kauth_cred_free(ncred); =20
> return error;
> }
> + kauth_cred_alloc_finalise_grouplist(ncred);
> =20
> return kauth_proc_setgroups(l, ncred);
> }
Thank you for making these changes!
Take care,
Bill
--Qxx1br4bt0+wmkIi
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (NetBSD)
iD8DBQFGftySWz+3JHUci9cRAli1AKCQWlE+xxeAiN5zrSoRG1IBnLMczgCdHzWm
dQnOVlm/GXPrqaT2hThS+jw=
=XQkY
-----END PGP SIGNATURE-----
--Qxx1br4bt0+wmkIi--