From 4582ef3bb2fac9073db8cfd815f915be152bff38 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= <wpk@isc.org>
Date: Fri, 19 Jun 2020 14:31:45 +0200
Subject: [PATCH] Fix a shutdown race in netmgr udp.

We need to mark the socket as inactive early (and synchronously)
in the stoplistening process - otherwise we might destroy the
callback argument before actually stopping listening, and call
the callback on a bad memory.
---
 lib/isc/netmgr/netmgr.c |  5 +++-
 lib/isc/netmgr/udp.c    | 52 ++++++++++++++++++++---------------------
 2 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index ac981a8613..8ac42822c2 100644
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -830,10 +830,13 @@ nmsocket_maybe_destroy(isc_nmsocket_t *sock) {
 	if (active_handles == 0 || sock->tcphandle != NULL) {
 		destroy = true;
 	}
-	UNLOCK(&sock->lock);
 
 	if (destroy) {
+		atomic_store(&sock->destroying, true);
+		UNLOCK(&sock->lock);
 		nmsocket_cleanup(sock, true);
+	} else {
+		UNLOCK(&sock->lock);
 	}
 }
 
diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c
index 066e5edcb5..6e2d2098cf 100644
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -209,19 +209,6 @@ static void
 stoplistening(isc_nmsocket_t *sock) {
 	REQUIRE(sock->type == isc_nm_udplistener);
 
-	/*
-	 * Socket is already closing; there's nothing to do.
-	 */
-	if (!isc__nmsocket_active(sock)) {
-		return;
-	}
-
-	/*
-	 * Mark it inactive now so that all sends will be ignored
-	 * and we won't try to stop listening again.
-	 */
-	atomic_store(&sock->active, false);
-
 	for (int i = 0; i < sock->nchildren; i++) {
 		isc__netievent_udpstop_t *event = NULL;
 
@@ -255,6 +242,18 @@ isc__nm_udp_stoplistening(isc_nmsocket_t *sock) {
 	REQUIRE(VALID_NMSOCK(sock));
 	REQUIRE(sock->type == isc_nm_udplistener);
 
+	/*
+	 * Socket is already closing; there's nothing to do.
+	 */
+	if (!isc__nmsocket_active(sock)) {
+		return;
+	}
+	/*
+	 * Mark it inactive now so that all sends will be ignored
+	 * and we won't try to stop listening again.
+	 */
+	atomic_store(&sock->active, false);
+
 	/*
 	 * If the manager is interlocked, re-enqueue this as an asynchronous
 	 * event. Otherwise, go ahead and stop listening right away.
@@ -330,25 +329,23 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
 #endif
 
 	/*
-	 * If addr == NULL that's the end of stream - we can
-	 * free the buffer and bail.
+	 * Three reasons to return now without processing:
+	 * - If addr == NULL that's the end of stream - we can
+	 *   free the buffer and bail.
+	 * - If we're simulating a firewall blocking UDP packets
+	 *   bigger than 'maxudp' bytes for testing purposes.
+	 * - If the socket is no longer active.
 	 */
-	if (addr == NULL) {
+	maxudp = atomic_load(&sock->mgr->maxudp);
+	if ((addr == NULL) || (maxudp != 0 && (uint32_t)nrecv > maxudp) ||
+	    (!isc__nmsocket_active(sock)))
+	{
 		if (free_buf) {
 			isc__nm_free_uvbuf(sock, buf);
 		}
 		return;
 	}
 
-	/*
-	 * Simulate a firewall blocking UDP packets bigger than
-	 * 'maxudp' bytes.
-	 */
-	maxudp = atomic_load(&sock->mgr->maxudp);
-	if (maxudp != 0 && (uint32_t)nrecv > maxudp) {
-		return;
-	}
-
 	result = isc_sockaddr_fromsockaddr(&sockaddr, addr);
 	RUNTIME_CHECK(result == ISC_R_SUCCESS);
 	nmhandle = isc__nmhandle_get(sock, &sockaddr, NULL);
@@ -385,7 +382,7 @@ isc__nm_udp_send(isc_nmhandle_t *handle, isc_region_t *region, isc_nm_cb_t cb,
 	uint32_t maxudp = atomic_load(&sock->mgr->maxudp);
 
 	/*
-	 * Simulate a firewall blocking UDP packets bigger than
+	 * We're simulating a firewall blocking UDP packets bigger than
 	 * 'maxudp' bytes, for testing purposes.
 	 *
 	 * The client would ordinarily have unreferenced the handle
@@ -509,6 +506,9 @@ udp_send_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req,
 	REQUIRE(sock->tid == isc_nm_tid());
 	REQUIRE(sock->type == isc_nm_udpsocket);
 
+	if (!isc__nmsocket_active(sock)) {
+		return (ISC_R_CANCELED);
+	}
 	isc_nmhandle_ref(req->handle);
 	rv = uv_udp_send(&req->uv_req.udp_send, &sock->uv_handle.udp,
 			 &req->uvbuf, 1, &peer->type.sa, udp_send_cb);