Brooks Davis wrote: >On Thu, Jul 30, 2020 at 03:48:34PM +0000, Rick Macklem wrote: >> 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. I think I will try and come up with a man page patch, noting that xdr_long() always puts 32bits on the wire, even if long is 64bits for the arch.
>> >> >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. > >Thank you for all the analysis. I'd initially changed all the uses >of bad to use xdr_int(), but switched to this "fix" because it's what >NetBSD and OpenBSD have been using for over a decade (and there was >less churn). I'm happy to flip it the other way if that seems more >correct/less confusing. I think your current patch is fine. The confusion is w.r.t. what xdr_long() does for a 64bit long and I think a man page update may be the way to go. --> If you look in xdr.c, xdr_int() assigns the value to a long and then ends up truncating it back down, similar to xdr_long(). --> Some of the stuff in xdr.c is pretty scary for 64bit longs, but it all seems to work, once you look at it for a while.;-) >The previous code does in fact cause a 64-bit load of a pointer to an >int on 64-bit platforms. I hit this in CheriBSD because that pointer >had 4-byte bounds. Yes. The first time I looked at the code (it was late evening), I misread ((u_int32_t)*lp) as *((u_int32_t *)lp) and that was why I thought your patch was broken. Thanks for doing this and sorry about the noise, rick ps: Personally, I've never understood why ANSI C allowed "long" to be 64bits on some arches. I still bump into hassles because the old K&R code was written assuming long to be 32bits. -- Brooks _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"