From: Evan Hunt <each@isc.org>
Date: Wed, 22 Feb 2023 19:04:12 +0000 (-0800)
Subject: fix a bug in dns_dispatch_getnext()
X-Git-Tag: v9.19.11~30^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4e93d44c743b903149c7cb80410f5010c99b15a3;p=thirdparty%2Fbind9.git

fix a bug in dns_dispatch_getnext()

when a message arrives over a TCP connection matching an expected
QID, the dispatch is updated so it no longer expects that QID,
but continues reading. subsequent messages with the same QID are
ignored, unless the dispatch entry has called dns_dispatch_getnext()
or dns_dispatch_resume().

however, a coding error caused those functions to have no effect
when the dispatch was reading, so streams of messages with the same
QID could not be received over a single TCP connection, breaking *XFR.

this has been corrected by changing the order of operations in
tcp_dispatch_getnext() so that disp->reading isn't checked until
after the dispatch entry has been reactivated.
---

diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c
index 9e16b5682b9..65a3ba77a8d 100644
--- a/lib/dns/dispatch.c
+++ b/lib/dns/dispatch.c
@@ -531,7 +531,7 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region,
 	isc_sockaddr_t peer;
 	isc_netaddr_t netaddr;
 	int match, timeout = 0;
-	dispatch_cb_t response = NULL;
+	bool respond = true;
 
 	REQUIRE(VALID_RESPONSE(resp));
 	REQUIRE(VALID_DISPATCH(resp->disp));
@@ -542,15 +542,13 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region,
 	INSIST(resp->reading);
 	resp->reading = false;
 
-	response = resp->response;
-
 	if (resp->state == DNS_DISPATCHSTATE_CANCELED) {
 		/*
 		 * Nobody is interested in the callback if the response
 		 * has been canceled already.  Detach from the response
 		 * and the handle.
 		 */
-		response = NULL;
+		respond = false;
 		eresult = ISC_R_CANCELED;
 	}
 
@@ -648,16 +646,16 @@ next:
 	 * Do not invoke the read callback just yet and instead wait for the
 	 * proper response to arrive until the original timeout fires.
 	 */
-	response = NULL;
+	respond = false;
 	udp_dispatch_getnext(resp, timeout);
 
 done:
 	UNLOCK(&disp->lock);
 
-	if (response != NULL) {
+	if (respond) {
 		dispentry_log(resp, LVL(90), "UDP read callback on %p: %s",
 			      handle, isc_result_totext(eresult));
-		response(eresult, region, resp->arg);
+		resp->response(eresult, region, resp->arg);
 	}
 
 	dns_dispentry_detach(&resp); /* DISPENTRY003 */
@@ -725,7 +723,10 @@ tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region, dns_qid_t *qid,
 		if (resp->reading) {
 			*respp = resp;
 		} else {
-			/* We already got our DNS message. */
+			/*
+			 * We already got a message for this QID and weren't
+			 * expecting any more.
+			 */
 			result = ISC_R_UNEXPECTED;
 		}
 	} else {
@@ -1576,6 +1577,8 @@ dns_dispatch_getnext(dns_dispentry_t *resp) {
 	isc_result_t result = ISC_R_SUCCESS;
 	int32_t timeout = -1;
 
+	dispentry_log(resp, LVL(90), "getnext for QID %d", resp->id);
+
 	LOCK(&disp->lock);
 	switch (disp->socktype) {
 	case isc_socktype_udp: {
@@ -1608,7 +1611,7 @@ udp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) {
 	dns_dispatch_t *disp = resp->disp;
 	dns_dispatchmgr_t *mgr = disp->mgr;
 	dns_qid_t *qid = mgr->qid;
-	dispatch_cb_t response = NULL;
+	bool respond = false;
 
 	LOCK(&disp->lock);
 	dispentry_log(resp, LVL(90),
@@ -1633,9 +1636,7 @@ udp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) {
 
 	case DNS_DISPATCHSTATE_CONNECTED:
 		if (resp->reading) {
-			dns_dispentry_ref(resp); /* DISPENTRY003 */
-			response = resp->response;
-
+			respond = true;
 			dispentry_log(resp, LVL(90), "canceling read on %p",
 				      resp->handle);
 			isc_nm_cancelread(resp->handle);
@@ -1659,11 +1660,10 @@ udp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) {
 unlock:
 	UNLOCK(&disp->lock);
 
-	if (response) {
+	if (respond) {
 		dispentry_log(resp, LVL(90), "read callback: %s",
 			      isc_result_totext(result));
-		response(result, NULL, resp->arg);
-		dns_dispentry_detach(&resp); /* DISPENTRY003 */
+		resp->response(result, NULL, resp->arg);
 	}
 }
 
@@ -2117,6 +2117,13 @@ tcp_dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp,
 		     int32_t timeout) {
 	REQUIRE(timeout <= INT16_MAX);
 
+	dispentry_log(resp, LVL(90), "continue reading");
+
+	if (!resp->reading) {
+		ISC_LIST_APPEND(disp->active, resp, alink);
+		resp->reading = true;
+	}
+
 	if (disp->reading) {
 		return;
 	}
@@ -2125,14 +2132,9 @@ tcp_dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp,
 		isc_nmhandle_settimeout(disp->handle, timeout);
 	}
 
-	dispentry_log(resp, LVL(90), "continue reading");
-
 	dns_dispatch_ref(disp); /* DISPATCH002 */
 	isc_nm_read(disp->handle, tcp_recv, disp);
 	disp->reading = true;
-
-	ISC_LIST_APPEND(disp->active, resp, alink);
-	resp->reading = true;
 }
 
 static void
@@ -2161,6 +2163,8 @@ dns_dispatch_resume(dns_dispentry_t *resp, uint16_t timeout) {
 
 	dns_dispatch_t *disp = resp->disp;
 
+	dispentry_log(resp, LVL(90), "resume");
+
 	LOCK(&disp->lock);
 	switch (disp->socktype) {
 	case isc_socktype_udp: {