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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2018-06-29 at 16:02 -0400, Chuck Lever wrote:
> > On Jun 29, 2018, at 3:48 PM, Trond Myklebust <trondmy@hammerspace.c
> > om> wrote:
> > 
> > On Fri, 2018-06-29 at 15:33 -0400, Chuck Lever wrote:
> > > > On Jun 29, 2018, at 3:29 PM, Trond Myklebust <trondmy@hammerspa
> > > > ce.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).
> 
> AFAICT we are casting *lp to an int32_t in this function. mycopy
> is not being cast to a long here.
> 
> Do you mean xdrstdio_GETlong needs further protection? Could be,
> there is a cast of an int32_t to a long there.
> 
> I agree that we should vet xdrstdio_getlong and xdrstdio_putlong for
> sign-extension issues very carefully.
> 

The worry for xdrstdio_putlong() should be how we handle the truncation
when going from (long)-> big endian (int32_t/u_int32_t). We don't want
to silently lose information.

AFAICS, when casting from a 'long' type, whether 'mycopy' is signed or
unsigned doesn't really matter when considering what ends up getting
encoded, as long as it is of size 32-bits. The exception would be if
sizeof(long) < 4, but libtirpc is broken for that case anyway...

> > > > 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
> 
> --
> Chuck Lever
> 
> 
> 
-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space

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




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux