Date: Wed, 3 Oct 2012 23:49:08 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Pedro F. Giffuni" <pfg@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r241152 - head/lib/libc/rpc Message-ID: <20121003224737.V46116@besplex.bde.org> In-Reply-To: <201210030344.q933iOdn054463@svn.freebsd.org> References: <201210030344.q933iOdn054463@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 3 Oct 2012, Pedro F. Giffuni wrote:
> Log:
> rpc: convert all uid and gid variables of the type uid_t and gid_t.
>
> As part of the previous commit, uses of xdr_int() were replaced
> with xdr_u_int(). This has undesired effects as the second
> argument doesn't match exactly uid_t or gid_t. It also breaks
> assumptions in the size of the provided types.
>
> To work around those issues we revert back to the use of xdr_int()
> but provide proper casting so the behaviour doesn't change.
Er, this makes the problem worse. It doesn't actually make the argument
match, but uses a bogus cast whose main effect is to hide the type
mismatch from the compiler (the compiler might still consider the cast
invalid).
> Modified: head/lib/libc/rpc/authunix_prot.c
> ==============================================================================
> --- head/lib/libc/rpc/authunix_prot.c Wed Oct 3 03:00:37 2012 (r241151)
> +++ head/lib/libc/rpc/authunix_prot.c Wed Oct 3 03:44:23 2012 (r241152)
> @@ -67,11 +67,11 @@ xdr_authunix_parms(xdrs, p)
>
> paup_gids = &p->aup_gids;
>
> - if (xdr_u_long(xdrs, &(p->aup_time))
> - && xdr_string(xdrs, &(p->aup_machname), MAX_MACHINE_NAME)
> - && xdr_u_int(xdrs, &(p->aup_uid))
> - && xdr_u_int(xdrs, &(p->aup_gid))
> - && xdr_array(xdrs, (char **) paup_gids,
> + if (xdr_u_long(xdrs, &(p->aup_time)) &&
The old code was closer to being correct, and is still used for aup_time.
For times it still works almost correctly as follows:
- times have type time_t, which is always signed and sometimes 64 bits
in FreeBSD
- aup_time has type u_long, which is always unsigned and sometimes 64
bits in FreeBSD. It is 64 bits for some arches that don't have 64-
bit u_longs (mips and arm), because time_t on these arches was
bloated to practice the transition to a 64-bit time_t (this is very
unnecessary since 32 bits unsigned work until 2106).
- the current time in seconds (in a time_t) is assigned to aup_time,
without any bounds checking of course
- after that, the time as a u_long in aup_time us usable.
Add the overflow checking and it would work correctly.
> + xdr_string(xdrs, &(p->aup_machname), MAX_MACHINE_NAME) &&
> + xdr_int(xdrs, (int *) &(p->aup_uid)) &&
> + xdr_int(xdrs, (int *) &(p->aup_gid)) &&
For ids it used to work almost correctly as follows:
- ids have type uid_t and gid_t, which are always uint32_t in FreeBSD
- aup_uid and aup_gid used to have type int
- the ids (in a uid_t or gid_t) were assigned to aup*_idm without any
bounds checking of course. The sign mismatch is a larger problem in
practice that for times. Times after 2038 shouldn't actually occur,
but ids above INT_MAX can. But in practice, the same benign overflow
will probably allow both to work when there is a sign error.
- after that, the time as a u_long in aup_time us usable.
Add the overflow checking and it would work correctly, but u_int's should
be used throughout so that supported ids like ((uid_t)-2) don't have to
rejected by the bounds check.
Now it works very incorrectly:
- the types in the assignment are now the same (uid_t or gid_t), so there
is no problem in the assignment, but this gives types that are unusable
here
- despite being unusable here, they are used here. &(p->aup_*id)) is a
pointer to a uid_t or a gid_t. Pretending that it is a pointer to an
int doesn't make it one. Without the (int *) cast, the compiler would
detect the type mismatch as a sign mismatch at certain warning levels
(the (int *) taken by xdr_int is incompatible with the object pointer
([ug]id_t *). If the conversion is reasonable, then xdr_int()'s
prototype would do it, and the has no effect except to possibly do
the same thing without a warning or with a different warning.
> + xdr_array(xdrs, (char **) paup_gids,
> &(p->aup_len), NGRPS, sizeof(int), (xdrproc_t)xdr_int) ) {
> return (TRUE);
> }
It already used a bogus cast on paup_gids. I'm surprised that compiles
(paup_gids has type gid_t **). I didn't notice that before, or that the
sizeof() always matched the xdr_int. The elements need to be ints to
work (as above), but now have type gid_t.
If the xdr_array() API had been invented after 1980, then it would have
used void ** instead of char **, something like the following:
- it apparently wants to pass the array pointer indirectly. The pointer
must have type void *, not gid_t *. Then we take its address and get
a void **. The pointer must be coverted from its original type to
void *, and the code already does a conversion, but with nonsense types:
- aup_gids used to have type int * and now has type gid_t *
- this must be converted to void * (or char * to match the pre-1980 API).
"void *vaup_gids = p->aup_gids;" might work.
- now we have a void *, we can point to it using void **. Take the address
of the local variable vaup_gids instead of p->aup_gids.
- xdr_array() needs to convert from the void ** back to the original pointer.
Indirecting the void ** only gives the void *. The technically least
incorrect way (without changing the API much) is to switch on the element
size.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121003224737.V46116>