Re: [PATCH] Fix memory leak in _nss_dns_gethostbyname4_r with big DNS answer

[Andreas Schwab <schwab at suse dot de>]

> +  /* Check whether ans2p was separately allocated.  */
> +  if (host_buffer.buf != orig_host_buffer)
> +    anslen = MAXPACKET;
> +  if (ans2p != NULL
> +      && (ans2p < host_buffer.ptr || ans2p >= host_buffer.ptr + anslen))
> +    free (ans2p);

It turned out that this condition is wrong.  send_vc/send_dg can reuse
the original buffer for the second answer if it fits there, even when
the first answer didn't, and this may try to free a non-heap pointer.
So to fix it properly let send_vc/send_dg tell exactly when the buffer
is newly allocated.

Andreas.

	[BZ #16574]
	* resolv/res_send.c (send_vc): Add parameter ansp2_malloced.
	Store non-zero if the second buffer was newly allocated.
	(send_dg): Likewise.
	(__libc_res_nsend): Add parameter ansp2_malloced and pass it down
	to send_vc and send_dg.
	(res_nsend): Pass NULL for ansp2_malloced.
	* resolv/res_query.c (__libc_res_nquery): Add parameter
	answerp2_malloced and pass it down to __libc_res_nsend.
	(res_nquery): Pass additional NULL to __libc_res_nquery.
	(__libc_res_nsearch): Add parameter answerp2_malloced and pass it
	down to __libc_res_nquery and __libc_res_nquerydomain.  Deallocate
	second answer buffer if answerp2_malloced was set.
	(res_nsearch): Pass additional NULL to __libc_res_nsearch.
	(__libc_res_nquerydomain): Add parameter
	answerp2_malloced and pass it down to __libc_res_nquery.
	(res_nquerydomain): Pass additional NULL to
	__libc_res_nquerydomain.
	* resolv/nss_dns/dns-network.c (_nss_dns_getnetbyname_r): Pass
	additional NULL to __libc_res_nsend and __libc_res_nquery.
	* resolv/nss_dns/dns-host.c (_nss_dns_gethostbyname3_r): Pass
	additional NULL to __libc_res_nsearch.
	(_nss_dns_gethostbyname4_r): Revert last change.  Use new
	parameter of __libc_res_nsearch to check for separately allocated
	second buffer.
	(_nss_dns_gethostbyaddr2_r): Pass additional NULL to
	__libc_res_nquery.
	* resolv/nss_dns/dns-canon.c (_nss_dns_getcanonname_r): Pass
	additional NULL to __libc_res_nquery.
	* resolv/gethnamaddr.c (gethostbyname2): Pass additional NULL to
	__libc_res_nsearch.
	(gethostbyaddr): Pass additional NULL to __libc_res_nquery.
	* include/resolv.h: Update prototypes of __libc_res_nquery,
	__libc_res_nsearch, __libc_res_nsend.

diff --git a/include/resolv.h b/include/resolv.h
index 87b3598..3904cb7 100644
--- a/include/resolv.h
+++ b/include/resolv.h
@@ -48,11 +48,11 @@ libc_hidden_proto (__res_randomid)
 libc_hidden_proto (__res_state)
 
 int __libc_res_nquery (res_state, const char *, int, int, u_char *, int,
-		       u_char **, u_char **, int *, int *);
+		       u_char **, u_char **, int *, int *, int *);
 int __libc_res_nsearch (res_state, const char *, int, int, u_char *, int,
-			u_char **, u_char **, int *, int *);
+			u_char **, u_char **, int *, int *, int *);
 int __libc_res_nsend (res_state, const u_char *, int, const u_char *, int,
-		      u_char *, int, u_char **, u_char **, int *, int *)
+		      u_char *, int, u_char **, u_char **, int *, int *, int *)
   attribute_hidden;
 
 libresolv_hidden_proto (_sethtent)
diff --git a/resolv/gethnamaddr.c b/resolv/gethnamaddr.c
index 1fd8f92..c73a0dc 100644
--- a/resolv/gethnamaddr.c
+++ b/resolv/gethnamaddr.c
@@ -621,7 +621,7 @@ gethostbyname2(name, af)
 	buf.buf = origbuf = (querybuf *) alloca (1024);
 
 	if ((n = __libc_res_nsearch(&_res, name, C_IN, type, buf.buf->buf, 1024,
-				    &buf.ptr, NULL, NULL, NULL)) < 0) {
+				    &buf.ptr, NULL, NULL, NULL, NULL)) < 0) {
 		if (buf.buf != origbuf)
 			free (buf.buf);
 		Dprintf("res_nsearch failed (%d)\n", n);
@@ -716,12 +716,12 @@ gethostbyaddr(addr, len, af)
 	buf.buf = orig_buf = (querybuf *) alloca (1024);
 
 	n = __libc_res_nquery(&_res, qbuf, C_IN, T_PTR, buf.buf->buf, 1024,
-			      &buf.ptr, NULL, NULL, NULL);
+			      &buf.ptr, NULL, NULL, NULL, NULL);
 	if (n < 0 && af == AF_INET6 && (_res.options & RES_NOIP6DOTINT) == 0) {
 		strcpy(qp, "ip6.int");
 		n = __libc_res_nquery(&_res, qbuf, C_IN, T_PTR, buf.buf->buf,
 				      buf.buf != orig_buf ? MAXPACKET : 1024,
-				      &buf.ptr, NULL, NULL, NULL);
+				      &buf.ptr, NULL, NULL, NULL, NULL);
 	}
 	if (n < 0) {
 		if (buf.buf != orig_buf)
diff --git a/resolv/nss_dns/dns-canon.c b/resolv/nss_dns/dns-canon.c
index a9db232..e8c112c 100644
--- a/resolv/nss_dns/dns-canon.c
+++ b/resolv/nss_dns/dns-canon.c
@@ -62,7 +62,7 @@ _nss_dns_getcanonname_r (const char *name, char *buffer, size_t buflen,
     {
       int r = __libc_res_nquery (&_res, name, ns_c_in, qtypes[i],
 				 buf, sizeof (buf), &ansp.ptr, NULL, NULL,
-				 NULL);
+				 NULL, NULL);
       if (r > 0)
 	{
 	  /* We need to decode the response.  Just one question record.
diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
index 47f998c..a5f2c0a 100644
--- a/resolv/nss_dns/dns-host.c
+++ b/resolv/nss_dns/dns-host.c
@@ -190,7 +190,7 @@ _nss_dns_gethostbyname3_r (const char *name, int af, struct hostent *result,
   host_buffer.buf = orig_host_buffer = (querybuf *) alloca (1024);
 
   n = __libc_res_nsearch (&_res, name, C_IN, type, host_buffer.buf->buf,
-			  1024, &host_buffer.ptr, NULL, NULL, NULL);
+			  1024, &host_buffer.ptr, NULL, NULL, NULL, NULL);
   if (n < 0)
     {
       switch (errno)
@@ -225,7 +225,7 @@ _nss_dns_gethostbyname3_r (const char *name, int af, struct hostent *result,
 	n = __libc_res_nsearch (&_res, name, C_IN, T_A, host_buffer.buf->buf,
 				host_buffer.buf != orig_host_buffer
 				? MAXPACKET : 1024, &host_buffer.ptr,
-				NULL, NULL, NULL);
+				NULL, NULL, NULL, NULL);
 
       if (n < 0)
 	{
@@ -298,23 +298,23 @@ _nss_dns_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
 	name = cp;
     }
 
-  int anslen = 2048;
   union
   {
     querybuf *buf;
     u_char *ptr;
   } host_buffer;
   querybuf *orig_host_buffer;
-  host_buffer.buf = orig_host_buffer = (querybuf *) alloca (anslen);
+  host_buffer.buf = orig_host_buffer = (querybuf *) alloca (2048);
   u_char *ans2p = NULL;
   int nans2p = 0;
   int resplen2 = 0;
+  int ans2p_malloced = 0;
 
   int olderr = errno;
   enum nss_status status;
   int n = __libc_res_nsearch (&_res, name, C_IN, T_UNSPEC,
-			      host_buffer.buf->buf, anslen, &host_buffer.ptr,
-			      &ans2p, &nans2p, &resplen2);
+			      host_buffer.buf->buf, 2048, &host_buffer.ptr,
+			      &ans2p, &nans2p, &resplen2, &ans2p_malloced);
   if (n >= 0)
     {
       status = gaih_getanswer (host_buffer.buf, n, (const querybuf *) ans2p,
@@ -351,10 +351,7 @@ _nss_dns_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
     }
 
   /* Check whether ans2p was separately allocated.  */
-  if (host_buffer.buf != orig_host_buffer)
-    anslen = MAXPACKET;
-  if (ans2p != NULL
-      && (ans2p < host_buffer.ptr || ans2p >= host_buffer.ptr + anslen))
+  if (ans2p_malloced)
     free (ans2p);
 
   if (host_buffer.buf != orig_host_buffer)
@@ -465,7 +462,7 @@ _nss_dns_gethostbyaddr2_r (const void *addr, socklen_t len, int af,
 	  strcpy (qp, "].ip6.arpa");
 	  n = __libc_res_nquery (&_res, qbuf, C_IN, T_PTR,
 				 host_buffer.buf->buf, 1024, &host_buffer.ptr,
-				 NULL, NULL, NULL);
+				 NULL, NULL, NULL, NULL);
 	  if (n >= 0)
 	    goto got_it_already;
 	}
@@ -486,14 +483,14 @@ _nss_dns_gethostbyaddr2_r (const void *addr, socklen_t len, int af,
     }
 
   n = __libc_res_nquery (&_res, qbuf, C_IN, T_PTR, host_buffer.buf->buf,
-			 1024, &host_buffer.ptr, NULL, NULL, NULL);
+			 1024, &host_buffer.ptr, NULL, NULL, NULL, NULL);
   if (n < 0 && af == AF_INET6 && (_res.options & RES_NOIP6DOTINT) == 0)
     {
       strcpy (qp, "ip6.int");
       n = __libc_res_nquery (&_res, qbuf, C_IN, T_PTR, host_buffer.buf->buf,
 			     host_buffer.buf != orig_host_buffer
 			     ? MAXPACKET : 1024, &host_buffer.ptr,
-			     NULL, NULL, NULL);
+			     NULL, NULL, NULL, NULL);
     }
   if (n < 0)
     {
diff --git a/resolv/nss_dns/dns-network.c b/resolv/nss_dns/dns-network.c
index b8aee21..0a77c8b 100644
--- a/resolv/nss_dns/dns-network.c
+++ b/resolv/nss_dns/dns-network.c
@@ -129,7 +129,7 @@ _nss_dns_getnetbyname_r (const char *name, struct netent *result,
   net_buffer.buf = orig_net_buffer = (querybuf *) alloca (1024);
 
   anslen = __libc_res_nsearch (&_res, qbuf, C_IN, T_PTR, net_buffer.buf->buf,
-			       1024, &net_buffer.ptr, NULL, NULL, NULL);
+			       1024, &net_buffer.ptr, NULL, NULL, NULL, NULL);
   if (anslen < 0)
     {
       /* Nothing found.  */
@@ -205,7 +205,7 @@ _nss_dns_getnetbyaddr_r (uint32_t net, int type, struct netent *result,
   net_buffer.buf = orig_net_buffer = (querybuf *) alloca (1024);
 
   anslen = __libc_res_nquery (&_res, qbuf, C_IN, T_PTR, net_buffer.buf->buf,
-			      1024, &net_buffer.ptr, NULL, NULL, NULL);
+			      1024, &net_buffer.ptr, NULL, NULL, NULL, NULL);
   if (anslen < 0)
     {
       /* Nothing found.  */
diff --git a/resolv/res_query.c b/resolv/res_query.c
index 88230f4..88c8255 100644
--- a/resolv/res_query.c
+++ b/resolv/res_query.c
@@ -98,7 +98,7 @@ static int
 __libc_res_nquerydomain(res_state statp, const char *name, const char *domain,
 			int class, int type, u_char *answer, int anslen,
 			u_char **answerp, u_char **answerp2, int *nanswerp2,
-			int *resplen2);
+			int *resplen2, int *answerp2_malloced);
 
 /*
  * Formulate a normal query, send, and await answer.
@@ -119,7 +119,8 @@ __libc_res_nquery(res_state statp,
 		  u_char **answerp,	/* if buffer needs to be enlarged */
 		  u_char **answerp2,
 		  int *nanswerp2,
-		  int *resplen2)
+		  int *resplen2,
+		  int *answerp2_malloced)
 {
 	HEADER *hp = (HEADER *) answer;
 	HEADER *hp2;
@@ -224,7 +225,8 @@ __libc_res_nquery(res_state statp,
 	}
 	assert (answerp == NULL || (void *) *answerp == (void *) answer);
 	n = __libc_res_nsend(statp, query1, nquery1, query2, nquery2, answer,
-			     anslen, answerp, answerp2, nanswerp2, resplen2);
+			     anslen, answerp, answerp2, nanswerp2, resplen2,
+			     answerp2_malloced);
 	if (use_malloc)
 		free (buf);
 	if (n < 0) {
@@ -316,7 +318,7 @@ res_nquery(res_state statp,
 	   int anslen)		/* size of answer buffer */
 {
 	return __libc_res_nquery(statp, name, class, type, answer, anslen,
-				 NULL, NULL, NULL, NULL);
+				 NULL, NULL, NULL, NULL, NULL);
 }
 libresolv_hidden_def (res_nquery)
 
@@ -335,7 +337,8 @@ __libc_res_nsearch(res_state statp,
 		   u_char **answerp,
 		   u_char **answerp2,
 		   int *nanswerp2,
-		   int *resplen2)
+		   int *resplen2,
+		   int *answerp2_malloced)
 {
 	const char *cp, * const *domain;
 	HEADER *hp = (HEADER *) answer;
@@ -360,7 +363,7 @@ __libc_res_nsearch(res_state statp,
 	if (!dots && (cp = res_hostalias(statp, name, tmp, sizeof tmp))!= NULL)
 		return (__libc_res_nquery(statp, cp, class, type, answer,
 					  anslen, answerp, answerp2,
-					  nanswerp2, resplen2));
+					  nanswerp2, resplen2, answerp2_malloced));
 
 #ifdef DEBUG
 	if (statp->options & RES_DEBUG)
@@ -377,7 +380,8 @@ __libc_res_nsearch(res_state statp,
 	if (dots >= statp->ndots || trailing_dot) {
 		ret = __libc_res_nquerydomain(statp, name, NULL, class, type,
 					      answer, anslen, answerp,
-					      answerp2, nanswerp2, resplen2);
+					      answerp2, nanswerp2, resplen2,
+					      answerp2_malloced);
 		if (ret > 0 || trailing_dot)
 			return (ret);
 		saved_herrno = h_errno;
@@ -386,11 +390,11 @@ __libc_res_nsearch(res_state statp,
 			answer = *answerp;
 			anslen = MAXPACKET;
 		}
-		if (answerp2
-		    && (*answerp2 < answer || *answerp2 >= answer + anslen))
+		if (answerp2 && *answerp2_malloced)
 		  {
 		    free (*answerp2);
 		    *answerp2 = NULL;
+		    *answerp2_malloced = 0;
 		  }
 	}
 
@@ -417,7 +421,7 @@ __libc_res_nsearch(res_state statp,
 						      class, type,
 						      answer, anslen, answerp,
 						      answerp2, nanswerp2,
-						      resplen2);
+						      resplen2, answerp2_malloced);
 			if (ret > 0)
 				return (ret);
 
@@ -425,12 +429,11 @@ __libc_res_nsearch(res_state statp,
 				answer = *answerp;
 				anslen = MAXPACKET;
 			}
-			if (answerp2
-			    && (*answerp2 < answer
-				|| *answerp2 >= answer + anslen))
+			if (answerp2 && *answerp2_malloced)
 			  {
 			    free (*answerp2);
 			    *answerp2 = NULL;
+			    *answerp2_malloced = 0;
 			  }
 
 			/*
@@ -486,7 +489,8 @@ __libc_res_nsearch(res_state statp,
 	    && !(tried_as_is || root_on_list)) {
 		ret = __libc_res_nquerydomain(statp, name, NULL, class, type,
 					      answer, anslen, answerp,
-					      answerp2, nanswerp2, resplen2);
+					      answerp2, nanswerp2, resplen2,
+					      answerp2_malloced);
 		if (ret > 0)
 			return (ret);
 	}
@@ -498,10 +502,11 @@ __libc_res_nsearch(res_state statp,
 	 * else send back meaningless H_ERRNO, that being the one from
 	 * the last DNSRCH we did.
 	 */
-	if (answerp2 && (*answerp2 < answer || *answerp2 >= answer + anslen))
+	if (answerp2 && *answerp2_malloced)
 	  {
 	    free (*answerp2);
 	    *answerp2 = NULL;
+	    *answerp2_malloced = 0;
 	  }
 	if (saved_herrno != -1)
 		RES_SET_H_ERRNO(statp, saved_herrno);
@@ -521,7 +526,7 @@ res_nsearch(res_state statp,
 	    int anslen)		/* size of answer */
 {
 	return __libc_res_nsearch(statp, name, class, type, answer,
-				  anslen, NULL, NULL, NULL, NULL);
+				  anslen, NULL, NULL, NULL, NULL, NULL);
 }
 libresolv_hidden_def (res_nsearch)
 
@@ -539,7 +544,8 @@ __libc_res_nquerydomain(res_state statp,
 			u_char **answerp,
 			u_char **answerp2,
 			int *nanswerp2,
-			int *resplen2)
+			int *resplen2,
+			int *answerp2_malloced)
 {
 	char nbuf[MAXDNAME];
 	const char *longname = nbuf;
@@ -581,7 +587,7 @@ __libc_res_nquerydomain(res_state statp,
 	}
 	return (__libc_res_nquery(statp, longname, class, type, answer,
 				  anslen, answerp, answerp2, nanswerp2,
-				  resplen2));
+				  resplen2, answerp2_malloced));
 }
 
 int
@@ -593,7 +599,8 @@ res_nquerydomain(res_state statp,
 	    int anslen)		/* size of answer */
 {
 	return __libc_res_nquerydomain(statp, name, domain, class, type,
-				       answer, anslen, NULL, NULL, NULL, NULL);
+				       answer, anslen, NULL, NULL, NULL, NULL,
+				       NULL);
 }
 libresolv_hidden_def (res_nquerydomain)
 
diff --git a/resolv/res_send.c b/resolv/res_send.c
index f46630d..ce7127e 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -186,12 +186,12 @@ evNowTime(struct timespec *res) {
 static int		send_vc(res_state, const u_char *, int,
 				const u_char *, int,
 				u_char **, int *, int *, int, u_char **,
-				u_char **, int *, int *);
+				u_char **, int *, int *, int *);
 static int		send_dg(res_state, const u_char *, int,
 				const u_char *, int,
 				u_char **, int *, int *, int,
 				int *, int *, u_char **,
-				u_char **, int *, int *);
+				u_char **, int *, int *, int *);
 #ifdef DEBUG
 static void		Aerror(const res_state, FILE *, const char *, int,
 			       const struct sockaddr *);
@@ -343,7 +343,7 @@ int
 __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 		 const u_char *buf2, int buflen2,
 		 u_char *ans, int anssiz, u_char **ansp, u_char **ansp2,
-		 int *nansp2, int *resplen2)
+		 int *nansp2, int *resplen2, int *ansp2_malloced)
 {
   int gotsomewhere, terrno, try, v_circuit, resplen, ns, n;
 
@@ -546,7 +546,8 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 			try = statp->retry;
 			n = send_vc(statp, buf, buflen, buf2, buflen2,
 				    &ans, &anssiz, &terrno,
-				    ns, ansp, ansp2, nansp2, resplen2);
+				    ns, ansp, ansp2, nansp2, resplen2,
+				    ansp2_malloced);
 			if (n < 0)
 				return (-1);
 			if (n == 0 && (buf2 == NULL || *resplen2 == 0))
@@ -556,7 +557,7 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 			n = send_dg(statp, buf, buflen, buf2, buflen2,
 				    &ans, &anssiz, &terrno,
 				    ns, &v_circuit, &gotsomewhere, ansp,
-				    ansp2, nansp2, resplen2);
+				    ansp2, nansp2, resplen2, ansp2_malloced);
 			if (n < 0)
 				return (-1);
 			if (n == 0 && (buf2 == NULL || *resplen2 == 0))
@@ -646,7 +647,7 @@ res_nsend(res_state statp,
 	  const u_char *buf, int buflen, u_char *ans, int anssiz)
 {
   return __libc_res_nsend(statp, buf, buflen, NULL, 0, ans, anssiz,
-			  NULL, NULL, NULL, NULL);
+			  NULL, NULL, NULL, NULL, NULL);
 }
 libresolv_hidden_def (res_nsend)
 
@@ -657,7 +658,7 @@ send_vc(res_state statp,
 	const u_char *buf, int buflen, const u_char *buf2, int buflen2,
 	u_char **ansp, int *anssizp,
 	int *terrno, int ns, u_char **anscp, u_char **ansp2, int *anssizp2,
-	int *resplen2)
+	int *resplen2, int *ansp2_malloced)
 {
 	const HEADER *hp = (HEADER *) buf;
 	const HEADER *hp2 = (HEADER *) buf2;
@@ -823,6 +824,8 @@ send_vc(res_state statp,
 			}
 			*thisanssizp = MAXPACKET;
 			*thisansp = newp;
+			if (thisansp == ansp2)
+			  *ansp2_malloced = 1;
 			anhp = (HEADER *) newp;
 			len = rlen;
 		} else {
@@ -992,7 +995,7 @@ send_dg(res_state statp,
 	const u_char *buf, int buflen, const u_char *buf2, int buflen2,
 	u_char **ansp, int *anssizp,
 	int *terrno, int ns, int *v_circuit, int *gotsomewhere, u_char **anscp,
-	u_char **ansp2, int *anssizp2, int *resplen2)
+	u_char **ansp2, int *anssizp2, int *resplen2, int *ansp2_malloced)
 {
 	const HEADER *hp = (HEADER *) buf;
 	const HEADER *hp2 = (HEADER *) buf2;
@@ -1238,6 +1241,8 @@ send_dg(res_state statp,
 			if (newp != NULL) {
 				*anssizp = MAXPACKET;
 				*thisansp = ans = newp;
+				if (thisansp == ansp2)
+				  *ansp2_malloced = 1;
 			}
 		}
 		HEADER *anhp = (HEADER *) *thisansp;
-- 
1.9.0

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