Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.4 \(3445.8.2\))
Subject: Re: [Libtirpc-devel] [PATCH V3] xdrstdio_create buffers do not output
 encoded values on ppc
From: Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <b2a9785b-4e09-d7fd-539f-b23ecefffd7f@RedHat.com>
Date: Tue, 10 Jul 2018 11:51:38 -0400
Cc: libtirpc List <libtirpc-devel@lists.sourceforge.net>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Message-Id: <07FC4A01-F7AE-4758-B239-6A48E7BE3247@oracle.com>
References: <20180710144405.28795-1-steved@redhat.com>
 <50421097-292E-4DFB-B25A-69A66D634C45@oracle.com>
 <b2a9785b-4e09-d7fd-539f-b23ecefffd7f@RedHat.com>
To: Steve Dickson <SteveD@RedHat.com>
Sender: linux-nfs-owner@vger.kernel.org


> On Jul 10, 2018, at 11:43 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>=20
>=20
>=20
> On 07/10/2018 10:49 AM, Chuck Lever wrote:
>>=20
>>=20
>>> On Jul 10, 2018, at 10:44 AM, Steve Dickson <SteveD@redhat.com> =
wrote:
>>>=20
>>> From: Daniel Sands <dnsands@sandia.gov>
>>>=20
>>> 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.
>>>=20
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=3D1261738
>>>=20
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>> v3: Reworked the bounds checking
>>>=20
>>> v2: Added bounds checking
>>>   Changed from unsigned to signed
>>>=20
>>> src/xdr_stdio.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>=20
>>> 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;
>>>=20
>>> -	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs->x_private) !=3D =
1)
>>> +	if (fread(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs->x_private) =
!=3D 1)
>>> 		return (FALSE);
>>> -	*lp =3D (long)ntohl((u_int32_t)*lp);
>>> +
>>> +	*lp =3D (long)ntohl(mycopy);
>>> 	return (TRUE);
>>> }
>>>=20
>>> @@ -115,8 +117,14 @@ xdrstdio_putlong(xdrs, lp)
>>> 	XDR *xdrs;
>>> 	const long *lp;
>>> {
>>> -	long mycopy =3D (long)htonl((u_int32_t)*lp);
>>> +	int32_t mycopy;
>>> +
>>> +#if defined(_LP64)
>>=20
>> Hi Steve, _LP64 is gcc-specific. Hope that's OK.
>>=20
>> 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
>=20
> 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.
>=20
>>=20
>>=20
>>> +	if ((*lp > INT32_MAX) || (*lp < INT32_MIN))
>>> +		return (FALSE);
>>> +#endif
>>>=20
>>> +	mycopy =3D (int32_t)htonl((int32_t)*lp);
>>> 	if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs->x_private) =
!=3D 1)
>>> 		return (FALSE);
>>> 	return (TRUE);
>>> --=20
>>> 2.17.1
>>>=20
>>>=20
>>> =
--------------------------------------------------------------------------=
----
>>> 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
>>=20
>> --
>> Chuck Lever

--
Chuck Lever