Delay isc__nm_uvreq_t deallocation to connection callback

When the TCP, TCPDNS or TLSDNS connection times out, the isc__nm_uvreq_t
would be pushed into sock->inactivereqs before the uv_tcp_connect()
callback finishes.  Because the isc__nmsocket_t keeps the list of
inactive isc__nm_uvreq_t, this would cause use-after-free only when the
sock->inactivereqs is full (which could never happen because the failure
happens in connection timeout callback) or when the sock->inactivereqs
mechanism is completely removed (f.e. when running under Address or
Thread Sanitizer).

Delay isc__nm_uvreq_t deallocation to the connection callback and only
signal the connection callback should be called by shutting down the
libuv socket from the connection timeout callback.

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 01a2a0209216e80f279715b6b52a291e88b3fc2b..00630688c5641bbaa4cf111860511a598b8e6a82 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -1014,6 +1014,7 @@ struct isc_nmsocket {
        atomic_bool connected;
        atomic_bool accepting;
        atomic_bool reading;
+       atomic_bool timedout;
        isc_refcount_t references;
 
        /*%

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 0e6c37fd7da19ec610b44e98898adbe1a005c851..e2bc7519f208c48d9d7064b8702686fa9ee9e914 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -1577,6 +1577,7 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type,
        atomic_init(&sock->connecting, false);
        atomic_init(&sock->keepalive, false);
        atomic_init(&sock->connected, false);
+       atomic_init(&sock->timedout, false);
 
        atomic_init(&sock->active_child_connections, 0);
 
@@ -2033,18 +2034,14 @@ isc__nmsocket_connecttimeout_cb(uv_timer_t *timer) {
                sock->tls.pending_req = NULL;
        }
 
-       /* Call the connect callback directly */
-
-       req->cb.connect(req->handle, ISC_R_TIMEDOUT, req->cbarg);
+       /*
+        * Mark the connection as timed out and shutdown the socket.
+        */
 
-       /* Timer is not running, cleanup and shutdown everything */
-       if (!isc__nmsocket_timer_running(sock)) {
-               INSIST(atomic_compare_exchange_strong(&sock->connecting,
-                                                     &(bool){ true }, false));
-               isc__nm_uvreq_put(&req, sock);
-               isc__nmsocket_clearcb(sock);
-               isc__nmsocket_shutdown(sock);
-       }
+       INSIST(atomic_compare_exchange_strong(&sock->timedout, &(bool){ false },
+                                             true));
+       isc__nmsocket_clearcb(sock);
+       isc__nmsocket_shutdown(sock);
 }
 
 void

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index e179bc05b0c1f444c6a874bcd6c5b667bc9c81c5..b033e71cc8da132c1575647ed5fb247c56049eb3 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -239,15 +239,16 @@ tcp_connect_cb(uv_connect_t *uvreq, int status) {
        isc__nmsocket_timer_stop(sock);
        uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
 
-       if (!atomic_load(&sock->connecting)) {
-               return;
-       }
-
        req = uv_handle_get_data((uv_handle_t *)uvreq);
 
        REQUIRE(VALID_UVREQ(req));
        REQUIRE(VALID_NMHANDLE(req->handle));
 
+       if (atomic_load(&sock->timedout)) {
+               result = ISC_R_TIMEDOUT;
+               goto error;
+       }
+
        if (!atomic_load(&sock->connecting)) {
                /*
                 * The connect was cancelled from timeout; just clean up

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 9244320ec0fc1204dcbc0335ec7c387931d0e9f4..fcce42df970d9e7a25cc7666daa4305815f204b2 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -206,15 +206,16 @@ tcpdns_connect_cb(uv_connect_t *uvreq, int status) {
        isc__nmsocket_timer_stop(sock);
        uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock);
 
-       if (!atomic_load(&sock->connecting)) {
-               return;
-       }
-
        req = uv_handle_get_data((uv_handle_t *)uvreq);
 
        REQUIRE(VALID_UVREQ(req));
        REQUIRE(VALID_NMHANDLE(req->handle));
 
+       if (atomic_load(&sock->timedout)) {
+               result = ISC_R_TIMEDOUT;
+               goto error;
+       }
+
        if (isc__nm_closing(sock)) {
                /* Network manager shutting down */
                result = ISC_R_SHUTTINGDOWN;
@@ -225,7 +226,7 @@ tcpdns_connect_cb(uv_connect_t *uvreq, int status) {
                goto error;
        } else if (status == UV_ETIMEDOUT) {
                /* Timeout status code here indicates hard error */
-               result = ISC_R_CANCELED;
+               result = ISC_R_TIMEDOUT;
                goto error;
        } else if (status != 0) {
                result = isc__nm_uverr2result(status);

diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c
index d91e04489df68c21ac73f455bca70249f37104c7..af5362f7e693785f04922d08fa41f0cfe265886d 100644 (file)
--- a/lib/isc/netmgr/tlsdns.c
+++ b/lib/isc/netmgr/tlsdns.c
@@ -221,15 +221,16 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) {
        REQUIRE(VALID_NMSOCK(sock));
        REQUIRE(sock->tid == isc_nm_tid());
 
-       if (!atomic_load(&sock->connecting)) {
-               return;
-       }
-
        req = uv_handle_get_data((uv_handle_t *)uvreq);
 
        REQUIRE(VALID_UVREQ(req));
        REQUIRE(VALID_NMHANDLE(req->handle));
 
+       if (atomic_load(&sock->timedout)) {
+               result = ISC_R_TIMEDOUT;
+               goto error;
+       }
+
        if (isc__nm_closing(sock)) {
                /* Network manager shutting down */
                result = ISC_R_SHUTTINGDOWN;
@@ -240,7 +241,7 @@ tlsdns_connect_cb(uv_connect_t *uvreq, int status) {
                goto error;
        } else if (status == UV_ETIMEDOUT) {
                /* Timeout status code here indicates hard error */
-               result = ISC_R_CANCELED;
+               result = ISC_R_TIMEDOUT;
                goto error;
        } else if (status != 0) {
                result = isc__nm_uverr2result(status);