From owner-svn-src-head@FreeBSD.ORG Wed Oct 3 13:49:18 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id BA84D106564A; Wed, 3 Oct 2012 13:49:18 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 4DA628FC16; Wed, 3 Oct 2012 13:49:17 +0000 (UTC) Received: from c122-106-157-84.carlnfd1.nsw.optusnet.com.au (c122-106-157-84.carlnfd1.nsw.optusnet.com.au [122.106.157.84]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q93Dn8wj020377 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 3 Oct 2012 23:49:09 +1000 Date: Wed, 3 Oct 2012 23:49:08 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Pedro F. Giffuni" In-Reply-To: <201210030344.q933iOdn054463@svn.freebsd.org> Message-ID: <20121003224737.V46116@besplex.bde.org> References: <201210030344.q933iOdn054463@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r241152 - head/lib/libc/rpc X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Oct 2012 13:49:18 -0000 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