svn commit: r241152 - head/lib/libc/rpc

[Bruce Evans]

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