--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--