From: Ondřej Surý <ondrej@isc.org>
Date: Tue, 22 Feb 2022 17:12:18 +0000 (+0100)
Subject: Delay isc__nm_uvreq_t deallocation to connection callback
X-Git-Tag: v9.19.0~99^2~1
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=326862791689ea7029f381b4afd05c37abbd1fe7;p=thirdparty%2Fbind9.git

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 01a2a020921..00630688c56 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 0e6c37fd7da..e2bc7519f20 100644
--- 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 e179bc05b0c..b033e71cc8d 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 9244320ec0f..fcce42df970 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 d91e04489df..af5362f7e69 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);