Date: Thu, 30 Jul 2020 15:48:34 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Ian Lepore <ian@freebsd.org>, Brooks Davis <brooks@FreeBSD.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-stable@freebsd.org" <svn-src-stable@freebsd.org>, "svn-src-stable-12@freebsd.org" <svn-src-stable-12@freebsd.org> Subject: Re: svn commit: r363625 - stable/12/usr.sbin/mountd Message-ID: <QB1PR01MB33646EB6938CB0A8A1C271D5DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <QB1PR01MB3364F79AB0FD28107B3AD341DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM> References: <202007272318.06RNIFjV005206@repo.freebsd.org> <QB1PR01MB3364334927A6502C76338B42DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM>, <4d5b871fad9412661c3914a64c8ca0b7a01d1dc6.camel@freebsd.org>, <QB1PR01MB3364F79AB0FD28107B3AD341DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM>
index | next in thread | previous in thread | raw e-mail
Rick Macklem wrote: >Ian Lepore wrote: >>On Thu, 2020-07-30 at 01:52 +0000, Rick Macklem wrote: >>> Brooks Davis wrote: >>> > Author: brooks >>> > Date: Mon Jul 27 23:18:14 2020 >>> > New Revision: 363625 >>> > URL: https://svnweb.freebsd.org/changeset/base/363625 >>> > >>> > Log: >>> > MFC r363439: >>> > >>> > Correct a type-mismatch between xdr_long and the variable "bad". >>> > >>> > [...] >>> --> I can't see how the xdr.c code would work for a machine that is >>> BIG_ENDIAN and where "long" is 64bits, but we don't have any of >>> those. >>> >> >>mips64 and powerpc64 are both big endian with 64-bit long. >Oops, I didn't know that. In the past, I've run PowerPC and MIPS, but thought >they both were little endian. (I recall the arches can be run either way.) > >Anyhow, take a look at head/lib/libc/xdr/xdr.c and it looks to me like it >has been broken "forever" (ever since we stopped using a K&R compiler >that would have always made "long" 32bits). OK, I took another look at xdr.c and it isn't broken as I thought. xdr_long() takes a "long *" argument ("long" in Sun XDR is 32bits), but then it only passes it as an argument to XDR_PUTLONG(), which is actually a call to xdrmem_putlong_aligned() or xdrmem_putlong_unaligned(). For xdrmem_putlong_aligned(), the line is: *(u_int32_t *)xdrs->x_private = htonl((u_int32_t)*lp); --> where lp is a "long *" I'll admit I'm not 100% sure if "(u_int32_t)*lp" gets the correct 32bits of a 64bit long pointer for all arches? (I'm not very good at knowing what type casts do.) If this is the equivalent of "u_int32_t t; t = *lp; htonl(t); then I think the code is ok? (At least it makes it clear that it is using 32bits of the value pointed to by the argument.) For xdrmem_putlong_unaligned(), it does the same thing via: u_int32_t l; …. l = htonl((u_int32_t)*lp); --> At least the man page for xdr_long() should be clarified to note it puts a 32bit quantity on the wire. >If anyone has either of these and can set up an NFS server on one of >them and then try and do an NFSv3 mount that is not allowed, it would >be interesting to see the packet trace and if the MNT RPC fails, because >it looks like it will put the high order 32bits on the wire and they'll >always be 0? It would still be interesting to test this on a 64bit big endian, but so long as the above cast works, it does look like it works for all arches. >Just to clarify. The behaviour wasn't broken by this commit. I just >don't see how the commit fixes anything? My mistake. Sorry for the noise. I now think the commit is correct since it uses "*lp" to get the value before casting it down to 32bits. Passing in an "int *" was incorrect. The code does seem to handle "long *" for 64bit arches, although it only puts 32bits "on-the-wire". rick, who was confused because he knew there was only supposed to be 32bits go on the wire. -- Ianhelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?QB1PR01MB33646EB6938CB0A8A1C271D5DD710>