From: Witold Kręcicki <wpk@isc.org>
Date: Fri, 6 Dec 2019 19:45:00 +0000 (+0100)
Subject: netmgr: fix a race in socket destruction, happening if we close the socket
X-Git-Tag: v9.15.7~20^2~11
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=23ab349bbd47cc168050caabe9e859b931bdff62;p=thirdparty%2Fbind9.git

netmgr: fix a race in socket destruction, happening if we close the socket
externally and, at the same time, a timeout timer callback was called.
---

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 1216918d26d..09f5ef3b79e 100644
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -1039,9 +1039,38 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
 		handle->doreset(handle->opaque);
 	}
 
+
+
+	/*
+	 * The handle is closed. If the socket has a callback configured
+	 * for that (e.g., to perform cleanup after request processing),
+	 * call it now.
+	 */
+	bool do_close = true;
+	if (sock->closehandle_cb != NULL) {
+		if (sock->tid == isc_nm_tid()) {
+			sock->closehandle_cb(sock);
+		} else {
+			isc__netievent_closecb_t * event =
+				isc__nm_get_ievent(sock->mgr,
+						   netievent_closecb);
+			isc_nmsocket_attach(sock, &event->sock);
+			isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
+					       (isc__netievent_t *) event);
+			/*
+			 * If we do this asynchronously then the async event
+			 * will clean the socket, so clean up the handle from
+			 * socket and exit.
+			 */
+			do_close = false;
+		}
+	}
+
 	/*
 	 * We do all of this under lock to avoid races with socket
 	 * destruction.
+	 * We have to do this now otherwise we might race - at this point
+	 * the socket is either unused or attached to event->sock.
 	 */
 	LOCK(&sock->lock);
 
@@ -1058,36 +1087,17 @@ isc_nmhandle_unref(isc_nmhandle_t *handle) {
 		reuse = isc_astack_trypush(sock->inactivehandles,
 					   handle);
 	}
-
-	UNLOCK(&sock->lock);
-
 	if (!reuse) {
 		nmhandle_free(sock, handle);
 	}
+	UNLOCK(&sock->lock);
 
-	/*
-	 * The handle is closed. If the socket has a callback configured
-	 * for that (e.g., to perform cleanup after request processing),
-	 * call it now.
-	 */
-	if (sock->closehandle_cb != NULL) {
-		if (sock->tid == isc_nm_tid()) {
-			sock->closehandle_cb(sock);
-		} else {
-			isc__netievent_closecb_t * event =
-				isc__nm_get_ievent(sock->mgr,
-						   netievent_closecb);
-			isc_nmsocket_attach(sock, &event->sock);
-			isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
-					       (isc__netievent_t *) event);
-			/*
-			 * If we do this asynchronously then the async event
-			 * will clean the socket, so just exit.
-			 */
-			return;
-		}
+	/* Close callback will clean everything up */
+	if (!do_close) {
+		return;
 	}
 
+
 	if (atomic_load(&sock->ah) == 0 &&
 	    !atomic_load(&sock->active) &&
 	    !atomic_load(&sock->destroying))