From: Artem Boldariev <artem@boldariev.com>
Date: Thu, 11 Feb 2021 13:03:44 +0000 (+0200)
Subject: Fix deadlock in isc_nm_tlsconnect()
X-Git-Tag: v9.17.11~12^2~3
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=66d20cf28bd1aefde220885e7b6d3d45a8507517;p=thirdparty%2Fbind9.git

Fix deadlock in isc_nm_tlsconnect()

when called from within the context of a network thread,
isc_nm_tlsconnect() hangs. it is waiting for the socket's
result code to be updated, but that update is supposed to happen
asynchronously in the network thread, and if we're already blocking
in the network thread, it can never occur.

we can kluge around this by setting the socket result code
early; this works for most clients (including "dig"), but it causes
inconsistent behaviors that manifest as test failures in the DoH unit
test.

so we kluged around it even more by setting the socket result code
early *only when running in the network thread*. we need a better
solution for this problem, but this will do for now.
---

diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c
index 9a75dc0f2ac..5505f3e2b46 100644
--- a/lib/isc/netmgr/tlsstream.c
+++ b/lib/isc/netmgr/tlsstream.c
@@ -78,18 +78,21 @@ inactive(isc_nmsocket_t *sock) {
 
 static void
 update_result(isc_nmsocket_t *sock, const isc_result_t result) {
-	if (!sock->tlsstream.server) {
-		LOCK(&sock->lock);
-		sock->result = result;
-		SIGNAL(&sock->cond);
-		while (!atomic_load(&sock->active)) {
-			WAIT(&sock->scond, &sock->lock);
+	if (!atomic_load(&sock->tlsstream.result_updated)) {
+		atomic_store(&sock->tlsstream.result_updated, true);
+		if (!sock->tlsstream.server) {
+			LOCK(&sock->lock);
+			sock->result = result;
+			SIGNAL(&sock->cond);
+			while (!atomic_load(&sock->active)) {
+				WAIT(&sock->scond, &sock->lock);
+			}
+			UNLOCK(&sock->lock);
+		} else {
+			LOCK(&sock->lock);
+			sock->result = result;
+			UNLOCK(&sock->lock);
 		}
-		UNLOCK(&sock->lock);
-	} else {
-		LOCK(&sock->lock);
-		sock->result = result;
-		UNLOCK(&sock->lock);
 	}
 }
 
@@ -751,6 +754,10 @@ isc_nm_tlsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 	isc_nmsocket_t *nsock = NULL, *tsock = NULL;
 	isc__netievent_tlsconnect_t *ievent = NULL;
 	isc_result_t result = ISC_R_DEFAULT;
+#if defined(NETMGR_TRACE) && defined(NETMGR_TRACE_VERBOSE)
+	fprintf(stderr, "TLS: isc_nm_tlsconnect(): in net thread: %s\n",
+		isc__nm_in_netthread() ? "yes" : "no");
+#endif /* NETMGR_TRACE */
 
 	REQUIRE(VALID_NM(mgr));
 
@@ -765,6 +772,7 @@ isc_nm_tlsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 	nsock->connect_cbarg = cbarg;
 	nsock->connect_timeout = timeout;
 	nsock->tlsstream.ctx = ctx;
+	nsock->tlsstream.connect_from_networker = isc__nm_in_netthread();
 
 	ievent = isc__nm_get_netievent_tlsconnect(mgr, nsock);
 	ievent->local = local->addr;
@@ -844,16 +852,19 @@ isc__nm_async_tlsconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
 	tlssock->tlsstream.tls = isc_tls_create(tlssock->tlsstream.ctx);
 	if (tlssock->tlsstream.tls == NULL) {
 		result = ISC_R_TLSERROR;
-		update_result(tlssock, result);
 		goto error;
 	}
 
 	tlssock->tid = isc_nm_tid();
 	tlssock->tlsstream.state = TLS_INIT;
 
-	(void)isc_nm_tcpconnect(worker->mgr, (isc_nmiface_t *)&ievent->local,
-				(isc_nmiface_t *)&ievent->peer, tcp_connected,
-				tlssock, tlssock->connect_timeout, 0);
+	result = isc_nm_tcpconnect(worker->mgr, (isc_nmiface_t *)&ievent->local,
+				   (isc_nmiface_t *)&ievent->peer,
+				   tcp_connected, tlssock,
+				   tlssock->connect_timeout, 0);
+	if (tlssock->tlsstream.connect_from_networker) {
+		update_result(tlssock, result);
+	}
 	return;
 
 error: