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.

(cherry picked from commit 326862791689ea7029f381b4afd05c37abbd1fe7)

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index 23b197179a3a9fb0c9e5314e69600296e7e9ebac..30f47341712fe3a43ac3e0b1eb98b1a2c9af71fd 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -833,6 +833,7 @@ struct isc_nmsocket {
        atomic_bool connected;
        bool accepting;
        bool reading;
+       atomic_bool timedout;
        isc_refcount_t references;
 
        /*%

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 4bd45e7235ce63b008386383b6dcb263a80bcb74..a8e3290f529bd6fce3cb96a5a641e8849abdb4e0 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -1509,6 +1509,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);
 
@@ -1936,18 +1937,14 @@ isc__nmsocket_connecttimeout_cb(uv_timer_t *timer) {
 
        isc__nmsocket_timer_stop(sock);
 
-       /* 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 e562ef2d694c207dd1576879cf81219d113b0590..64914c29e384595b172481d6be098d2e84e7388e 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 61a8e6b710b3fc05ea0de1f3a9531750df7eb699..8fa2a43c5f384d0c6c00bb217e97a972dd94f0fa 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -206,22 +206,23 @@ 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__nmsocket_closing(sock)) {
                /* Socket was closed midflight by isc__nm_tcpdns_shutdown() */
                result = ISC_R_CANCELED;
                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);