From 01c4c3301e55b7d6a935a95ac0829e37fb317a0e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= <wpk@isc.org>
Date: Thu, 26 Mar 2020 14:25:06 +0100
Subject: [PATCH] Deactivate the handle before sending the async close
 callback.

We could have a race between handle closing and processing async
callback. Deactivate the handle before issuing the callback - we
have the socket referenced anyway so it's not a problem.
---
 CHANGES                     |  5 +++++
 lib/isc/netmgr/netmgr-int.h |  3 +--
 lib/isc/netmgr/netmgr.c     | 31 +++++++++++++------------------
 3 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/CHANGES b/CHANGES
index 19d0f29ade..2ad5684af7 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,8 @@
+5370.	[bug]		Deactivation of a netmgr handle associated with a
+			socket could be skipped in some circumstances.
+			Fixed by deactivating the netmgr handle before
+			scheduling the asynchronous close routine. [GL #1700]
+
 5369.	[func]		Add the ability to specify whether or not to wait
 			for nameserver domain names to be looked up, with
 			a new RPZ modifying directive 'nsdname-wait-recurse'.
diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 6472115009..c825a5854d 100644
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -216,6 +216,7 @@ typedef isc__netievent__socket_t isc__netievent_tcpclose_t;
 typedef isc__netievent__socket_t isc__netievent_tcpdnsclose_t;
 typedef isc__netievent__socket_t isc__netievent_startread_t;
 typedef isc__netievent__socket_t isc__netievent_pauseread_t;
+typedef isc__netievent__socket_t isc__netievent_closecb_t;
 
 typedef struct isc__netievent__socket_req {
 	isc__netievent_type type;
@@ -241,8 +242,6 @@ typedef struct isc__netievent__socket_handle {
 	isc_nmhandle_t *handle;
 } isc__netievent__socket_handle_t;
 
-typedef isc__netievent__socket_handle_t isc__netievent_closecb_t;
-
 typedef struct isc__netievent_udpsend {
 	isc__netievent_type type;
 	isc_nmsocket_t *sock;
diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 55e7525436..f4b784862f 100644
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -1164,7 +1164,15 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
 	}
 
 	/*
-	 * The handle is closed. If the socket has a callback configured
+	 * Temporarily reference the socket to ensure that it can't
+	 * be deleted by another thread while we're deactivating the
+	 * handle.
+	 */
+	isc_nmsocket_attach(sock, &tmp);
+	nmhandle_deactivate(sock, handle);
+
+	/*
+	 * The handle is gone now. If the socket has a callback configured
 	 * for that (e.g., to perform cleanup after request processing),
 	 * call it now, or schedule it to run asynchronously.
 	 */
@@ -1174,27 +1182,16 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
 		} else {
 			isc__netievent_closecb_t *event = isc__nm_get_ievent(
 				sock->mgr, netievent_closecb);
+			/*
+			 * The socket will be finally detached by the closecb
+			 * event handler.
+			 */
 			isc_nmsocket_attach(sock, &event->sock);
-			event->handle = handle;
 			isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
 					       (isc__netievent_t *)event);
-
-			/*
-			 * If we're doing this asynchronously, then the
-			 * async event will take care of cleaning up the
-			 * handle and closing the socket.
-			 */
-			return;
 		}
 	}
 
-	/*
-	 * Temporarily reference the socket to ensure that it can't
-	 * be deleted by another thread while we're deactivating the
-	 * handle.
-	 */
-	isc_nmsocket_attach(sock, &tmp);
-	nmhandle_deactivate(sock, handle);
 	isc_nmsocket_detach(&tmp);
 }
 
@@ -1387,8 +1384,6 @@ isc__nm_async_closecb(isc__networker_t *worker, isc__netievent_t *ev0) {
 
 	UNUSED(worker);
 
-	nmhandle_deactivate(ievent->sock, ievent->handle);
-
 	ievent->sock->closehandle_cb(ievent->sock);
 	isc_nmsocket_detach(&ievent->sock);
 }