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

Message ID E69B5C56-452D-4433-83C4-FC234F01194A@bell.net
State New, archived
Headers

Commit Message

John David Anglin June 17, 2016, 10:52 p.m. UTC
  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
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.
  

Comments

Mike Frysinger June 22, 2016, 10:51 a.m. UTC | #1
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
  

Patch

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,