xdrstdio_create buffers do not output encoded values on ppc

PATCH Superseded Steve Dickson 2 participants 5 messages Jul 10, 2018

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2018-07-10 14:44 UTC #0 lore raw

PATCH v3 1/1 xdrstdio_create buffers do not output encoded values on ppc

From: Daniel Sands <[email protected]>

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 <[email protected]>

---
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(-)

src/xdr_stdio.c +11 -3

index 4410262..b902acd 100644
@@ -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);

Signature · 1 line

2.17.1

Chuck Lever

Reviewed-by on v3 1/1

2018-07-10 14:49 UTC #1 lore raw

Quoted 33 lines · Steve Dickson

=20
From: Daniel Sands <[email protected]>
=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 <[email protected]>
---
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
src/xdr_stdio.c +1 -0
index 4410262..b902acd 100644
@@ -103,10 +103,12 @@ xdrstdio_getlong(xdrs, lp)
 	XDR *xdrs;
 	long *lp;
 {
+	int32_t mycopy;
=20

src/xdr_stdio.c

Quoted 1 line

src/xdr_stdio.c +0 -1

-	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs->x_private) !=3D =

src/xdr_stdio.c

Quoted 1 line

src/xdr_stdio.c +1 -0

+	if (fread(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs->x_private) =

!=3D 1)

Quoted 8 lines

		return (FALSE);

src/xdr_stdio.c +2 -1

-	*lp =3D (long)ntohl((u_int32_t)*lp);
+
+	*lp =3D (long)ntohl(mycopy);

	return (TRUE);
}

=20

src/xdr_stdio.c xdrstdio_putlong(xdrs, lp) · line 122

Quoted 7 lines

src/xdr_stdio.c +3 -1

@@ -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)

Hi Steve, _LP64 is gcc-specific. Hope that's OK.

Reviewed-by Chuck Lever <[email protected]>

Quoted 6 lines

src/xdr_stdio.c +3 -0

+	if ((*lp > INT32_MAX) || (*lp < INT32_MIN))
+		return (FALSE);
+#endif

=20

src/xdr_stdio.c +1 -0

+	mycopy =3D (int32_t)htonl((int32_t)*lp);

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

!=3D 1)

Quoted 7 lines

		return (FALSE);
	return (TRUE);
--=20
2.17.1
=20
=20
=

--------------------------------------------------------------------------=
----

Quoted 6 lines

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever

Steve Dickson

author

2018-07-10 15:43 UTC #2 lore raw

Quoted 53 lines · Chuck Lever

Quoted 43 lines · Steve Dickson
From: Daniel Sands <[email protected]>
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 <[email protected]>
---
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(-)
src/xdr_stdio.c +4 -2
index 4410262..b902acd 100644
@@ -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);
 }
 
src/xdr_stdio.c xdrstdio_putlong(xdrs, lp) · line 122
Quoted 7 lines
src/xdr_stdio.c +3 -1
@@ -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.

Quoted 1 line

Reviewed-by Chuck Lever <[email protected]>

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.

Quoted 27 lines

Quoted 21 lines

src/xdr_stdio.c +4 -0

+	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);

Signature · 10 lines

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever

Chuck Lever

2018-07-10 15:52 UTC #3 lore raw

Quoted 10 lines · Steve Dickson

=20
=20
=20
Quoted 3 lines · Chuck Lever
=20
=20
Quoted 1 line
On Jul 10, 2018, at 10:44 AM, Steve Dickson <[email protected]> =

Quoted 23 lines · wrote:

Quoted 23 lines
Quoted 23 lines
=20
From: Daniel Sands <[email protected]>
=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 <[email protected]>
---
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
src/xdr_stdio.c +0 -1
index 4410262..b902acd 100644
--- a/src/xdr_stdio.c

Quoted 8 lines

Quoted 8 lines
src/xdr_stdio.c
Quoted 8 lines
+++ b/src/xdr_stdio.c
src/xdr_stdio.c +1 -0
@@ -103,10 +103,12 @@ xdrstdio_getlong(xdrs, lp)
 	XDR *xdrs;
 	long *lp;
 {
+	int32_t mycopy;
=20
src/xdr_stdio.c +0 -1
-	if (fread(lp, sizeof(int32_t), 1, (FILE *)xdrs->x_private) !=3D =

Quoted 1 line

Quoted 1 line
src/xdr_stdio.c
Quoted 1 line
src/xdr_stdio.c +1 -0
+	if (fread(&mycopy, sizeof(int32_t), 1, (FILE *)xdrs->x_private) =

!=3D 1)

Quoted 19 lines

Quoted 18 lines
Quoted 8 lines
		return (FALSE);
src/xdr_stdio.c +2 -1
-	*lp =3D (long)ntohl((u_int32_t)*lp);
+
+	*lp =3D (long)ntohl(mycopy);
	return (TRUE);
}
=20
src/xdr_stdio.c xdrstdio_putlong(xdrs, lp) · line 122
Quoted 7 lines
src/xdr_stdio.c +3 -1
@@ -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
Quoted 1 line
Reviewed-by Chuck Lever <[email protected]>

Quoted 2 lines

Here the reason I left it:
=

https://stackoverflow.com/questions/685124/how-to-identify-a-64-bit-build-=
on-linux-using-the-preprocessor

Quoted 2 lines

=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.

Quoted 10 lines

steved.
=20

Quoted 8 lines

=20
=20

Quoted 6 lines

src/xdr_stdio.c +3 -0

+	if ((*lp > INT32_MAX) || (*lp < INT32_MIN))
+		return (FALSE);
+#endif

=20

src/xdr_stdio.c +1 -0

+	mycopy =3D (int32_t)htonl((int32_t)*lp);

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

!=3D 1)

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
		return (FALSE);
	return (TRUE);
--=20
2.17.1
=20
=20
=

--------------------------------------------------------------------------=
----

Quoted 9 lines

Quoted 9 lines
Quoted 6 lines
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel
=20
--
Chuck Lever

--
Chuck Lever

Steve Dickson

author

2018-07-10 16:40 UTC #4 lore raw

Quoted 70 lines · Chuck Lever

Quoted 60 lines · Steve Dickson
Quoted 53 lines · Chuck Lever
Quoted 43 lines · Steve Dickson
From: Daniel Sands <[email protected]>
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 <[email protected]>
---
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(-)
src/xdr_stdio.c +4 -2
index 4410262..b902acd 100644
@@ -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);
 }
 
src/xdr_stdio.c xdrstdio_putlong(xdrs, lp) · line 122
Quoted 7 lines
src/xdr_stdio.c +3 -1
@@ -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.
Quoted 5 lines
Quoted 1 line
Reviewed-by Chuck Lever <[email protected]>
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.

Quoted 6 lines

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.

Quoted 34 lines

Quoted 28 lines

steved.

Quoted 24 lines

Quoted 21 lines

src/xdr_stdio.c +4 -0

+	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);

Signature · 10 lines

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/libtirpc-devel

--
Chuck Lever