This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Fix sunrpc 64-bit (especially big-endian) issues (bug 14821)
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: <libc-alpha at sourceware dot org>
- Date: Thu, 8 Nov 2012 22:58:55 +0000
- Subject: Fix sunrpc 64-bit (especially big-endian) issues (bug 14821)
In bug 14821 I note several problems in the sunrpc code (clnt_tcp.c,
clnt_udp.c, clnt_unix.c) for 64-bit, especially for big-endian.
clnttcp_control writes u_long values into a buffer at offsets both 12
and 16 (BYTES_PER_XDR_UNIT is always 4), meaning:
* When u_long is 64 bits, these values overlap.
* When u_long is 64 bits, one of them will not be 8-byte-aligned but
the processor / ABI may require such values to be 8-byte aligned.
* When u_long is 64 bits and the processor is bit-endian, the value
will be written in a different location from where the other end of
the connection, on another system, expects it (htonl deals with
getting the right byte order of the low 4 bytes, but nothing ensures
that the low 4 bytes of the value go in the right place in the
buffer).
This patch fixes these issues as follows:
* The values written into this buffer at fixed offsets are always
written as 32-bit values (in network byte order); as I understand
it, this buffer is passed over the network for clnt_tcp and
clnt_udp, so what's what seems to make sense (and even clnt_unix
would need at least to interoperate with 32-bit processes on the
same system).
* The value accessed through the "info" pointer is still treated
consistently as u_long, on the presumption that this is the correct
interface to this code. (I do note, however, that key_call.c uses
CLSET_VERS with a pointer to an int variable, which would seem wrong
both before and after this patch. I have not investigated how code
outside of glibc actually uses these interfaces.)
* There was some #if 0 code with a comment "This original code has
aliasing issues." and a replacement using memcpy. In some cases
memcpy was used to access the ct_mcall buffer; in some cases for
accesses through the "info" pointer. I'm not sure what the aliasing
issues were, or whether they were actual invalid C or just compiler
warnings. (It's not valid to access an object with effective type
char through non-character types, though it is valid to access
object with other types through the type char. But the ct_mcall
objects are probably actually allocated storage rather than having a
declared type; I don't know what their effective types end up
being. And I'd think that the info pointers should really be
pointing to u_long values.)
Although it's possible that such aliasing issues do not in fact
exist, and that with the change to 32-bit accesses to ct_mcall
alignment issues may no longer exist either, this patch leaves the
code consistently using memcpy for accesses to both places (where
previously it was inconsistent).
Tested x86_64, and verified that it allows big-endian n64 MIPS
mount.nfs to work where previously it failed. I don't know enough
about RPC to produce a self-contained testcase.
I do not know whether libtirpc has such issues or not.
2012-11-08 Joseph Myers <joseph@codesourcery.com>
[BZ #14821]
* sunrpc/clnt_tcp.c (clnttcp_control): Access values at fixed
offset in buffer as u_int32_t not u_long. Consistently use memcpy
for copies of such integer values.
* sunrpc/clnt_udp.c (clntudp_control): Likewise.
* sunrpc/clnt_unix.c (clntunix_control): Likewise.
diff --git a/sunrpc/clnt_tcp.c b/sunrpc/clnt_tcp.c
index 6bd4c8c..ec85930 100644
--- a/sunrpc/clnt_tcp.c
+++ b/sunrpc/clnt_tcp.c
@@ -364,8 +364,8 @@ static bool_t
clnttcp_control (CLIENT *cl, int request, char *info)
{
struct ct_data *ct = (struct ct_data *) cl->cl_private;
- u_long *mcall_ptr;
u_long ul;
+ u_int32_t ui32;
switch (request)
@@ -395,24 +395,15 @@ clnttcp_control (CLIENT *cl, int request, char *info)
* first element in the call structure *.
* This will get the xid of the PREVIOUS call
*/
-#if 0
- /* This original code has aliasing issues. */
- *(u_long *)info = ntohl (*(u_long *)ct->ct_mcall);
-#else
- mcall_ptr = (u_long *)ct->ct_mcall;
- ul = ntohl (*mcall_ptr);
+ memcpy (&ui32, ct->ct_mcall, sizeof (ui32));
+ ul = ntohl (ui32);
memcpy (info, &ul, sizeof (ul));
-#endif
break;
case CLSET_XID:
/* This will set the xid of the NEXT call */
-#if 0
- /* This original code has aliasing issues. */
- *(u_long *)ct->ct_mcall = htonl (*(u_long *)info - 1);
-#else
- ul = ntohl (*(u_long *)info - 1);
- memcpy (ct->ct_mcall, &ul, sizeof (ul));
-#endif
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul - 1);
+ memcpy (ct->ct_mcall, &ui32, sizeof (ui32));
/* decrement by 1 as clnttcp_call() increments once */
break;
case CLGET_VERS:
@@ -422,12 +413,14 @@ clnttcp_control (CLIENT *cl, int request, char *info)
* begining of the RPC header. MUST be changed if the
* call_struct is changed
*/
- *(u_long *)info = ntohl (*(u_long *)(ct->ct_mcall +
- 4 * BYTES_PER_XDR_UNIT));
+ memcpy (&ui32, ct->ct_mcall + 4 * BYTES_PER_XDR_UNIT, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_VERS:
- *(u_long *)(ct->ct_mcall + 4 * BYTES_PER_XDR_UNIT)
- = htonl (*(u_long *)info);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul);
+ memcpy (ct->ct_mcall + 4 * BYTES_PER_XDR_UNIT, &ui32, sizeof (ui32));
break;
case CLGET_PROG:
/*
@@ -436,12 +429,14 @@ clnttcp_control (CLIENT *cl, int request, char *info)
* begining of the RPC header. MUST be changed if the
* call_struct is changed
*/
- *(u_long *)info = ntohl(*(u_long *)(ct->ct_mcall +
- 3 * BYTES_PER_XDR_UNIT));
+ memcpy (&ui32, ct->ct_mcall + 3 * BYTES_PER_XDR_UNIT, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_PROG:
- *(u_long *)(ct->ct_mcall + 3 * BYTES_PER_XDR_UNIT)
- = htonl(*(u_long *)info);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul);
+ memcpy (ct->ct_mcall + 3 * BYTES_PER_XDR_UNIT, &ui32, sizeof (ui32));
break;
/* The following are only possible with TI-RPC */
case CLGET_RETRY_TIMEOUT:
diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c
index 7ecf2ef..8890cc6 100644
--- a/sunrpc/clnt_udp.c
+++ b/sunrpc/clnt_udp.c
@@ -547,6 +547,8 @@ static bool_t
clntudp_control (CLIENT *cl, int request, char *info)
{
struct cu_data *cu = (struct cu_data *) cl->cl_private;
+ u_long ul;
+ u_int32_t ui32;
switch (request)
{
@@ -580,11 +582,15 @@ clntudp_control (CLIENT *cl, int request, char *info)
* first element in the call structure *.
* This will get the xid of the PREVIOUS call
*/
- *(u_long *)info = ntohl(*(u_long *)cu->cu_outbuf);
+ memcpy (&ui32, cu->cu_outbuf, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_XID:
/* This will set the xid of the NEXT call */
- *(u_long *)cu->cu_outbuf = htonl(*(u_long *)info - 1);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul - 1);
+ memcpy (cu->cu_outbuf, &ui32, sizeof (ui32));
/* decrement by 1 as clntudp_call() increments once */
break;
case CLGET_VERS:
@@ -594,12 +600,14 @@ clntudp_control (CLIENT *cl, int request, char *info)
* begining of the RPC header. MUST be changed if the
* call_struct is changed
*/
- *(u_long *)info = ntohl(*(u_long *)(cu->cu_outbuf +
- 4 * BYTES_PER_XDR_UNIT));
+ memcpy (&ui32, cu->cu_outbuf + 4 * BYTES_PER_XDR_UNIT, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_VERS:
- *(u_long *)(cu->cu_outbuf + 4 * BYTES_PER_XDR_UNIT)
- = htonl(*(u_long *)info);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul);
+ memcpy (cu->cu_outbuf + 4 * BYTES_PER_XDR_UNIT, &ui32, sizeof (ui32));
break;
case CLGET_PROG:
/*
@@ -608,12 +616,14 @@ clntudp_control (CLIENT *cl, int request, char *info)
* begining of the RPC header. MUST be changed if the
* call_struct is changed
*/
- *(u_long *)info = ntohl(*(u_long *)(cu->cu_outbuf +
- 3 * BYTES_PER_XDR_UNIT));
+ memcpy (&ui32, cu->cu_outbuf + 3 * BYTES_PER_XDR_UNIT, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_PROG:
- *(u_long *)(cu->cu_outbuf + 3 * BYTES_PER_XDR_UNIT)
- = htonl(*(u_long *)info);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul);
+ memcpy (cu->cu_outbuf + 3 * BYTES_PER_XDR_UNIT, &ui32, sizeof (ui32));
break;
/* The following are only possible with TI-RPC */
case CLGET_SVC_ADDR:
diff --git a/sunrpc/clnt_unix.c b/sunrpc/clnt_unix.c
index 776ceab..9b5d7ca 100644
--- a/sunrpc/clnt_unix.c
+++ b/sunrpc/clnt_unix.c
@@ -338,8 +338,8 @@ static bool_t
clntunix_control (CLIENT *cl, int request, char *info)
{
struct ct_data *ct = (struct ct_data *) cl->cl_private;
- u_long *mcall_ptr;
u_long ul;
+ u_int32_t ui32;
switch (request)
{
@@ -367,24 +367,15 @@ clntunix_control (CLIENT *cl, int request, char *info)
* first element in the call structure *.
* This will get the xid of the PREVIOUS call
*/
-#if 0
- /* This original code has aliasing issues. */
- *(u_long *) info = ntohl (*(u_long *)ct->ct_mcall);
-#else
- mcall_ptr = (u_long *)ct->ct_mcall;
- ul = ntohl (*mcall_ptr);
+ memcpy (&ui32, ct->ct_mcall, sizeof (ui32));
+ ul = ntohl (ui32);
memcpy (info, &ul, sizeof (ul));
-#endif
break;
case CLSET_XID:
/* This will set the xid of the NEXT call */
-#if 0
- /* This original code has aliasing issues. */
- *(u_long *) ct->ct_mcall = htonl (*(u_long *)info - 1);
-#else
- ul = ntohl (*(u_long *)info - 1);
- memcpy (ct->ct_mcall, &ul, sizeof (ul));
-#endif
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul - 1);
+ memcpy (ct->ct_mcall, &ui32, sizeof (ui32));
/* decrement by 1 as clntunix_call() increments once */
break;
case CLGET_VERS:
@@ -394,12 +385,14 @@ clntunix_control (CLIENT *cl, int request, char *info)
* begining of the RPC header. MUST be changed if the
* call_struct is changed
*/
- *(u_long *) info = ntohl (*(u_long *) (ct->ct_mcall
- + 4 * BYTES_PER_XDR_UNIT));
+ memcpy (&ui32, ct->ct_mcall + 4 * BYTES_PER_XDR_UNIT, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_VERS:
- *(u_long *) (ct->ct_mcall + 4 * BYTES_PER_XDR_UNIT)
- = htonl (*(u_long *) info);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul);
+ memcpy (ct->ct_mcall + 4 * BYTES_PER_XDR_UNIT, &ui32, sizeof (ui32));
break;
case CLGET_PROG:
/*
@@ -408,12 +401,14 @@ clntunix_control (CLIENT *cl, int request, char *info)
* begining of the RPC header. MUST be changed if the
* call_struct is changed
*/
- *(u_long *) info = ntohl (*(u_long *) (ct->ct_mcall
- + 3 * BYTES_PER_XDR_UNIT));
+ memcpy (&ui32, ct->ct_mcall + 3 * BYTES_PER_XDR_UNIT, sizeof (ui32));
+ ul = ntohl (ui32);
+ memcpy (info, &ul, sizeof (ul));
break;
case CLSET_PROG:
- *(u_long *) (ct->ct_mcall + 3 * BYTES_PER_XDR_UNIT)
- = htonl(*(u_long *) info);
+ memcpy (&ul, info, sizeof (ul));
+ ui32 = htonl (ul);
+ memcpy (ct->ct_mcall + 3 * BYTES_PER_XDR_UNIT, &ui32, sizeof (ui32));
break;
/* The following are only possible with TI-RPC */
case CLGET_RETRY_TIMEOUT:
--
Joseph S. Myers
joseph@codesourcery.com