From: Witold Kręcicki <wpk@isc.org>
Date: Wed, 10 Jun 2020 00:07:16 +0000 (-0700)
Subject: allow tcpdns sockets to self-reference while connected
X-Git-Tag: v9.17.3~49^2~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cd79b4953800daf87b6bfcea0b6f3ce3bec29988;p=thirdparty%2Fbind9.git

allow tcpdns sockets to self-reference while connected

A TCPDNS socket creates a handle for each complete DNS message.

Previously, when all the handles were disconnected, the socket
would be closed, but the wrapped TCP socket might still have
more to read.

Now, when a connection is established, the TCPDNS socket creates
a reference to itself by attaching itself to sock->self. This
reference isn't cleared until the connection is closed via
EOF, timeout, or server shutdown. This allows the socket to remain
open even when there are no active handles for it.
---

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index d066198bd01..2a38b5b5325 100644
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -371,6 +371,8 @@ struct isc_nmsocket {
 	isc_nmsocket_t *parent;
 	/*% Listener socket this connection was accepted on */
 	isc_nmsocket_t *listener;
+	/*% Self, for self-contained unreferenced sockets (tcpdns) */
+	isc_nmsocket_t *self;
 
 	/*%
 	 * quota is the TCP client, attached when a TCP connection
diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 1a4a82c831a..a1e69887108 100644
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -1184,11 +1184,6 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
 		handle->doreset(handle->opaque);
 	}
 
-	/*
-	 * Temporarily reference the socket to ensure that it can't
-	 * be deleted by another thread while we're deactivating the
-	 * handle.
-	 */
 	nmhandle_deactivate(sock, handle);
 
 	/*
diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index d879fceeafb..f2b487d2962 100644
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -511,7 +511,9 @@ readtimeout_cb(uv_timer_t *handle) {
 	if (sock->quota) {
 		isc_quota_detach(&sock->quota);
 	}
-	sock->rcb.recv(sock->tcphandle, NULL, sock->rcbarg);
+	if (sock->rcb.recv != NULL) {
+		sock->rcb.recv(sock->tcphandle, NULL, sock->rcbarg);
+	}
 }
 
 isc_result_t
diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 6ae5c111cf5..c7d23e96a99 100644
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -82,6 +82,8 @@ static void
 timer_close_cb(uv_handle_t *handle) {
 	isc_nmsocket_t *sock = (isc_nmsocket_t *)uv_handle_get_data(handle);
 	INSIST(VALID_NMSOCK(sock));
+	atomic_store(&sock->closed, true);
+	tcpdns_close_direct(sock);
 }
 
 static void
@@ -91,7 +93,9 @@ dnstcp_readtimeout(uv_timer_t *timer) {
 
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->tid == isc_nm_tid());
-	tcpdns_close_direct(sock);
+	/* Close the TCP connection, it's closing should fire 'our' closing */
+	isc_nmhandle_unref(sock->outerhandle);
+	sock->outerhandle = NULL;
 }
 
 /*
@@ -123,6 +127,8 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 	dnssock->extrahandlesize = dnslistensock->extrahandlesize;
 	isc__nmsocket_attach(dnslistensock, &dnssock->listener);
 
+	isc__nmsocket_attach(dnssock, &dnssock->self);
+
 	dnssock->outerhandle = handle;
 	isc_nmhandle_ref(dnssock->outerhandle);
 
@@ -137,12 +143,12 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 	dnssock->timer_initialized = true;
 	uv_timer_start(&dnssock->timer, dnstcp_readtimeout,
 		       dnssock->read_timeout, 0);
-
 	isc_nmhandle_ref(handle);
 	result = isc_nm_read(handle, dnslisten_readcb, dnssock);
 	if (result != ISC_R_SUCCESS) {
 		isc_nmhandle_unref(handle);
 	}
+	isc__nmsocket_detach(&dnssock);
 }
 
 /*
@@ -194,11 +200,6 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) {
 				dnssock->buf_len);
 		}
 
-		/*
-		 * dnssock is now attached to dnshandle
-		 */
-		isc__nmsocket_detach(&dnssock);
-
 		*handlep = dnshandle;
 		return (ISC_R_SUCCESS);
 	}
@@ -223,7 +224,10 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
 
 	if (region == NULL) {
 		/* Connection closed */
-		isc__nm_tcpdns_close(dnssock);
+		isc_nmhandle_unref(handle);
+		if (dnssock->self != NULL) {
+			isc__nmsocket_detach(&dnssock->self);
+		}
 		return;
 	}
 
@@ -500,6 +504,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 	isc_mem_attach(sock->mgr->mctx, &t->mctx);
 	t->orighandle = handle;
 	isc_nmhandle_ref(t->orighandle);
+	isc_nmhandle_ref(t->handle);
 
 	t->region = (isc_region_t){ .base = isc_mem_get(t->mctx,
 							region->length + 2),
@@ -514,6 +519,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
 static void
 tcpdns_close_direct(isc_nmsocket_t *sock) {
 	REQUIRE(sock->tid == isc_nm_tid());
+
 	/* We don't need atomics here, it's all in single network thread */
 	if (sock->timer_initialized) {
 		/*
@@ -524,6 +530,8 @@ tcpdns_close_direct(isc_nmsocket_t *sock) {
 		sock->timer_initialized = false;
 		uv_timer_stop(&sock->timer);
 		uv_close((uv_handle_t *)&sock->timer, timer_close_cb);
+	} else if (sock->self != NULL) {
+		isc__nmsocket_detach(&sock->self);
 	} else {
 		/*
 		 * At this point we're certain that there are no external