Re: [PATCH V2] xdrstdio_create buffers do not output encoded values on ppc

[Trond Myklebust <trondmy@xxxxxxxxxxxxxxx>]

On Fri, 2018-06-29 at 15:33 -0400, Chuck Lever wrote:
> > On Jun 29, 2018, at 3:29 PM, Trond Myklebust <trondmy@hammerspace.c
> > om> wrote:
> > 
> > On Fri, 2018-06-29 at 14:42 -0400, Steve Dickson wrote:
> > > From: Daniel Sands <dnsands@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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)
> > > +	if ((*lp > INT32_MAX) || (*lp < INT32_MIN))
> > > +		return (FALSE);
> > 
> > The will work just fine if your cast is:
> > 	int a;
> > 
> > 	lp = (long)a;
> > 
> > ...but it won't do the right thing for
> > 
> > 	unsigned int b = UINT32_MAX;
> > 
> > 	lp = (long)b;	/* set lp to UINT32_MAX */
> 
> mycopy is an int32_t now. Does that help?

Not really. 'mycopy' isn't involved in the check above, and *lp ==
UINT32_MAX should in either case get cast to the same 32-bit bit
pattern whether mycopy is an int32_t or a u_int32_t.

It's when you try to cast back from 'mycopy' to a long type that the
two can end up differing (an int32_t might get sign extended).

> 
> > Since there us no dedicated xdrstdio_putulong() to use for unsigned
> > integers, we need the check to work for both cases.
> > For that reason, I think the above check either has to be:
> > 
> > 	if (*lp == (long)((u_int32_t)*lp))
> > 		return (FALSE);
> > 
> > ...or something uglier like
> > 
> > 	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);
> > 
> > -- 
> > Trond Myklebust
> > CTO, Hammerspace Inc
> > 4300 El Camino Real, Suite 105
> > Los Altos, CA 94022
> > www.hammer.space
> > 
> > N‹§ēæėrļ›yúčšØbēXŽķĮ§vØ^–)Þš{.nĮ+‰·ĨŠ{ąû"žØ^n‡rĄöĶzˁëh™Ļč­Ú&ĒøŪ
> > GŦéhŪ(­éšŽŠÝĒj"úķm§ĸïęäzđÞ–ŠāþfĢĒ·hšˆ§~ˆm
> 
> --
> Chuck Lever
> 
> 
> 
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx

��.n��������+%������w��{.n�����{��w���jg��������ݢj����G�������j:+v���w�m������w�������h�����٥