[PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

The attached patch fixes BZ 20243.  The HEADER struct needs to be packed so that
byte accesses are used.  Tested using glibc 2.22-11on hppa.  Please install if okay.

Dave
--
John David Anglin	dave.anglin@bell.net



[-- Attachment #2: nameser_compat.h.d.txt --]
[-- Type: text/plain, Size: 624 bytes --]

2016-06-16  John David Anglin  <danglin@gcc.gnu.org>

	[BZ 20243]
	* resolv/arpa/nameser_compat.h (HEADER): Use attribute packed.

diff --git a/resolv/arpa/nameser_compat.h b/resolv/arpa/nameser_compat.h
index d59c9e4..6c0f69c 100644
--- a/resolv/arpa/nameser_compat.h
+++ b/resolv/arpa/nameser_compat.h
@@ -80,7 +80,7 @@ typedef struct {
 	unsigned	ancount :16;	/*%< number of answer entries */
 	unsigned	nscount :16;	/*%< number of authority entries */
 	unsigned	arcount :16;	/*%< number of resource entries */
-} HEADER;
+} __attribute__ ((packed)) HEADER;
 
 #define PACKETSZ	NS_PACKETSZ
 #define MAXDNAME	NS_MAXDNAME

Re: [PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[Florian Weimer]

On 06/15/2016 01:42 PM, John David Anglin wrote:
> The attached patch fixes BZ 20243.  The HEADER struct needs to be packed so that
> byte accesses are used.  Tested using glibc 2.22-11on hppa.  Please install if okay.

> -} HEADER;
> +} __attribute__ ((packed)) HEADER;

Unfortunately, we cannot apply this simple fix because this is a public 
header, and the above changes alignment of the struct.  It would also 
have to be __attribute__ ((__packed__)).

Florian

Re: [PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> On 06/15/2016 01:42 PM, John David Anglin wrote:
>> The attached patch fixes BZ 20243.  The HEADER struct needs to be packed so that
>> byte accesses are used.  Tested using glibc 2.22-11on hppa.  Please install if okay.
>
>> -} HEADER;
>> +} __attribute__ ((packed)) HEADER;
>
> Unfortunately, we cannot apply this simple fix because this is a public
> header, and the above changes alignment of the struct.  It would also have
> to be __attribute__ ((__packed__)).

It also needs to work with other compilers.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[John David Anglin]

On 2016-06-15 8:16 AM, Andreas Schwab wrote:
> Florian Weimer<fweimer@redhat.com>  writes:
>
>> >On 06/15/2016 01:42 PM, John David Anglin wrote:
>>> >>The attached patch fixes BZ 20243.  The HEADER struct needs to be packed so that
>>> >>byte accesses are used.  Tested using glibc 2.22-11on hppa.  Please install if okay.
>> >
>>> >>-} HEADER;
>>> >>+} __attribute__ ((packed)) HEADER;
>> >
>> >Unfortunately, we cannot apply this simple fix because this is a public
>> >header, and the above changes alignment of the struct.  It would also have
>> >to be __attribute__ ((__packed__)).
> It also needs to work with other compilers.

The packed attribute is used in several places in glibc.  Maybe a packed 
derivative of HEADER
can be used in res_query.c to avoid the above issue?

I don't think the packed derivative affects the layout of HEADER given 
the way the fields are
arranged.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[Florian Weimer]

On 06/15/2016 04:40 PM, John David Anglin wrote:
> On 2016-06-15 8:16 AM, Andreas Schwab wrote:
>> Florian Weimer<fweimer@redhat.com>  writes:
>>
>>> >On 06/15/2016 01:42 PM, John David Anglin wrote:
>>>> >>The attached patch fixes BZ 20243.  The HEADER struct needs to be
>>>> packed so that
>>>> >>byte accesses are used.  Tested using glibc 2.22-11on hppa.
>>>> Please install if okay.
>>> >
>>>> >>-} HEADER;
>>>> >>+} __attribute__ ((packed)) HEADER;


> I don't think the packed derivative affects the layout of HEADER given
> the way the fields are
> arranged.

I verified that it changes alignment, even on x86_64.  That's the 
intent, and it results in a layout change (if this struct is used as a 
part of other structs, for example).

Florian

Re: [PATCH] Fix misaligned access accessing HEADER struct in res_query.c

[John David Anglin]

On 2016-06-15 10:43 AM, Florian Weimer wrote:
>> I don't think the packed derivative affects the layout of HEADER given
>> the way the fields are
>> arranged.
>
> I verified that it changes alignment, even on x86_64.  That's the 
> intent, and it results in a layout change (if this struct is used as a 
> part of other structs, for example).

I'm suggesting this be done in res_query.c where we we need byte 
alignment.  Nothing else would
have access to this typedef, so it couldn't affect the layout of any 
other structs.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH v2] Fix misaligned access accessing HEADER struct in res_query.c

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

The attached patch fixes BZ 20243. It uses the gcc aligned attribute to decrease the
alignment of variables using the HEADER type.  This results in byte accesses being
used for the structure fields and thereby avoids the misaligned accesses noted in the BZ.

The change no longer affects the public interface for the HEADER type.

Tested with trunk and Debian 2.22 builds on hppa-linux.  No regressions were observed in the
test results.

Please install if okay.

Dave
--
John David Anglin	dave.anglin@bell.net



[-- Attachment #2: submitted-header.diff.txt --]
[-- Type: text/plain, Size: 6270 bytes --]

2016-06-17  John David Anglin  <danglin@gcc.gnu.org>

	[BZ 20243]
	* res_mkquery.c (HEADER1): New typedef with alignment 1.
	(res_nmkquery): Use HEADER1 instead of HEADER for pointers
	requiring byte access.
	(__res_nopt): Likewise.
	* res_query.c (HEADER1): New typedef with alignment 1.
	(__libc_res_nquery): Use HEADER1 instead of HEADER for pointers
	requiring byte access.
	(__libc_res_nsearch): Likewise.
	* res_send.c (HEADER1): New typedef with alignment 1.
	(res_nameinquery): Use HEADER1 instead of HEADER for pointers
	requiring byte access.
	(res_queriesmatch): Likewise.
	(send_vc): Likewise.
	(send_dg): Likewise.

diff --git a/resolv/res_mkquery.c b/resolv/res_mkquery.c
index 12f9730..994e7f8 100644
--- a/resolv/res_mkquery.c
+++ b/resolv/res_mkquery.c
@@ -83,6 +83,8 @@
 # define RANDOM_BITS(Var) { uint64_t v64; HP_TIMING_NOW (v64); Var = v64; }
 #endif
 
+typedef HEADER __attribute__ ((aligned(1))) HEADER1;
+
 /*
  * Form all types of queries.
  * Returns the size of the result or -1.
@@ -98,7 +100,7 @@ res_nmkquery(res_state statp,
 	     u_char *buf,		/* buffer to put query */
 	     int buflen)		/* size of buffer */
 {
-	HEADER *hp;
+	HEADER1 *hp;
 	u_char *cp;
 	int n;
 	u_char *dnptrs[20], **dpp, **lastdnptr;
@@ -114,7 +116,7 @@ res_nmkquery(res_state statp,
 	if ((buf == NULL) || (buflen < HFIXEDSZ))
 		return (-1);
 	memset(buf, 0, HFIXEDSZ);
-	hp = (HEADER *) buf;
+	hp = (HEADER1 *) buf;
 	/* We randomize the IDs every time.  The old code just
 	   incremented by one after the initial randomization which
 	   still predictable if the application does multiple
@@ -229,7 +231,7 @@ __res_nopt(res_state statp,
 		printf(";; res_nopt()\n");
 #endif
 
-	HEADER *hp = (HEADER *) buf;
+	HEADER1 *hp = (HEADER1 *) buf;
 	u_char *cp = buf + n0;
 	u_char *ep = buf + buflen;
 
diff --git a/resolv/res_query.c b/resolv/res_query.c
index 944d1a9..f8b02a8 100644
--- a/resolv/res_query.c
+++ b/resolv/res_query.c
@@ -78,6 +78,8 @@
 #include <stdlib.h>
 #include <string.h>
 
+typedef HEADER __attribute__ ((aligned(1))) HEADER1;
+
 /* Options.  Leave them on. */
 /* #undef DEBUG */
 
@@ -117,8 +119,8 @@ __libc_res_nquery(res_state statp,
 		  int *resplen2,
 		  int *answerp2_malloced)
 {
-	HEADER *hp = (HEADER *) answer;
-	HEADER *hp2;
+	HEADER1 *hp = (HEADER *) answer;
+	HEADER1 *hp2;
 	int n, use_malloc = 0;
 	u_int oflags = statp->_flags;
 
@@ -235,7 +237,7 @@ __libc_res_nquery(res_state statp,
 
 	if (answerp != NULL)
 	  /* __libc_res_nsend might have reallocated the buffer.  */
-	  hp = (HEADER *) *answerp;
+	  hp = (HEADER1 *) *answerp;
 
 	/* We simplify the following tests by assigning HP to HP2 or
 	   vice versa.  It is easy to verify that this is the same as
@@ -246,7 +248,7 @@ __libc_res_nquery(res_state statp,
 	  }
 	else
 	  {
-	    hp2 = (HEADER *) *answerp2;
+	    hp2 = (HEADER1 *) *answerp2;
 	    if (n < (int) sizeof (HEADER))
 	      {
 	        hp = hp2;
@@ -336,7 +338,7 @@ __libc_res_nsearch(res_state statp,
 		   int *answerp2_malloced)
 {
 	const char *cp, * const *domain;
-	HEADER *hp = (HEADER *) answer;
+	HEADER1 *hp = (HEADER1 *) answer;
 	char tmp[NS_MAXDNAME];
 	u_int dots;
 	int trailing_dot, ret, saved_herrno;
diff --git a/resolv/res_send.c b/resolv/res_send.c
index 869294f..5583323 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -110,6 +110,8 @@
 #include <kernel-features.h>
 #include <libc-internal.h>
 
+typedef HEADER __attribute__ ((aligned(1))) HEADER1;
+
 #if PACKETSZ > 65536
 #define MAXPACKET       PACKETSZ
 #else
@@ -269,7 +271,7 @@ res_nameinquery(const char *name, int type, int class,
 		const u_char *buf, const u_char *eom)
 {
 	const u_char *cp = buf + HFIXEDSZ;
-	int qdcount = ntohs(((HEADER*)buf)->qdcount);
+	int qdcount = ntohs(((HEADER1*)buf)->qdcount);
 
 	while (qdcount-- > 0) {
 		char tname[MAXDNAME+1];
@@ -313,15 +315,15 @@ res_queriesmatch(const u_char *buf1, const u_char *eom1,
 	 * Only header section present in replies to
 	 * dynamic update packets.
 	 */
-	if ((((HEADER *)buf1)->opcode == ns_o_update) &&
-	    (((HEADER *)buf2)->opcode == ns_o_update))
+	if ((((HEADER1 *)buf1)->opcode == ns_o_update) &&
+	    (((HEADER1 *)buf2)->opcode == ns_o_update))
 		return (1);
 
 	/* Note that we initially do not convert QDCOUNT to the host byte
 	   order.  We can compare it with the second buffer's QDCOUNT
 	   value without doing this.  */
-	int qdcount = ((HEADER*)buf1)->qdcount;
-	if (qdcount != ((HEADER*)buf2)->qdcount)
+	int qdcount = ((HEADER1*)buf1)->qdcount;
+	if (qdcount != ((HEADER1*)buf2)->qdcount)
 		return (0);
 
 	qdcount = htons (qdcount);
@@ -734,9 +736,9 @@ send_vc(res_state statp,
 	int *terrno, int ns, u_char **anscp, u_char **ansp2, int *anssizp2,
 	int *resplen2, int *ansp2_malloced)
 {
-	const HEADER *hp = (HEADER *) buf;
-	const HEADER *hp2 = (HEADER *) buf2;
-	HEADER *anhp = (HEADER *) *ansp;
+	const HEADER1 *hp = (HEADER1 *) buf;
+	const HEADER1 *hp2 = (HEADER1 *) buf2;
+	HEADER1 *anhp = (HEADER1 *) *ansp;
 	struct sockaddr *nsap = get_nsaddr (statp, ns);
 	int truncating, connreset, n;
 	/* On some architectures compiler might emit a warning indicating
@@ -873,7 +875,7 @@ send_vc(res_state statp,
 		thisansp = ansp2;
 		thisresplenp = resplen2;
 	}
-	anhp = (HEADER *) *thisansp;
+	anhp = (HEADER1 *) *thisansp;
 
 	*thisresplenp = rlen;
 	/* Is the answer buffer too small?  */
@@ -1092,8 +1094,8 @@ send_dg(res_state statp,
 	int *terrno, int ns, int *v_circuit, int *gotsomewhere, u_char **anscp,
 	u_char **ansp2, int *anssizp2, int *resplen2, int *ansp2_malloced)
 {
-	const HEADER *hp = (HEADER *) buf;
-	const HEADER *hp2 = (HEADER *) buf2;
+	const HEADER1 *hp = (HEADER1 *) buf;
+	const HEADER1 *hp2 = (HEADER1 *) buf2;
 	struct timespec now, timeout, finish;
 	struct pollfd pfd[1];
 	int ptimeout;
@@ -1343,7 +1345,7 @@ send_dg(res_state statp,
 			);
 		}
 
-		HEADER *anhp = (HEADER *) *thisansp;
+		HEADER1 *anhp = (HEADER1 *) *thisansp;
 		socklen_t fromlen = sizeof(struct sockaddr_in6);
 		assert (sizeof(from) <= fromlen);
 		*thisresplenp = recvfrom(pfd[0].fd, (char*)*thisansp,

Re: [PATCH v2] Fix misaligned access accessing HEADER struct in res_query.c

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On 17 Jun 2016 18:52, John David Anglin wrote:
> --- a/resolv/res_mkquery.c
> +++ b/resolv/res_mkquery.c
> @@ -83,6 +83,8 @@
>  # define RANDOM_BITS(Var) { uint64_t v64; HP_TIMING_NOW (v64); Var = v64; }
>  #endif
>  
> +typedef HEADER __attribute__ ((aligned(1))) HEADER1;

could do with a comment above it explaining what this is all about

should be __aligned__

bike shed: maybe "UHEADER" is better ?

code-wise, should this file always be using this variant ?  would
it be too ugly to do:
	typedef ...
	#define HEADER HEADER1
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3] Fix misaligned accesses to fields in HEADER struct defined in <arpa/nameser_compat.h>

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

On 2016-06-22, at 6:51 AM, Mike Frysinger wrote:

> On 17 Jun 2016 18:52, John David Anglin wrote:
>> --- a/resolv/res_mkquery.c
>> +++ b/resolv/res_mkquery.c
>> @@ -83,6 +83,8 @@
>> # define RANDOM_BITS(Var) { uint64_t v64; HP_TIMING_NOW (v64); Var = v64; }
>> #endif
>> 
>> +typedef HEADER __attribute__ ((aligned(1))) HEADER1;
> 
> could do with a comment above it explaining what this is all about
> 
> should be __aligned__
> 
> bike shed: maybe "UHEADER" is better ?
> 
> code-wise, should this file always be using this variant ?  would
> it be too ugly to do:
> 	typedef ...
> 	#define HEADER HEADER1

The attached patch implements the above suggestions except for res_query.c where
is directly substituted as necessary.

Tested on hppa-unknown-linux-gnu with a trunk build and check.

Please install if okay.

Dave
--
John David Anglin	dave.anglin@bell.net



[-- Attachment #2: submitted-header-v3.diff.txt --]
[-- Type: text/plain, Size: 3908 bytes --]

2016-06-25  John David Anglin  <danglin@gcc.gnu.org>

        [BZ 20243]
        * res_mkquery.c (UHEADER): New typedef derived from HEADER typedef.
	(HEADER): Define.
        * res_send.c: Likewise.
        * res_query.c (UHEADER): New typedef derived from HEADER typedef.
        (__libc_res_nquery): Use UHEADER typedef instead of HEADER for pointers
        requiring byte access.
        (__libc_res_nsearch): Likewise.

diff --git a/resolv/res_mkquery.c b/resolv/res_mkquery.c
index 12f9730..1c4c0bc 100644
--- a/resolv/res_mkquery.c
+++ b/resolv/res_mkquery.c
@@ -83,6 +83,15 @@
 # define RANDOM_BITS(Var) { uint64_t v64; HP_TIMING_NOW (v64); Var = v64; }
 #endif
 
+/* The structure HEADER is normally aligned to a word boundary and its
+   fields are accessed using word loads and stores.  We need to access
+   this structure when it is aligned on a byte boundary.  This can cause
+   problems on machines with strict alignment.  So, we create a new
+   typedef to reduce its alignment to one.  This ensures the fields are
+   accessed with byte loads and stores.  */
+typedef HEADER __attribute__ ((__aligned__(1))) UHEADER;
+#define HEADER UHEADER
+
 /*
  * Form all types of queries.
  * Returns the size of the result or -1.
diff --git a/resolv/res_query.c b/resolv/res_query.c
index 944d1a9..ebf0fb9 100644
--- a/resolv/res_query.c
+++ b/resolv/res_query.c
@@ -78,6 +78,14 @@
 #include <stdlib.h>
 #include <string.h>
 
+/* The structure HEADER is normally aligned to a word boundary and its
+   fields are accessed using word loads and stores.  We need to access 
+   this structure when it is aligned on a byte boundary.  This can cause
+   problems on machines with strict alignment.  So, we create a new
+   typedef to reduce its alignment to one.  This ensures the fields are
+   accessed with byte loads and stores.  */
+typedef HEADER __attribute__ ((__aligned__(1))) UHEADER;
+
 /* Options.  Leave them on. */
 /* #undef DEBUG */
 
@@ -117,8 +125,8 @@ __libc_res_nquery(res_state statp,
 		  int *resplen2,
 		  int *answerp2_malloced)
 {
-	HEADER *hp = (HEADER *) answer;
-	HEADER *hp2;
+	UHEADER *hp = (UHEADER *) answer;
+	UHEADER *hp2;
 	int n, use_malloc = 0;
 	u_int oflags = statp->_flags;
 
@@ -235,7 +243,7 @@ __libc_res_nquery(res_state statp,
 
 	if (answerp != NULL)
 	  /* __libc_res_nsend might have reallocated the buffer.  */
-	  hp = (HEADER *) *answerp;
+	  hp = (UHEADER *) *answerp;
 
 	/* We simplify the following tests by assigning HP to HP2 or
 	   vice versa.  It is easy to verify that this is the same as
@@ -246,7 +254,7 @@ __libc_res_nquery(res_state statp,
 	  }
 	else
 	  {
-	    hp2 = (HEADER *) *answerp2;
+	    hp2 = (UHEADER *) *answerp2;
 	    if (n < (int) sizeof (HEADER))
 	      {
 	        hp = hp2;
@@ -336,7 +344,7 @@ __libc_res_nsearch(res_state statp,
 		   int *answerp2_malloced)
 {
 	const char *cp, * const *domain;
-	HEADER *hp = (HEADER *) answer;
+	UHEADER *hp = (UHEADER *) answer;
 	char tmp[NS_MAXDNAME];
 	u_int dots;
 	int trailing_dot, ret, saved_herrno;
diff --git a/resolv/res_send.c b/resolv/res_send.c
index 869294f..da075af 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -110,6 +110,15 @@
 #include <kernel-features.h>
 #include <libc-internal.h>
 
+/* The structure HEADER is normally aligned to a word boundary and its
+   fields are accessed using word loads and stores.  We need to access 
+   this structure when it is aligned on a byte boundary.  This can cause
+   problems on machines with strict alignment.  So, we create a new
+   typedef to reduce its alignment to one.  This ensures the fields are
+   accessed with byte loads and stores.  */
+typedef HEADER __attribute__ ((__aligned__(1))) UHEADER;
+#define HEADER UHEADER
+
 #if PACKETSZ > 65536
 #define MAXPACKET       PACKETSZ
 #else