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 we actually stop listening, and call
the callback on bad memory.

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index dafe203601aaa7c91e2e511610e779f562f84c8c..4394247f984e39b9eaf42b275e765f0c21e2ccbe 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -833,10 +833,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 b2b8166afb64b79a08f518a4cd5de8f75efdba1b..0e782f5cdece34623b4f3cb1bb9d26ffcb5c4a38 100644 (file)
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -210,19 +210,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;
 
@@ -256,6 +243,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.
@@ -336,10 +335,17 @@ 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);
                }
@@ -347,16 +353,6 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *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) {
-               isc__nmsocket_detach(&sock);
-               return;
-       }
-
        result = isc_sockaddr_fromsockaddr(&sockaddr, addr);
        RUNTIME_CHECK(result == ISC_R_SUCCESS);
        nmhandle = isc__nmhandle_get(sock, &sockaddr, NULL);
@@ -398,7 +394,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
@@ -522,6 +518,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);