> On Jul 11, 2018, at 2:19 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > On Wed, 2018-07-11 at 14:06 -0400, Chuck Lever wrote: >>> On Jul 11, 2018, at 12:38 PM, Trond Myklebust <trondmy@hammerspace. >>> com> wrote: >>> >>> On Wed, 2018-07-11 at 12:05 -0400, Steve Dickson wrote: >>>> Hey Trond, >>>> >>>> On 07/11/2018 11:25 AM, Steve Dickson wrote: >>>>> The cause is that the xdr_putlong uses a long to store the >>>>> converted value, then passes it to fwrite as a byte buffer. >>>>> Only the first 4 bytes are written, which is okay for a LE >>>>> system after byteswapping, but writes all zeroes on BE systems. >>>>> >>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1261738 >>>>> >>>>> Reviewed-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >>>>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx> >>>>> --- >>>>> v4: Use UINT32_MAX instead of INT32_MAX in boundary check. >>>> >>>> Talking with some old crusty types ;-) they pointed out >>>> that all these routines use a signed arguments and >>>> always have... So again why is using an unsigned max >>>> the right thing to do? >>> >>> As I said earlier, you are casting from a larger type to a smaller >>> type, and you want it to work for both signed and unsigned 32-bit >>> values. Consider this kind of code: >>> >>> int32_t foo = 0xFFFF0000; >>> ret = xdrstdio_putlong(xdr, foo); I doubt that will compile everywhere, since the second parameter is supposed to be a pointer. Even if we add "&" the C compiler will likely not allow an implicit typecast and an "address of". Let's call the API portably: long foo = 0xFFFF0000; ret = xdrstdio_putlong(xdr, &foo); On a system with 32-bit longs, foo contains a negative value. On a system with 64-bit longs, foo contains a positive value. My narrow range check will fail, but a UINT32_MAX check would allow xdrstdio_putlong to proceed. My desired outcome is that the API works for the same range of values on platforms with 32-bit longs and 64-bit longs. If this API is to work the same on both classes of platform, I guess we do want "*lp > UINT32_MAX || *lp < INT32_MIN". >>> Since foo is signed, then (long)foo ends up getting cast to >>> 0xFFFFFFFFFFFF0000L, which is negative, but is > (long)INT32_MIN. >>> So >>> ret == TRUE; >> On 32-bit systems, xdrstdio_putlong() cannot work for values >> between INT32_MAX and UINT32_MAX. It should return FALSE for >> these values on 64-bit systems. In fact, that is the way this >> API behaves for other 64-bit aware libtirpc implementations. > > On a 32-bit system there is no need to check anything, because > sizeof(long) == sizeof(int) == 4. > The problems occur once you start casting from a smaller sized integer > to a larger one because the unsigned cast will behave differently to > the signed cast and result in a bitwise very different value. I'm confused. The patch _removes_ unsigned typecasts. We're no longer converting between signed and unsigned integers in these two API functions. xdrstdio_putlong converts a (potentially) larger signed integer to a (potentially) smaller signed integer. If a larger integer cannot fit in a smaller destination, the only thing that can happen is that the most-significant bits of the larger integer are discarded, IIUC. >>> Now try: >>> >>> uint32_t bar = 0xFFFF0000U; >>> ret = xdrstdio_putlong(xdr, bar); >>> >>> >>> Since bar is unsigned, then (long)bar gets cast to 0xFFFF0000L. >>> That is >>> clearly > (long)INT32_MAX == 0x7FFFFFFFL, and so the above fails >>> with >>> ret == FALSE. >>> >>> >>> BTW: The above is pretty much exactly how xdr_int32_t() and >>> xdr_uint32_t() work. The former does a signed cast to long, while >>> the >>> latter does an unsigned cast to long. >> >> If that's the case, we should examine how other 64-bit aware >> libtirpc implementations work and fix ours to behave the same. >> Our libtirpc forked in the late 1990s before 64-bit systems >> were widely deployed, so I have some doubts whether our >> implementation is correct. > > Observe: > > bool_t > xdr_int32_t(xdrs, int32_p) > XDR *xdrs; > int32_t *int32_p; > { > long l; > > switch (xdrs->x_op) { > > case XDR_ENCODE: > l = (long) *int32_p; > return (XDR_PUTLONG(xdrs, &l)); > > case XDR_DECODE: > if (!XDR_GETLONG(xdrs, &l)) { > return (FALSE); > } > *int32_p = (int32_t) l; > return (TRUE); > > case XDR_FREE: > return (TRUE); > } > /* NOTREACHED */ > return (FALSE); > } > > bool_t > xdr_u_int32_t(xdrs, u_int32_p) > XDR *xdrs; > u_int32_t *u_int32_p; > { > u_long l; > > switch (xdrs->x_op) { > > case XDR_ENCODE: > l = (u_long) *u_int32_p; > return (XDR_PUTLONG(xdrs, (long *)&l)); > > case XDR_DECODE: > if (!XDR_GETLONG(xdrs, (long *)&l)) { > return (FALSE); > } > *u_int32_p = (u_int32_t) l; > return (TRUE); > > case XDR_FREE: > return (TRUE); > } > /* NOTREACHED */ > return (FALSE); > } > >>>> steved. >>>> >>>>> >>>>> v3: Reworked the bounds checking >>>>> >>>>> v2: Added bounds checking >>>>> Changed from unsigned to signed >>>>> >>>>> src/xdr_stdio.c | 15 ++++++++++++--- >>>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c >>>>> index 4410262..846c7bf 100644 >>>>> --- a/src/xdr_stdio.c >>>>> +++ b/src/xdr_stdio.c >>>>> @@ -38,6 +38,7 @@ >>>>> */ >>>>> >>>>> #include <stdio.h> >>>>> +#include <stdint.h> >>>>> >>>>> #include <arpa/inet.h> >>>>> #include <rpc/types.h> >>>>> @@ -103,10 +104,12 @@ xdrstdio_getlong(xdrs, lp) >>>>> XDR *xdrs; >>>>> long *lp; >>>>> { >>>>> + int32_t mycopy; >>>>> >>>>> - if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs- >>>>>> x_private) >>>>> != 1) >>>>> + if (fread(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs- >>>>>> x_private) != 1) >>>>> >>>>> return (FALSE); >>>>> - *lp = (long)ntohl((u_int32_t)*lp); >>>>> + >>>>> + *lp = (long)ntohl(mycopy); >>>>> return (TRUE); >>>>> } >>>>> >>>>> @@ -115,8 +118,14 @@ xdrstdio_putlong(xdrs, lp) >>>>> XDR *xdrs; >>>>> const long *lp; >>>>> { >>>>> - long mycopy = (long)htonl((u_int32_t)*lp); >>>>> + int32_t mycopy; >>>>> + >>>>> +#if defined(_LP64) >>>>> + if ((*lp > UINT32_MAX) || (*lp < INT32_MIN)) >>>>> + return (FALSE); >>>>> +#endif >>>>> >>>>> + mycopy = (int32_t)htonl((int32_t)*lp); >>>>> if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs- >>>>>> x_private) != 1) >>>>> >>>>> return (FALSE); >>>>> return (TRUE); >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux- >>>> nfs" >>>> in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.htm >>>> l >>> >>> -- >>> Trond Myklebust >>> Linux NFS client maintainer, Hammerspace >>> trond.myklebust@xxxxxxxxxxxxxxx >>> >>> ----------------------------------------------------------------- >>> ------------- >>> Check out the vibrant tech community on one of the world's most >>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot >>> _______________________________________________ >>> Libtirpc-devel mailing list >>> Libtirpc-devel@xxxxxxxxxxxxxxxxxxxxx >>> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel >> >> -- >> Chuck Lever >> >> >> > -- > Trond Myklebust > CTO, Hammerspace Inc > 4300 El Camino Real, Suite 105 > Los Altos, CA 94022 > www.hammer.space -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
