Fix the data race when read-writing sock->active by using cmpxchg

diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h
index 95822d1c7831df38f1d0c54ca7cd0ab5c6dc9a52..8843cbd54f0ec4ea24310d9bb9e112baf96ec569 100644 (file)
--- a/lib/isc/include/isc/netmgr.h
+++ b/lib/isc/include/isc/netmgr.h
@@ -208,7 +208,7 @@ isc_nm_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg);
  * is data to process.
  */
 
-isc_result_t
+void
 isc_nm_pauseread(isc_nmhandle_t *handle);
 /*%<
  * Pause reading on this handle's socket, but remember the callback.

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index dd1366970abff7b8aea554a7a205db540d78bed2..99ce506987bc6cb3b9c8642be369552569f282d5 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -660,6 +660,18 @@ isc__nmsocket_active(isc_nmsocket_t *sock);
  * or, for child sockets, 'sock->parent->active'.
  */
 
+bool
+isc__nmsocket_deactivate(isc_nmsocket_t *sock);
+/*%<
+ * @brief Deactivate active socket
+ *
+ * Atomically deactive the socket by setting @p sock->active or, for child
+ * sockets, @p sock->parent->active to @c false
+ *
+ * @param[in] sock - valid nmsocket
+ * @return @c false if the socket was already inactive, @c true otherwise
+ */
+
 void
 isc__nmsocket_clearcb(isc_nmsocket_t *sock);
 /*%<
@@ -718,7 +730,7 @@ isc__nm_tcp_close(isc_nmsocket_t *sock);
 /*%<
  * Close a TCP socket.
  */
-isc_result_t
+void
 isc__nm_tcp_pauseread(isc_nmsocket_t *sock);
 /*%<
  * Pause reading on this socket, while still remembering the callback.

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 4e945ba082a9cef3d1af247f769c34970b9e4ad1..cb6b18911c476098d186c3425a4eed30df4e0d1a 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -731,6 +731,19 @@ isc__nmsocket_active(isc_nmsocket_t *sock) {
        return (atomic_load(&sock->active));
 }
 
+bool
+isc__nmsocket_deactivate(isc_nmsocket_t *sock) {
+       REQUIRE(VALID_NMSOCK(sock));
+
+       if (sock->parent != NULL) {
+               return (atomic_compare_exchange_strong(&sock->parent->active,
+                                                      &(bool){ true }, false));
+       }
+
+       return (atomic_compare_exchange_strong(&sock->active, &(bool){ true },
+                                              false));
+}
+
 void
 isc__nmsocket_attach(isc_nmsocket_t *sock, isc_nmsocket_t **target) {
        REQUIRE(VALID_NMSOCK(sock));
@@ -1380,7 +1393,7 @@ isc__nm_uvreq_get(isc_nm_t *mgr, isc_nmsocket_t *sock) {
        REQUIRE(VALID_NM(mgr));
        REQUIRE(VALID_NMSOCK(sock));
 
-       if (sock != NULL && atomic_load(&sock->active)) {
+       if (sock != NULL && isc__nmsocket_active(sock)) {
                /* Try to reuse one */
                req = isc_astack_pop(sock->inactivereqs);
        }
@@ -1419,7 +1432,7 @@ isc__nm_uvreq_put(isc__nm_uvreq_t **req0, isc_nmsocket_t *sock) {
        handle = req->handle;
        req->handle = NULL;
 
-       if (!atomic_load(&sock->active) ||
+       if (!isc__nmsocket_active(sock) ||
            !isc_astack_trypush(sock->inactivereqs, req)) {
                isc_mempool_put(sock->mgr->reqpool, req);
        }
@@ -1481,7 +1494,7 @@ isc_nm_cancelread(isc_nmhandle_t *handle) {
        }
 }
 
-isc_result_t
+void
 isc_nm_pauseread(isc_nmhandle_t *handle) {
        REQUIRE(VALID_NMHANDLE(handle));
 
@@ -1489,7 +1502,8 @@ isc_nm_pauseread(isc_nmhandle_t *handle) {
 
        switch (sock->type) {
        case isc_nm_tcpsocket:
-               return (isc__nm_tcp_pauseread(sock));
+               isc__nm_tcp_pauseread(sock);
+               break;
        default:
                INSIST(0);
                ISC_UNREACHABLE();

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index ef575ee502270c44928a571b99db5d4ed397eb5c..43d3f9fc9fbb99befddc3a2caa6fe9018a166f5d 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -722,17 +722,17 @@ isc__nm_async_tcp_startread(isc__networker_t *worker, isc__netievent_t *ev0) {
        }
 }
 
-isc_result_t
+void
 isc__nm_tcp_pauseread(isc_nmsocket_t *sock) {
        isc__netievent_pauseread_t *ievent = NULL;
 
        REQUIRE(VALID_NMSOCK(sock));
 
-       if (atomic_load(&sock->readpaused)) {
-               return (ISC_R_SUCCESS);
+       if (!atomic_compare_exchange_strong(&sock->readpaused, &(bool){ false },
+                                           true)) {
+               return;
        }
 
-       atomic_store(&sock->readpaused, true);
        ievent = isc__nm_get_ievent(sock->mgr, netievent_tcppauseread);
        ievent->sock = sock;
 
@@ -745,7 +745,7 @@ isc__nm_tcp_pauseread(isc_nmsocket_t *sock) {
                                       (isc__netievent_t *)ievent);
        }
 
-       return (ISC_R_SUCCESS);
+       return;
 }
 
 void
@@ -778,12 +778,11 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) {
                return;
        }
 
-       if (!atomic_load(&sock->readpaused)) {
+       if (!atomic_compare_exchange_strong(&sock->readpaused, &(bool){ true },
+                                           false)) {
                return;
        }
 
-       atomic_store(&sock->readpaused, false);
-
        ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpstartread);
        ievent->sock = sock;
 
@@ -903,9 +902,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
 
        REQUIRE(VALID_NMSOCK(ssock));
 
-       if (!atomic_load_relaxed(&ssock->active) ||
-           atomic_load_relaxed(&ssock->mgr->closing))
-       {
+       if (!isc__nmsocket_active(ssock) || atomic_load(&ssock->mgr->closing)) {
                /* We're closing, bail */
                if (quota != NULL) {
                        isc_quota_detach(&quota);
@@ -1197,12 +1194,14 @@ isc__nm_tcp_shutdown(isc_nmsocket_t *sock) {
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_nm_tid());
 
-       if (!isc__nmsocket_active(sock)) {
+       /*
+        * If the socket is active, mark it inactive and
+        * continue. If it isn't active, stop now.
+        */
+       if (!isc__nmsocket_deactivate(sock)) {
                return;
        }
 
-       atomic_store(&sock->active, false);
-
        if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) {
                failed_read_cb(sock, ISC_R_CANCELED);
        }

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index dd1513f4f521c544509977ef02095b8737a37603..6a20ea4c7c3d04aa7ec7c3c99eb698e6dccd8d7f 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -544,7 +544,7 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
        REQUIRE(worker->id == sock->tid);
        REQUIRE(sock->tid == isc_nm_tid());
 
-       if (atomic_load(&sock->active) && sock->outerhandle != NULL) {
+       if (isc__nmsocket_active(sock) && sock->outerhandle != NULL) {
                isc_nmhandle_t *sendhandle = NULL;
                isc_region_t r;
 

diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c
index d11b13bfe66ccea80c16d0456d0f9f7fdb3649b5..d97a0b612a40f6b0e7de5dffadc8c23f0583b758 100644 (file)
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -262,16 +262,12 @@ isc__nm_udp_stoplistening(isc_nmsocket_t *sock) {
        REQUIRE(sock->type == isc_nm_udplistener);
 
        /*
-        * Socket is already closing; there's nothing to do.
+        * If the socket is active, mark it inactive and
+        * continue. If it isn't active, stop now.
         */
-       if (!isc__nmsocket_active(sock)) {
+       if (!isc__nmsocket_deactivate(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