diff mbox

xdrstdio_create buffers do not output encoded values on ppc

Message ID 20180628152914.7445-1-steved@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve Dickson June 28, 2018, 3:29 p.m. UTC 
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>
---
 src/xdr_stdio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Trond Myklebust June 28, 2018, 4:17 p.m. UTC | #1 
On Thu, 2018-06-28 at 11:29 -0400, Steve Dickson 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>

> ---

>  src/xdr_stdio.c | 10 +++++-----

>  1 file changed, 5 insertions(+), 5 deletions(-)

> 

> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c

> index 4410262..b415f61 100644

> --- a/src/xdr_stdio.c

> +++ b/src/xdr_stdio.c

> @@ -103,10 +103,10 @@ xdrstdio_getlong(xdrs, lp)

>  	XDR *xdrs;

>  	long *lp;

>  {

> -

> -	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs->x_private)

> != 1)

> +	u_int32_t mycopy;

> +	if (fread(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs-

> >x_private) != 1)

>  		return (FALSE);

> -	*lp = (long)ntohl((u_int32_t)*lp);

> +	*lp = (long)ntohl(mycopy);

>  	return (TRUE);

>  }

>  

> @@ -115,9 +115,9 @@ xdrstdio_putlong(xdrs, lp)

>  	XDR *xdrs;

>  	const long *lp;

>  {

> -	long mycopy = (long)htonl((u_int32_t)*lp);

> +	u_int32_t mycopy = (u_int32_t)htonl((u_int32_t)*lp);

>  

> -	if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs-

> >x_private) != 1)

> +	if (fwrite(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs-

> >x_private) != 1)

>  		return (FALSE);

>  	return (TRUE);

>  }


Hmm... Given that most setups today tend to be 64-bit, shouldn't there
also be bounds checking in the above 'xdrstdio_putlong()' in order to
make it safe?

Something like

if ((long)(u_int32_t)*lp != *lp) return (FALSE);

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
Steve Dickson June 28, 2018, 4:40 p.m. UTC | #2 
On 06/28/2018 12:17 PM, Trond Myklebust wrote:
> On Thu, 2018-06-28 at 11:29 -0400, Steve Dickson 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>
>> ---
>>  src/xdr_stdio.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c
>> index 4410262..b415f61 100644
>> --- a/src/xdr_stdio.c
>> +++ b/src/xdr_stdio.c
>> @@ -103,10 +103,10 @@ xdrstdio_getlong(xdrs, lp)
>>  	XDR *xdrs;
>>  	long *lp;
>>  {
>> -
>> -	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs->x_private)
>> != 1)
>> +	u_int32_t mycopy;
>> +	if (fread(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs-
>>> x_private) != 1)
>>  		return (FALSE);
>> -	*lp = (long)ntohl((u_int32_t)*lp);
>> +	*lp = (long)ntohl(mycopy);
>>  	return (TRUE);
>>  }
>>  
>> @@ -115,9 +115,9 @@ xdrstdio_putlong(xdrs, lp)
>>  	XDR *xdrs;
>>  	const long *lp;
>>  {
>> -	long mycopy = (long)htonl((u_int32_t)*lp);
>> +	u_int32_t mycopy = (u_int32_t)htonl((u_int32_t)*lp);
>>  
>> -	if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs-
>>> x_private) != 1)
>> +	if (fwrite(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs-
>>> x_private) != 1)
>>  		return (FALSE);
>>  	return (TRUE);
>>  }
> 
> Hmm... Given that most setups today tend to be 64-bit, shouldn't there
> also be bounds checking in the above 'xdrstdio_putlong()' in order to
> make it safe?
> 
> Something like
> 
> if ((long)(u_int32_t)*lp != *lp) return (FALSE);
> 
Sorry... I'm not following this... why is this necessary
and what are you making safe?

steved.
--
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
Trond Myklebust June 29, 2018, 1:47 a.m. UTC | #3 
On Thu, 2018-06-28 at 12:40 -0400, Steve Dickson wrote:
> 

> On 06/28/2018 12:17 PM, Trond Myklebust wrote:

> > On Thu, 2018-06-28 at 11:29 -0400, Steve Dickson 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>

> > > ---

> > >  src/xdr_stdio.c | 10 +++++-----

> > >  1 file changed, 5 insertions(+), 5 deletions(-)

> > > 

> > > diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c

> > > index 4410262..b415f61 100644

> > > --- a/src/xdr_stdio.c

> > > +++ b/src/xdr_stdio.c

> > > @@ -103,10 +103,10 @@ xdrstdio_getlong(xdrs, lp)

> > >  	XDR *xdrs;

> > >  	long *lp;

> > >  {

> > > -

> > > -	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs-

> > > >x_private)

> > > != 1)

> > > +	u_int32_t mycopy;

> > > +	if (fread(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs-

> > > > x_private) != 1)

> > > 

> > >  		return (FALSE);

> > > -	*lp = (long)ntohl((u_int32_t)*lp);

> > > +	*lp = (long)ntohl(mycopy);

> > >  	return (TRUE);

> > >  }

> > >  

> > > @@ -115,9 +115,9 @@ xdrstdio_putlong(xdrs, lp)

> > >  	XDR *xdrs;

> > >  	const long *lp;

> > >  {

> > > -	long mycopy = (long)htonl((u_int32_t)*lp);

> > > +	u_int32_t mycopy = (u_int32_t)htonl((u_int32_t)*lp);

> > >  

> > > -	if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs-

> > > > x_private) != 1)

> > > 

> > > +	if (fwrite(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs-

> > > > x_private) != 1)

> > > 

> > >  		return (FALSE);

> > >  	return (TRUE);

> > >  }

> > 

> > Hmm... Given that most setups today tend to be 64-bit, shouldn't

> > there

> > also be bounds checking in the above 'xdrstdio_putlong()' in order

> > to

> > make it safe?

> > 

> > Something like

> > 

> > if ((long)(u_int32_t)*lp != *lp) return (FALSE);

> > 

> 

> Sorry... I'm not following this... why is this necessary

> and what are you making safe?

> 

A long integer on most 64-bit systems is 64-bit long, but in the code
above, you are pushing that value into a 32-bit big-endian integer. So
a safe implementation would normally check for whether or not the 64-
bit value is getting truncated when it gets cast to u_int32_t, and
would throw an error in the case where the long really does not fit
into that 32-bit integer.

-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space
diff mbox

Patch

diff --git a/src/xdr_stdio.c b/src/xdr_stdio.c
index 4410262..b415f61 100644
--- a/src/xdr_stdio.c
+++ b/src/xdr_stdio.c
@@ -103,10 +103,10 @@  xdrstdio_getlong(xdrs, lp)
 	XDR *xdrs;
 	long *lp;
 {
-
-	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs->x_private) != 1)
+	u_int32_t mycopy;
+	if (fread(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs->x_private) != 1)
 		return (FALSE);
-	*lp = (long)ntohl((u_int32_t)*lp);
+	*lp = (long)ntohl(mycopy);
 	return (TRUE);
 }
 
@@ -115,9 +115,9 @@  xdrstdio_putlong(xdrs, lp)
 	XDR *xdrs;
 	const long *lp;
 {
-	long mycopy = (long)htonl((u_int32_t)*lp);
+	u_int32_t mycopy = (u_int32_t)htonl((u_int32_t)*lp);
 
-	if (fwrite(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs->x_private) != 1)
+	if (fwrite(&mycopy, sizeof(u_int32_t), 1, (FILE *)xdrs->x_private) != 1)
 		return (FALSE);
 	return (TRUE);
 }