svn commit: r363625 - stable/12/usr.sbin/mountd

[Jessica Clarke]

On 30 Jul 2020, at 23:38, Rick Macklem <rmacklem at uoguelph.ca> wrote:
> 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.

Yeah, c.f. XChangeProperty(3) which has a similarly weird interface
(but slightly more so) that seriously confused me a few years ago. If
you pass 8 for the number of bits to return to you it wants a char, for
16 it wants a short and for 32 it wants a long. XGetWindowProperty(3)
also talks about long_offset and long_length when it really means
int32_offset/length, or just int_offset/length these days in POSIX. As
for _why_ it's done, well, char is 8, short is 16, int is 32 so then
why make long 32 too when it could be 64 and still efficient (in terms
of instructions, not necessarily space; also means long long is free
for int128 on 128-bit machines!). Plus maybe more often than not people
wanted a machine word, not a 32-bit value, though by now hopefully
people have learnt to use size_t/uintptr_t as appropriate (CHERI work
suggests that's _mostly_ true). But then we have to have Windows which
decided to keep a 32-bit long, so it's all a mess anyway.

Jess