| Message ID | 20180710144405.28795-1-steved@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Jul 10, 2018, at 10:44 AM, Steve Dickson <SteveD@redhat.com> wrote: > > From: Daniel Sands <dnsands@sandia.gov> > > 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 > > Signed-off-by: Steve Dickson <steved@redhat.com> > --- > v3: Reworked the bounds checking > > v2: Added bounds checking > Changed from unsigned to signed > > src/xdr_stdio.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c > index 4410262..b902acd 100644 > --- a/src/xdr_stdio.c > +++ b/src/xdr_stdio.c > @@ -103,10 +103,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 +117,14 @@ xdrstdio_putlong(xdrs, lp) > XDR *xdrs; > const long *lp; > { > - long mycopy = (long)htonl((u_int32_t)*lp); > + int32_t mycopy; > + > +#if defined(_LP64) Hi Steve, _LP64 is gcc-specific. Hope that's OK. Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > + if ((*lp > INT32_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); > -- > 2.17.1 > > > ------------------------------------------------------------------------------ > 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@lists.sourceforge.net > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2018 10:49 AM, Chuck Lever wrote: > > >> On Jul 10, 2018, at 10:44 AM, Steve Dickson <SteveD@redhat.com> wrote: >> >> From: Daniel Sands <dnsands@sandia.gov> >> >> 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 >> >> Signed-off-by: Steve Dickson <steved@redhat.com> >> --- >> v3: Reworked the bounds checking >> >> v2: Added bounds checking >> Changed from unsigned to signed >> >> src/xdr_stdio.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c >> index 4410262..b902acd 100644 >> --- a/src/xdr_stdio.c >> +++ b/src/xdr_stdio.c >> @@ -103,10 +103,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 +117,14 @@ xdrstdio_putlong(xdrs, lp) >> XDR *xdrs; >> const long *lp; >> { >> - long mycopy = (long)htonl((u_int32_t)*lp); >> + int32_t mycopy; >> + >> +#if defined(_LP64) > > Hi Steve, _LP64 is gcc-specific. Hope that's OK. > > Reviewed-by: Chuck Lever <chuck.lever@oracle.com> Here the reason I left it: https://stackoverflow.com/questions/685124/how-to-identify-a-64-bit-build-on-linux-using-the-preprocessor Should it be removed? steved. > > >> + if ((*lp > INT32_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); >> -- >> 2.17.1 >> >> >> ------------------------------------------------------------------------------ >> 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@lists.sourceforge.net >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Jul 10, 2018, at 11:43 AM, Steve Dickson <SteveD@RedHat.com> wrote: > > > > On 07/10/2018 10:49 AM, Chuck Lever wrote: >> >> >>> On Jul 10, 2018, at 10:44 AM, Steve Dickson <SteveD@redhat.com> wrote: >>> >>> From: Daniel Sands <dnsands@sandia.gov> >>> >>> 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 >>> >>> Signed-off-by: Steve Dickson <steved@redhat.com> >>> --- >>> v3: Reworked the bounds checking >>> >>> v2: Added bounds checking >>> Changed from unsigned to signed >>> >>> src/xdr_stdio.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c >>> index 4410262..b902acd 100644 >>> --- a/src/xdr_stdio.c >>> +++ b/src/xdr_stdio.c >>> @@ -103,10 +103,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 +117,14 @@ xdrstdio_putlong(xdrs, lp) >>> XDR *xdrs; >>> const long *lp; >>> { >>> - long mycopy = (long)htonl((u_int32_t)*lp); >>> + int32_t mycopy; >>> + >>> +#if defined(_LP64) >> >> Hi Steve, _LP64 is gcc-specific. Hope that's OK. >> >> Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > Here the reason I left it: > https://stackoverflow.com/questions/685124/how-to-identify-a-64-bit-build-on-linux-using-the-preprocessor > > Should it be removed? Not at all, I'm just pointing out that if libtirpc is built with a non-gcc compiler, _LP64 is not guaranteed to be defined. That stackoverflow article suggests a couple of more portable ways of checking for 64-bit. Are there distributions that might build libtirpc with a non-gcc compiler (like Clang) ? If no, then this isn't an issue at all. > steved. > >> >> >>> + if ((*lp > INT32_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); >>> -- >>> 2.17.1 >>> >>> >>> ------------------------------------------------------------------------------ >>> 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@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel >> >> -- >> Chuck Lever -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2018 11:51 AM, Chuck Lever wrote: > > >> On Jul 10, 2018, at 11:43 AM, Steve Dickson <SteveD@RedHat.com> wrote: >> >> >> >> On 07/10/2018 10:49 AM, Chuck Lever wrote: >>> >>> >>>> On Jul 10, 2018, at 10:44 AM, Steve Dickson <SteveD@redhat.com> wrote: >>>> >>>> From: Daniel Sands <dnsands@sandia.gov> >>>> >>>> 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 >>>> >>>> Signed-off-by: Steve Dickson <steved@redhat.com> >>>> --- >>>> v3: Reworked the bounds checking >>>> >>>> v2: Added bounds checking >>>> Changed from unsigned to signed >>>> >>>> src/xdr_stdio.c | 14 +++++++++++--- >>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c >>>> index 4410262..b902acd 100644 >>>> --- a/src/xdr_stdio.c >>>> +++ b/src/xdr_stdio.c >>>> @@ -103,10 +103,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 +117,14 @@ xdrstdio_putlong(xdrs, lp) >>>> XDR *xdrs; >>>> const long *lp; >>>> { >>>> - long mycopy = (long)htonl((u_int32_t)*lp); >>>> + int32_t mycopy; >>>> + >>>> +#if defined(_LP64) >>> >>> Hi Steve, _LP64 is gcc-specific. Hope that's OK. >>> >>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com> >> Here the reason I left it: >> https://stackoverflow.com/questions/685124/how-to-identify-a-64-bit-build-on-linux-using-the-preprocessor >> >> Should it be removed? > > Not at all, I'm just pointing out that if libtirpc is built > with a non-gcc compiler, _LP64 is not guaranteed to be > defined. > > That stackoverflow article suggests a couple of more portable > ways of checking for 64-bit. > > Are there distributions that might build libtirpc with a > non-gcc compiler (like Clang) ? If no, then this isn't an > issue at all. Lets cross that bridge if/when it comes... Thanks for the time! steved. > > >> steved. >> >>> >>> >>>> + if ((*lp > INT32_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); >>>> -- >>>> 2.17.1 >>>> >>>> >>>> ------------------------------------------------------------------------------ >>>> 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@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/libtirpc-devel >>> >>> -- >>> Chuck Lever > > -- > Chuck Lever > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c index 4410262..b902acd 100644 --- a/src/xdr_stdio.c +++ b/src/xdr_stdio.c @@ -103,10 +103,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 +117,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 > INT32_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);