Date: Tue, 02 Oct 2012 20:47:10 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r241141 - in head: include/rpc lib/libc/rpc sys/rpc Message-ID: <506B991E.6010207@FreeBSD.org> In-Reply-To: <20121003075143.N938@besplex.bde.org> References: <201210021900.q92J0uHT088549@svn.freebsd.org> <20121003075143.N938@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Thank you Bruce;
On 10/02/2012 17:19, Bruce Evans wrote:
> On Tue, 2 Oct 2012, Pedro F. Giffuni wrote:
>
>> Log:
>> RPC: Convert all uid and gid variables of the type uid_t and gid_t.
>>
>> This matches what upstream (OpenSolaris) does.
>>
>> Tested by: David Wolfskill
>> Obtained from: Bull GNU/Linux NFSv4 project (libtirpc)
>> MFC after: 3 days
>
> This still assumes that uid_t and gid_t are precisely u_int, in stronger
> ways than before.
>
>> Modified: head/lib/libc/rpc/authunix_prot.c
>> ==============================================================================
>>
>> --- head/lib/libc/rpc/authunix_prot.c Tue Oct 2 18:38:05 2012
>> (r241140)
>> +++ head/lib/libc/rpc/authunix_prot.c Tue Oct 2 19:00:56 2012
>> (r241141)
>> @@ -60,7 +60,7 @@ xdr_authunix_parms(xdrs, p)
>> XDR *xdrs;
>> struct authunix_parms *p;
>> {
>> - int **paup_gids;
>> + gid_t **paup_gids;
>>
>> assert(xdrs != NULL);
>> assert(p != NULL);
>> @@ -69,8 +69,8 @@ xdr_authunix_parms(xdrs, p)
>>
>> if (xdr_u_long(xdrs, &(p->aup_time))
>> && xdr_string(xdrs, &(p->aup_machname), MAX_MACHINE_NAME)
>> - && xdr_int(xdrs, &(p->aup_uid))
>> - && xdr_int(xdrs, &(p->aup_gid))
>> + && xdr_u_int(xdrs, &(p->aup_uid))
>> + && xdr_u_int(xdrs, &(p->aup_gid))
>> && xdr_array(xdrs, (char **) paup_gids,
>> &(p->aup_len), NGRPS, sizeof(int), (xdrproc_t)xdr_int) ) {
>> return (TRUE);
>>
>
> xdr doesn't support arbitrary types. Here the very name of xdr_u_int()
> indicates that it only works on u_int's. Its second arg must be a
> pointer to u_int (misspelled unsigned in the man page, so it doesn't
> match the function name in a different, harmless way). The arg used
> to be a pointer to an int, and the call to xdr_int() used to match that.
> The arg is now a pointer to a uid_t or gid_t, and the call to xdr_u_int()
> only matches that accidentally. (The types happen to be uint32_t, which
> happens to be u_int.)
>
OK, to solve this I was thinking of adding a cast to uint32_t, but this is
ugly and doesn't really do nothing. Looking at OpenSolaris they do a
cast but more than a cosmetical fix it is a requirement because they
still use xdr_int there. This sounds like a better approach for us too.
> More careful code would select an xdr translation function based on
> sizeof(uid_t) etc.
>
> The above xdr_array() call takes a element size arg that is necessary
> for stepping through the array, but isn't careful to use sizeof() on
> the correct type. It uses sizeof() on a hard-coded type, and you just
> changed the element type without changing the hard-coded type. It used
> to match (int == int), but now doesn't (int != gid_t). sizeof() should
> be applied to objects and not types to get the object size right without
> hard-coding its type.
>
Nice catch, that is certainly wrong, and libtirpc still has it. This isn't
necessary when adopting the solution above though.
> The first type of type error should be detected at compile time. The
> second one (the hard-coded sizeof(int)) probably cannot be. And there
> is yet another new type error in the xdr_array() call. It takes a
> pointer to a translation function. The function used to match the
> elemennt
> type, but now doesn't (int != gid_t, and also int != underlying type
> of gid_t == u_int). The API requires casting the pointer to a generic
> one using an obfuscated typedef, so the compiler cannot detect this
> type mismatch at compile time (without breaking the API generally).
>
I only changed authunix_create and its parameters, any other underlying
issue is preexistent :).
Pedro.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?506B991E.6010207>