From eda4300bbbda8bc8b9800a788cff33e7781452bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= <wpk@isc.org>
Date: Wed, 15 Jan 2020 14:53:42 +0100
Subject: [PATCH] netmgr: have a single source of truth for tcpdns callback

We pass interface as an opaque argument to tcpdns listening socket.
If we stop listening on an interface but still have in-flight connections
the opaque 'interface' is not properly reference counted, and we might
hit a dead memory. We put just a single source of truth in a listening
socket and make the child sockets use that instead of copying the
value from listening socket. We clean the callback when we stop listening.
---
 lib/isc/netmgr/netmgr-int.h |  3 +++
 lib/isc/netmgr/tcpdns.c     | 26 ++++++++++++++++----------
 lib/ns/client.c             |  5 +++++
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index cf9478f171..177adec3c6 100644
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -347,7 +347,10 @@ struct isc_nmsocket {
 	int				tid;
 	isc_nmsocket_type		type;
 	isc_nm_t			*mgr;
+	/*% Parent socket for multithreaded listeners */
 	isc_nmsocket_t			*parent;
+	/*% Listener socket this connection was accepted on */
+	isc_nmsocket_t			*listener;
 
 	/*%
 	 * quota is the TCP client, attached when a TCP connection
diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 7aa90fefcd..a15cb07b96 100644
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -121,10 +121,8 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 	isc__nmsocket_init(dnssock, handle->sock->mgr,
 			   isc_nm_tcpdnssocket, handle->sock->iface);
 
-	/* We need to copy read callbacks from outer socket */
-	dnssock->rcb.recv = dnslistensock->rcb.recv;
-	dnssock->rcbarg = dnslistensock->rcbarg;
 	dnssock->extrahandlesize = dnslistensock->extrahandlesize;
+	isc_nmsocket_attach(dnslistensock, &dnssock->listener);
 	isc_nmsocket_attach(handle->sock, &dnssock->outer);
 	dnssock->peer = handle->sock->peer;
 	dnssock->read_timeout = handle->sock->mgr->init;
@@ -171,14 +169,17 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) {
 	 */
 	len = dnslen(dnssock->buf);
 	if (len <= dnssock->buf_len - 2) {
-		isc_nmhandle_t *dnshandle = NULL;
-		isc_region_t r2 = {
-			.base = dnssock->buf + 2,
-			.length = len
-		};
+		isc_nmhandle_t *dnshandle = isc__nmhandle_get(dnssock,
+							      NULL, NULL);
+		isc_nmsocket_t *listener = dnssock->listener;
 
-		dnshandle = isc__nmhandle_get(dnssock, NULL, NULL);
-		dnssock->rcb.recv(dnshandle, &r2, dnssock->rcbarg);
+		if (listener != NULL && listener->rcb.recv != NULL) {
+			listener->rcb.recv(dnshandle,
+					   &(isc_region_t){
+						.base = dnssock->buf + 2,
+						.length = len
+					   }, listener->rcbarg);
+		}
 
 		len += 2;
 		dnssock->buf_len -= len;
@@ -322,6 +323,8 @@ isc_nm_tcpdns_stoplistening(isc_nmsocket_t *sock) {
 
 	atomic_store(&sock->listening, false);
 	atomic_store(&sock->closed, true);
+	sock->rcb.recv = NULL;
+	sock->rcbarg = NULL;
 
 	if (sock->outer != NULL) {
 		isc_nm_tcp_stoplistening(sock->outer);
@@ -502,6 +505,9 @@ tcpdns_close_direct(isc_nmsocket_t *sock) {
 		sock->outer->rcb.recv = NULL;
 		isc_nmsocket_detach(&sock->outer);
 	}
+	if (sock->listener != NULL) {
+		isc_nmsocket_detach(&sock->listener);
+	}
 	/* We don't need atomics here, it's all in single network thread */
 	if (sock->timer_initialized) {
 		sock->timer_initialized = false;
diff --git a/lib/ns/client.c b/lib/ns/client.c
index a7878aa8a0..ad99883a23 100644
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -1625,6 +1625,11 @@ ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
 	ifp = (ns_interface_t *) arg;
 
 	mgr = ifp->clientmgr;
+	if (mgr == NULL) {
+		/* The interface was shut down in the meantime, just bail */
+		return;
+	}
+
 	REQUIRE(VALID_MANAGER(mgr));
 
 	client = isc_nmhandle_getdata(handle);