Put up additional safe guards to not use inactive/closed tcpdns socket

When we are operating on the tcpdns socket, we need to double check
whether the socket or its outerhandle or its listener or its mgr is
still active and when not, bail out early.

(cherry picked from commit c14c1fdd2c43c96a6b31affc719cdf18f0057667)

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 23129a66db5b594a40de801e1d71a218f6f40f8b..e6d5cf697171cbf099582a68a29c539a0ea58a2e 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -873,7 +873,7 @@ isc__nm_async_tcpdnsstop(isc__networker_t *worker, isc__netievent_t *ev0);
 void
 isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0);
 
-isc_result_t
+void
 isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
 
 void

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index eefae24457e9ecc823cbb3a3088dbc3821ead40d..adeda031927335b48ba5ea65ee1b56f47ba420c2 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -1391,11 +1391,10 @@ isc__nm_async_tcpclose(isc__networker_t *worker, isc__netievent_t *ev0) {
        isc_nmsocket_t *sock = ievent->sock;
 
        REQUIRE(VALID_NMSOCK(sock));
+       REQUIRE(sock->tid == isc_nm_tid());
 
        UNUSED(worker);
 
-       REQUIRE(sock->tid == isc_nm_tid());
-
        tcp_close_direct(sock);
 }
 

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index f5df92c10564b9d60f0fe63532a045f98c1f2632..d7d4dbb0d3fbb2f881a3b823477a3cf486428912 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -152,6 +152,7 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
        dnssock->timer_initialized = true;
        uv_timer_start(&dnssock->timer, dnstcp_readtimeout,
                       dnssock->read_timeout, 0);
+       dnssock->timer_running = true;
 
        /*
         * Add a reference to handle to keep it from being freed by
@@ -259,26 +260,39 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult,
        REQUIRE(VALID_NMSOCK(dnssock));
        REQUIRE(dnssock->tid == isc_nm_tid());
        REQUIRE(VALID_NMHANDLE(handle));
-       if (eresult == ISC_R_SUCCESS &&
-           (!isc__nmsocket_active(dnssock) || dnssock->outerhandle == NULL))
+
+       if (!isc__nmsocket_active(dnssock) || atomic_load(&dnssock->closing) ||
+           dnssock->outerhandle == NULL ||
+           (dnssock->listener != NULL &&
+            !isc__nmsocket_active(dnssock->listener)) ||
+           atomic_load(&dnssock->mgr->closing))
        {
-               eresult = ISC_R_CANCELED;
+               if (eresult == ISC_R_SUCCESS) {
+                       eresult = ISC_R_CANCELED;
+               }
        }
 
        if (region == NULL || eresult != ISC_R_SUCCESS) {
+               isc_nm_recv_cb_t cb = dnssock->recv_cb;
+               void *cbarg = dnssock->recv_cbarg;
+
                /* Connection closed */
                atomic_store(&dnssock->result, eresult);
-               if (atomic_load(&dnssock->client) && dnssock->recv_cb != NULL) {
-                       dnssock->recv_cb(dnssock->statichandle, eresult, NULL,
-                                        dnssock->recv_cbarg);
+               isc__nmsocket_clearcb(dnssock);
+               if (atomic_load(&dnssock->client) && cb != NULL) {
+                       cb(dnssock->statichandle, eresult, NULL, cbarg);
                }
+
                if (dnssock->self != NULL) {
                        isc__nmsocket_detach(&dnssock->self);
                }
-               isc__nmsocket_clearcb(dnssock);
                if (dnssock->outerhandle != NULL) {
+                       isc__nmsocket_clearcb(dnssock->outerhandle->sock);
                        isc_nmhandle_detach(&dnssock->outerhandle);
                }
+               if (dnssock->listener != NULL) {
+                       isc__nmsocket_detach(&dnssock->listener);
+               }
 
                /*
                 * Server connections will hold two handle references when
@@ -487,6 +501,7 @@ resume_processing(void *arg) {
                if (sock->timer_initialized) {
                        uv_timer_start(&sock->timer, dnstcp_readtimeout,
                                       sock->read_timeout, 0);
+                       sock->timer_running = true;
                }
        }
 
@@ -551,30 +566,46 @@ tcpdnssend_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
        isc_nmhandle_detach(&handle);
 }
 
+/*
+ * The socket is closing, outerhandle has been detached, listener is
+ * inactive, or the netmgr is closing: any operation on it should abort
+ * with ISC_R_CANCELED.
+ */
+static bool
+inactive(isc_nmsocket_t *sock) {
+       return (!isc__nmsocket_active(sock) || atomic_load(&sock->closing) ||
+               sock->outerhandle == NULL ||
+               (sock->listener != NULL &&
+                !isc__nmsocket_active(sock->listener)) ||
+               atomic_load(&sock->mgr->closing));
+}
+
 void
 isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
        isc__netievent_tcpdnssend_t *ievent =
                (isc__netievent_tcpdnssend_t *)ev0;
        isc__nm_uvreq_t *req = ievent->req;
        isc_nmsocket_t *sock = ievent->sock;
+       isc_nmhandle_t *sendhandle = NULL;
+       isc_region_t r;
 
+       REQUIRE(VALID_NMSOCK(sock));
+       REQUIRE(VALID_UVREQ(req));
        REQUIRE(worker->id == sock->tid);
        REQUIRE(sock->tid == isc_nm_tid());
+       REQUIRE(sock->type == isc_nm_tcpdnssocket);
 
-       if (isc__nmsocket_active(sock) && sock->outerhandle != NULL) {
-               isc_nmhandle_t *sendhandle = NULL;
-               isc_region_t r;
-
-               r.base = (unsigned char *)req->uvbuf.base;
-               r.length = req->uvbuf.len;
-               isc_nmhandle_attach(sock->outerhandle, &sendhandle);
-               isc_nm_send(sendhandle, &r, tcpdnssend_cb, req);
-       } else {
+       if (inactive(sock)) {
                req->cb.send(req->handle, ISC_R_CANCELED, req->cbarg);
-               isc_mem_put(req->sock->mgr->mctx, req->uvbuf.base,
-                           req->uvbuf.len);
+               isc_mem_put(sock->mgr->mctx, req->uvbuf.base, req->uvbuf.len);
                isc__nm_uvreq_put(&req, req->handle->sock);
+               return;
        }
+
+       r.base = (unsigned char *)req->uvbuf.base;
+       r.length = req->uvbuf.len;
+       isc_nmhandle_attach(sock->outerhandle, &sendhandle);
+       isc_nm_send(sendhandle, &r, tcpdnssend_cb, req);
 }
 
 /*
@@ -592,7 +623,7 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->type == isc_nm_tcpdnssocket);
 
-       if (!isc__nmsocket_active(sock)) {
+       if (inactive(sock)) {
                cb(handle, ISC_R_CANCELED, cbarg);
                return;
        }
@@ -607,38 +638,25 @@ isc__nm_tcpdns_send(isc_nmhandle_t *handle, isc_region_t *region,
        *(uint16_t *)uvreq->uvbuf.base = htons(region->length);
        memmove(uvreq->uvbuf.base + 2, region->base, region->length);
 
-       if (sock->tid == isc_nm_tid()) {
-               isc_nmhandle_t *sendhandle = NULL;
-               isc_region_t r;
-
-               r.base = (unsigned char *)uvreq->uvbuf.base;
-               r.length = uvreq->uvbuf.len;
-               if (sock->outerhandle != NULL) {
-                       isc_nmhandle_attach(sock->outerhandle, &sendhandle);
-                       isc_nm_send(sock->outerhandle, &r, tcpdnssend_cb,
-                                   uvreq);
-               } else {
-                       cb(handle, ISC_R_CANCELED, cbarg);
-                       isc_mem_put(sock->mgr->mctx, uvreq->uvbuf.base,
-                                   uvreq->uvbuf.len);
-                       isc__nm_uvreq_put(&uvreq, sock);
-               }
-       } else {
-               isc__netievent_tcpdnssend_t *ievent = NULL;
+       isc__netievent_tcpdnssend_t *ievent = NULL;
 
-               ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpdnssend);
-               ievent->req = uvreq;
-               ievent->sock = sock;
+       ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpdnssend);
+       ievent->req = uvreq;
+       ievent->sock = sock;
 
-               isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
-                                      (isc__netievent_t *)ievent);
-       }
+       isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
+                              (isc__netievent_t *)ievent);
 }
 
 static void
 tcpdns_close_direct(isc_nmsocket_t *sock) {
        REQUIRE(sock->tid == isc_nm_tid());
 
+       if (sock->timer_running) {
+               uv_timer_stop(&sock->timer);
+               sock->timer_running = false;
+       }
+
        /* We don't need atomics here, it's all in single network thread */
        if (sock->self != NULL) {
                isc__nmsocket_detach(&sock->self);
@@ -673,6 +691,11 @@ isc__nm_tcpdns_close(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->type == isc_nm_tcpdnssocket);
 
+       if (!atomic_compare_exchange_strong(&sock->closing, &(bool){ false },
+                                           true)) {
+               return;
+       }
+
        if (sock->tid == isc_nm_tid()) {
                tcpdns_close_direct(sock);
        } else {
@@ -689,8 +712,12 @@ void
 isc__nm_async_tcpdnsclose(isc__networker_t *worker, isc__netievent_t *ev0) {
        isc__netievent_tcpdnsclose_t *ievent =
                (isc__netievent_tcpdnsclose_t *)ev0;
+       isc_nmsocket_t *sock = ievent->sock;
 
-       REQUIRE(worker->id == ievent->sock->tid);
+       REQUIRE(VALID_NMSOCK(sock));
+       REQUIRE(sock->tid == isc_nm_tid());
+
+       UNUSED(worker);
 
        tcpdns_close_direct(ievent->sock);
 }
@@ -739,6 +766,7 @@ tcpdnsconnect_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
        dnssock->timer_initialized = true;
        uv_timer_start(&dnssock->timer, dnstcp_readtimeout,
                       dnssock->read_timeout, 0);
+       dnssock->timer_running = true;
 
        /*
         * The connection is now established; we start reading immediately,
@@ -764,7 +792,7 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
                                  timeout, 0));
 }
 
-isc_result_t
+void
 isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
        isc_nmsocket_t *sock = handle->sock;
        isc__netievent_tcpdnsread_t *ievent = NULL;
@@ -775,6 +803,11 @@ isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
        REQUIRE(sock->tid == isc_nm_tid());
        REQUIRE(atomic_load(&sock->client));
 
+       if (inactive(sock)) {
+               cb(handle, ISC_R_NOTCONNECTED, NULL, cbarg);
+               return;
+       }
+
        /*
         * This MUST be done asynchronously, no matter which thread we're
         * in. The callback function for isc_nm_read() often calls
@@ -794,7 +827,6 @@ isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
        isc_nmhandle_attach(handle, &eventhandle);
        isc__nm_enqueue_ievent(&sock->mgr->workers[sock->tid],
                               (isc__netievent_t *)ievent);
-       return (ISC_R_SUCCESS);
 }
 
 void
@@ -811,11 +843,15 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) {
 
        handle = sock->statichandle;
 
-       if (sock->type != isc_nm_tcpdnssocket || sock->outerhandle == NULL) {
-               if (sock->recv_cb != NULL) {
-                       sock->recv_cb(handle, ISC_R_NOTCONNECTED, NULL,
-                                     sock->recv_cbarg);
+       if (inactive(sock)) {
+               isc_nm_recv_cb_t cb = sock->recv_cb;
+               void *cbarg = sock->recv_cbarg;
+
+               isc__nmsocket_clearcb(sock);
+               if (cb != NULL) {
+                       cb(handle, ISC_R_NOTCONNECTED, NULL, cbarg);
                }
+
                isc_nmhandle_detach(&handle);
                return;
        }
@@ -836,6 +872,7 @@ isc__nm_async_tcpdnsread(isc__networker_t *worker, isc__netievent_t *ev0) {
                if (sock->timer_initialized) {
                        uv_timer_start(&sock->timer, dnstcp_readtimeout,
                                       sock->read_timeout, 0);
+                       sock->timer_running = true;
                }
                isc_nm_resumeread(sock->outerhandle);
        } else {