> On Jul 11, 2018, at 12:38 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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); > > 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. > 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. >> 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.html > -- > 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 -- 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
