From afe8a60f98b3785feffae371d023c67fa124ccc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 22 Feb 2022 18:12:18 +0100 Subject: [PATCH] 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) --- lib/isc/netmgr/netmgr-int.h | 1 + lib/isc/netmgr/netmgr.c | 19 ++++++++----------- lib/isc/netmgr/tcp.c | 9 +++++---- lib/isc/netmgr/tcpdns.c | 11 ++++++----- lib/isc/netmgr/tlsdns.c | 11 ++++++----- 5 files changed, 26 insertions(+), 25 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 3a849b6c4b..bcec229391 100644 --- 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 f9e5dbb26a..bd58ffaaa7 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -1578,6 +1578,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); @@ -2034,18 +2035,14 @@ isc__nmsocket_connecttimeout_cb(uv_timer_t *timer) { sock->tls.pending_req = NULL; } - /* Call the connect callback directly */ + /* + * Mark the connection as timed out and shutdown the socket. + */ - req->cb.connect(req->handle, ISC_R_TIMEDOUT, req->cbarg); - - /* 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 e179bc05b0..b033e71cc8 100644 --- 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 9244320ec0..fcce42df97 100644 --- 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 d91e04489d..af5362f7e6 100644 --- 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);