Date: Wed, 3 Oct 2012 17:42:15 -0400 (EDT) From: Rick Macklem <rmacklem@uoguelph.ca> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, "Pedro F. Giffuni" <pfg@FreeBSD.org>, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r241152 - head/lib/libc/rpc Message-ID: <740058841.1682948.1349300535316.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20121003224737.V46116@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans wrote:
> 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
I haven't been looking at these patches (and don't really have any time
to do so). I was just going to note that Sun RPC puts 32bit uid and gids
on the wire. As such, I think you'll want the "on the wire" defined as
32bit #s (maybe uint32_t or int32_t) and the xdr conversion routines
should be ones that handle 32bits. (I'd have to look at them to remember
which ones are correct for 32bits on all arches. I think they once used
the naming where "long" ws considered to be 32bits. Ah, the good old days;-)
Anyhow, good luck with it guys, rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?740058841.1682948.1349300535316.JavaMail.root>