diff mbox

Simplify handling of nameserver configuration in resolver

Message ID mvmiobztql1.fsf@hawking.suse.de
State Committed
Headers

Commit Message

Andreas Schwab May 11, 2015, 9:23 a.m. UTC 
  Much of the IPv6 support in resolv.conf is overly convoluted and hard to
understand, proven by the fact that trying to fix something in the past
has always (re-)introduced bugs in other corners.  This patch removes
the use of the ext.nsmap member of struct __res_state and always uses
an identity mapping betwen the nsaddr_list array and the ext.nsaddrs
array.  The fact that a nameserver has an IPv6 address is signalled by
setting nsaddr_list[].sin_family to zero.

This has been suceessfully tested for some time in openSUSE Factory.

Andreas.

	[BZ #13028]
	[BZ #17053]
	* resolv/res_init.c (__res_vinit): Remove use of ext.nsmap member
	of struct __res_state.
	* resolv/res_send.c (__libc_res_nsend): Likewise.
	(get_nsaddr): New function.
	(res_ourserver_p, send_vc, reopen): Use it instead of accessing
	statp directly.
---
 resolv/res_init.c |  47 +++++-----------
 resolv/res_send.c | 164 +++++++++++++++++++++++-------------------------------
 2 files changed, 84 insertions(+), 127 deletions(-)

Comments

Siddhesh Poyarekar May 21, 2015, 5:07 a.m. UTC | #1 
On Mon, May 11, 2015 at 11:23:54AM +0200, Andreas Schwab wrote:
> Much of the IPv6 support in resolv.conf is overly convoluted and hard to
> understand, proven by the fact that trying to fix something in the past
> has always (re-)introduced bugs in other corners.  This patch removes
> the use of the ext.nsmap member of struct __res_state and always uses
> an identity mapping betwen the nsaddr_list array and the ext.nsaddrs
> array.  The fact that a nameserver has an IPv6 address is signalled by
> setting nsaddr_list[].sin_family to zero.
> 
> This has been suceessfully tested for some time in openSUSE Factory.
> 
> Andreas.
> 
> 	[BZ #13028]
> 	[BZ #17053]
> 	* resolv/res_init.c (__res_vinit): Remove use of ext.nsmap member
> 	of struct __res_state.
> 	* resolv/res_send.c (__libc_res_nsend): Likewise.
> 	(get_nsaddr): New function.
> 	(res_ourserver_p, send_vc, reopen): Use it instead of accessing
> 	statp directly.

Thank you for doing this.  Looks good to me.

Siddhesh
diff mbox

Patch

diff --git a/resolv/res_init.c b/resolv/res_init.c
index 553ba12..66561ff 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -153,10 +153,8 @@  __res_vinit(res_state statp, int preinit) {
 	char *cp, **pp;
 	int n;
 	char buf[BUFSIZ];
-	int nserv = 0;    /* number of IPv4 nameservers read from file */
-#ifdef _LIBC
-	int nservall = 0; /* number of (IPv4 + IPV6) nameservers read from file */
-#endif
+	int nserv = 0;    /* number of nameservers read from file */
+	int have_serv6 = 0;
 	int haveenv = 0;
 	int havesearch = 0;
 #ifdef RESOLVSORT
@@ -184,15 +182,9 @@  __res_vinit(res_state statp, int preinit) {
 	statp->_flags = 0;
 	statp->qhook = NULL;
 	statp->rhook = NULL;
-	statp->_u._ext.nsinit = 0;
 	statp->_u._ext.nscount = 0;
-#ifdef _LIBC
-	statp->_u._ext.nscount6 = 0;
-	for (n = 0; n < MAXNS; n++) {
-		statp->_u._ext.nsaddrs[n] = NULL;
-		statp->_u._ext.nsmap[n] = MAXNS;
-	}
-#endif
+	for (n = 0; n < MAXNS; n++)
+	    statp->_u._ext.nsaddrs[n] = NULL;
 
 	/* Allow user to override the local domain definition */
 	if ((cp = getenv("LOCALDOMAIN")) != NULL) {
@@ -296,11 +288,7 @@  __res_vinit(res_state statp, int preinit) {
 		    continue;
 		}
 		/* read nameservers to query */
-#ifdef _LIBC
-		if (MATCH(buf, "nameserver") && nservall < MAXNS) {
-#else
 		if (MATCH(buf, "nameserver") && nserv < MAXNS) {
-#endif
 		    struct in_addr a;
 
 		    cp = buf + sizeof("nameserver") - 1;
@@ -308,13 +296,12 @@  __res_vinit(res_state statp, int preinit) {
 			cp++;
 		    if ((*cp != '\0') && (*cp != '\n')
 			&& __inet_aton(cp, &a)) {
-			statp->nsaddr_list[nservall].sin_addr = a;
-			statp->nsaddr_list[nservall].sin_family = AF_INET;
-			statp->nsaddr_list[nservall].sin_port =
+			statp->nsaddr_list[nserv].sin_addr = a;
+			statp->nsaddr_list[nserv].sin_family = AF_INET;
+			statp->nsaddr_list[nserv].sin_port =
 				htons(NAMESERVER_PORT);
 			nserv++;
 #ifdef _LIBC
-			nservall++;
 		    } else {
 			struct in6_addr a6;
 			char *el;
@@ -356,10 +343,11 @@  __res_vinit(res_state statp, int preinit) {
 				    }
 				}
 
-				statp->_u._ext.nsaddrs[nservall] = sa6;
-				statp->_u._ext.nssocks[nservall] = -1;
-				statp->_u._ext.nsmap[nservall] = MAXNS + 1;
-				nservall++;
+				statp->nsaddr_list[nserv].sin_family = 0;
+				statp->_u._ext.nsaddrs[nserv] = sa6;
+				statp->_u._ext.nssocks[nserv] = -1;
+				have_serv6 = 1;
+				nserv++;
 			    }
 			}
 #endif
@@ -414,10 +402,9 @@  __res_vinit(res_state statp, int preinit) {
 		    continue;
 		}
 	    }
-	    statp->nscount = nservall;
+	    statp->nscount = nserv;
 #ifdef _LIBC
-	    if (nservall - nserv > 0) {
-		statp->_u._ext.nscount6 = nservall - nserv;
+	    if (have_serv6) {
 		/* We try IPv6 servers again.  */
 		statp->ipv6_unavail = false;
 	    }
@@ -606,11 +593,7 @@  __res_iclose(res_state statp, bool free_addr) {
 		statp->_vcsock = -1;
 		statp->_flags &= ~(RES_F_VC | RES_F_CONN);
 	}
-#ifdef _LIBC
-	for (ns = 0; ns < MAXNS; ns++)
-#else
 	for (ns = 0; ns < statp->_u._ext.nscount; ns++)
-#endif
 		if (statp->_u._ext.nsaddrs[ns]) {
 			if (statp->_u._ext.nssocks[ns] != -1) {
 				close_not_cancel_no_status(statp->_u._ext.nssocks[ns]);
@@ -621,8 +604,6 @@  __res_iclose(res_state statp, bool free_addr) {
 				statp->_u._ext.nsaddrs[ns] = NULL;
 			}
 		}
-	if (free_addr)
-		statp->_u._ext.nsinit = 0;
 }
 libc_hidden_def (__res_iclose)
 
diff --git a/resolv/res_send.c b/resolv/res_send.c
index c35fb66..5e53cc2 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -184,6 +184,7 @@  evNowTime(struct timespec *res) {
 
 /* Forward. */
 
+static struct sockaddr *get_nsaddr (res_state, int);
 static int		send_vc(res_state, const u_char *, int,
 				const u_char *, int,
 				u_char **, int *, int *, int, u_char **,
@@ -221,20 +222,21 @@  res_ourserver_p(const res_state statp, const struct sockaddr_in6 *inp)
 	    in_port_t port = in4p->sin_port;
 	    in_addr_t addr = in4p->sin_addr.s_addr;
 
-	    for (ns = 0;  ns < MAXNS;  ns++) {
+	    for (ns = 0;  ns < statp->nscount;  ns++) {
 		const struct sockaddr_in *srv =
-		    (struct sockaddr_in *)EXT(statp).nsaddrs[ns];
+		    (struct sockaddr_in *) get_nsaddr (statp, ns);
 
-		if ((srv != NULL) && (srv->sin_family == AF_INET) &&
+		if ((srv->sin_family == AF_INET) &&
 		    (srv->sin_port == port) &&
 		    (srv->sin_addr.s_addr == INADDR_ANY ||
 		     srv->sin_addr.s_addr == addr))
 		    return (1);
 	    }
 	} else if (inp->sin6_family == AF_INET6) {
-	    for (ns = 0;  ns < MAXNS;  ns++) {
-		const struct sockaddr_in6 *srv = EXT(statp).nsaddrs[ns];
-		if ((srv != NULL) && (srv->sin6_family == AF_INET6) &&
+	    for (ns = 0;  ns < statp->nscount;  ns++) {
+		const struct sockaddr_in6 *srv
+		  = (struct sockaddr_in6 *) get_nsaddr (statp, ns);
+		if ((srv->sin6_family == AF_INET6) &&
 		    (srv->sin6_port == inp->sin6_port) &&
 		    !(memcmp(&srv->sin6_addr, &in6addr_any,
 			     sizeof (struct in6_addr)) &&
@@ -384,80 +386,48 @@  __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 	 * If the ns_addr_list in the resolver context has changed, then
 	 * invalidate our cached copy and the associated timing data.
 	 */
-	if (EXT(statp).nsinit) {
+	if (EXT(statp).nscount != 0) {
 		int needclose = 0;
 
 		if (EXT(statp).nscount != statp->nscount)
 			needclose++;
 		else
-			for (ns = 0; ns < MAXNS; ns++) {
-				unsigned int map = EXT(statp).nsmap[ns];
-				if (map < MAXNS
+			for (ns = 0; ns < statp->nscount; ns++) {
+				if (statp->nsaddr_list[ns].sin_family != 0
 				    && !sock_eq((struct sockaddr_in6 *)
-						&statp->nsaddr_list[map],
+						&statp->nsaddr_list[ns],
 						EXT(statp).nsaddrs[ns]))
 				{
 					needclose++;
 					break;
 				}
 			}
-		if (needclose)
+		if (needclose) {
 			__res_iclose(statp, false);
+			EXT(statp).nscount = 0;
+		}
 	}
 
 	/*
 	 * Maybe initialize our private copy of the ns_addr_list.
 	 */
-	if (EXT(statp).nsinit == 0) {
-		unsigned char map[MAXNS];
-
-		memset (map, MAXNS, sizeof (map));
-		for (n = 0; n < MAXNS; n++) {
-			ns = EXT(statp).nsmap[n];
-			if (ns < statp->nscount)
-				map[ns] = n;
-			else if (ns < MAXNS) {
-				free(EXT(statp).nsaddrs[n]);
-				EXT(statp).nsaddrs[n] = NULL;
-				EXT(statp).nsmap[n] = MAXNS;
-			}
-		}
-		n = statp->nscount;
-		if (statp->nscount > EXT(statp).nscount)
-			for (n = EXT(statp).nscount, ns = 0;
-			     n < statp->nscount; n++) {
-				while (ns < MAXNS
-				       && EXT(statp).nsmap[ns] != MAXNS)
-					ns++;
-				if (ns == MAXNS)
-					break;
-				/* NS never exceeds MAXNS, but gcc 4.9 somehow
-				   does not see this.  */
-				DIAG_PUSH_NEEDS_COMMENT;
-				DIAG_IGNORE_NEEDS_COMMENT (4.9,
-							   "-Warray-bounds");
-				EXT(statp).nsmap[ns] = n;
-				DIAG_POP_NEEDS_COMMENT;
-				map[n] = ns++;
-			}
-		EXT(statp).nscount = n;
-		for (ns = 0; ns < EXT(statp).nscount; ns++) {
-			n = map[ns];
-			if (EXT(statp).nsaddrs[n] == NULL)
-				EXT(statp).nsaddrs[n] =
+	if (EXT(statp).nscount == 0) {
+		for (ns = 0; ns < statp->nscount; ns++) {
+			EXT(statp).nssocks[ns] = -1;
+			if (statp->nsaddr_list[ns].sin_family == 0)
+				continue;
+			if (EXT(statp).nsaddrs[ns] == NULL)
+				EXT(statp).nsaddrs[ns] =
 				    malloc(sizeof (struct sockaddr_in6));
-			if (EXT(statp).nsaddrs[n] != NULL) {
-				memset (mempcpy(EXT(statp).nsaddrs[n],
+			if (EXT(statp).nsaddrs[ns] != NULL)
+				memset (mempcpy(EXT(statp).nsaddrs[ns],
 						&statp->nsaddr_list[ns],
 						sizeof (struct sockaddr_in)),
 					'\0',
 					sizeof (struct sockaddr_in6)
 					- sizeof (struct sockaddr_in));
-				EXT(statp).nssocks[n] = -1;
-				n++;
-			}
 		}
-		EXT(statp).nsinit = 1;
+		EXT(statp).nscount = statp->nscount;
 	}
 
 	/*
@@ -466,44 +436,37 @@  __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 	 */
 	if (__builtin_expect ((statp->options & RES_ROTATE) != 0, 0) &&
 	    (statp->options & RES_BLAST) == 0) {
-		struct sockaddr_in6 *ina;
-		unsigned int map;
-
-		n = 0;
-		while (n < MAXNS && EXT(statp).nsmap[n] == MAXNS)
-			n++;
-		if (n < MAXNS) {
-			ina = EXT(statp).nsaddrs[n];
-			map = EXT(statp).nsmap[n];
-			for (;;) {
-				ns = n + 1;
-				while (ns < MAXNS
-				       && EXT(statp).nsmap[ns] == MAXNS)
-					ns++;
-				if (ns == MAXNS)
-					break;
-				EXT(statp).nsaddrs[n] = EXT(statp).nsaddrs[ns];
-				EXT(statp).nsmap[n] = EXT(statp).nsmap[ns];
-				n = ns;
-			}
-			EXT(statp).nsaddrs[n] = ina;
-			EXT(statp).nsmap[n] = map;
+		struct sockaddr_in ina;
+		struct sockaddr_in6 *inp;
+		int lastns = statp->nscount - 1;
+		int fd;
+
+		inp = EXT(statp).nsaddrs[0];
+		ina = statp->nsaddr_list[0];
+		fd = EXT(statp).nssocks[0];
+		for (ns = 0; ns < lastns; ns++) {
+		    EXT(statp).nsaddrs[ns] = EXT(statp).nsaddrs[ns + 1];
+		    statp->nsaddr_list[ns] = statp->nsaddr_list[ns + 1];
+		    EXT(statp).nssocks[ns] = EXT(statp).nssocks[ns + 1];
 		}
+		EXT(statp).nsaddrs[lastns] = inp;
+		statp->nsaddr_list[lastns] = ina;
+		EXT(statp).nssocks[lastns] = fd;
 	}
 
 	/*
 	 * Send request, RETRY times, or until successful.
 	 */
 	for (try = 0; try < statp->retry; try++) {
-	    for (ns = 0; ns < MAXNS; ns++)
+	    for (ns = 0; ns < statp->nscount; ns++)
 	    {
 #ifdef DEBUG
 		char tmpbuf[40];
 #endif
-		struct sockaddr_in6 *nsap = EXT(statp).nsaddrs[ns];
+#if defined USE_HOOKS || defined DEBUG
+		struct sockaddr *nsap = get_nsaddr (statp, ns);
+#endif
 
-		if (nsap == NULL)
-			goto next_ns;
 	    same_ns:
 #ifdef USE_HOOKS
 		if (__glibc_unlikely (statp->qhook != NULL))       {
@@ -542,9 +505,9 @@  __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 
 		Dprint(statp->options & RES_DEBUG,
 		       (stdout, ";; Querying server (# %d) address = %s\n",
-			ns + 1, inet_ntop(nsap->sin6_family,
-					  (nsap->sin6_family == AF_INET6
-					   ? &nsap->sin6_addr
+			ns + 1, inet_ntop(nsap->sa_family,
+					  (nsap->sa_family == AF_INET6
+					   ? &((struct sockaddr_in6 *) nsap)->sin6_addr
 					   : &((struct sockaddr_in *) nsap)->sin_addr),
 					  tmpbuf, sizeof (tmpbuf))));
 
@@ -660,6 +623,21 @@  libresolv_hidden_def (res_nsend)
 
 /* Private */
 
+static struct sockaddr *
+get_nsaddr (res_state statp, int n)
+{
+
+  if (statp->nsaddr_list[n].sin_family == 0 && EXT(statp).nsaddrs[n] != NULL)
+    /* EXT(statp).nsaddrs[n] holds an address that is larger than
+       struct sockaddr, and user code did not update
+       statp->nsaddr_list[n].  */
+    return (struct sockaddr *) EXT(statp).nsaddrs[n];
+  else
+    /* User code updated statp->nsaddr_list[n], or statp->nsaddr_list[n]
+       has the same content as EXT(statp).nsaddrs[n].  */
+    return (struct sockaddr *) (void *) &statp->nsaddr_list[n];
+}
+
 static int
 send_vc(res_state statp,
 	const u_char *buf, int buflen, const u_char *buf2, int buflen2,
@@ -674,7 +652,7 @@  send_vc(res_state statp,
 	// XXX REMOVE
 	// int anssiz = *anssizp;
 	HEADER *anhp = (HEADER *) ans;
-	struct sockaddr_in6 *nsap = EXT(statp).nsaddrs[ns];
+	struct sockaddr *nsap = get_nsaddr (statp, ns);
 	int truncating, connreset, n;
 	/* On some architectures compiler might emit a warning indicating
 	   'resplen' may be used uninitialized.  However if buf2 == NULL
@@ -711,8 +689,8 @@  send_vc(res_state statp,
 
 		if (getpeername(statp->_vcsock,
 				(struct sockaddr *)&peer, &size) < 0 ||
-		    !sock_eq(&peer, nsap)) {
-		  __res_iclose(statp, false);
+		    !sock_eq(&peer, (struct sockaddr_in6 *) nsap)) {
+			__res_iclose(statp, false);
 			statp->_flags &= ~RES_F_VC;
 		}
 	}
@@ -721,20 +699,19 @@  send_vc(res_state statp,
 		if (statp->_vcsock >= 0)
 		  __res_iclose(statp, false);
 
-		statp->_vcsock = socket(nsap->sin6_family, SOCK_STREAM, 0);
+		statp->_vcsock = socket(nsap->sa_family, SOCK_STREAM, 0);
 		if (statp->_vcsock < 0) {
 			*terrno = errno;
 			Perror(statp, stderr, "socket(vc)", errno);
 			return (-1);
 		}
 		__set_errno (0);
-		if (connect(statp->_vcsock, (struct sockaddr *)nsap,
-			    nsap->sin6_family == AF_INET
+		if (connect(statp->_vcsock, nsap,
+			    nsap->sa_family == AF_INET
 			    ? sizeof (struct sockaddr_in)
 			    : sizeof (struct sockaddr_in6)) < 0) {
 			*terrno = errno;
-			Aerror(statp, stderr, "connect/vc", errno,
-			       (struct sockaddr *) nsap);
+			Aerror(statp, stderr, "connect/vc", errno, nsap);
 			__res_iclose(statp, false);
 			return (0);
 		}
@@ -945,8 +922,7 @@  static int
 reopen (res_state statp, int *terrno, int ns)
 {
 	if (EXT(statp).nssocks[ns] == -1) {
-		struct sockaddr *nsap
-		  = (struct sockaddr *) EXT(statp).nsaddrs[ns];
+		struct sockaddr *nsap = get_nsaddr (statp, ns);
 		socklen_t slen;
 
 		/* only try IPv6 if IPv6 NS and if not failed before */