From: Ondřej Surý <ondrej@sury.org>
Date: Thu, 3 Dec 2020 12:00:33 +0000 (+0100)
Subject: Fix datarace when UDP/TCP connect fails and we are in nmthread
X-Git-Tag: v9.17.8~6^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=151852f4283a013104ff1de01125d0876a866dac;p=thirdparty%2Fbind9.git

Fix datarace when UDP/TCP connect fails and we are in nmthread

When we were in nmthread, the isc__nm_async_<proto>connect() function
executes in the same thread as the isc__nm_<proto>connect() and on a
failure, it would block indefinitely because the failure branch was
setting sock->active to false before the condition around the wait had a
chance to skip the WAIT().

This also fixes the zero system test being stuck on FreeBSD 11, so we
re-enable the test in the commit.
---

diff --git a/bin/tests/system/zero/prereq.sh b/bin/tests/system/zero/prereq.sh
deleted file mode 100755
index 55d18ace17c..00000000000
--- a/bin/tests/system/zero/prereq.sh
+++ /dev/null
@@ -1,19 +0,0 @@
-#!/bin/sh
-#
-# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
-#
-# This Source Code Form is subject to the terms of the Mozilla Public
-# License, v. 2.0. If a copy of the MPL was not distributed with this
-# file, you can obtain one at https://mozilla.org/MPL/2.0/.
-#
-# See the COPYRIGHT file distributed with this work for additional
-# information regarding copyright ownership.
-
-. ../conf.sh
-
-if [ "$(uname -s)" != "Linux" ]; then
-    echo_i "This test is currently broken on non-Linux platforms"
-    exit 255
-fi
-
-exit 0
diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index 02b276c8746..78bcb54521d 100644
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -158,6 +158,7 @@ failed_connect_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req,
 static isc_result_t
 tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	isc__networker_t *worker = NULL;
+	isc_result_t result = ISC_R_DEFAULT;
 	int r;
 
 	REQUIRE(VALID_NMSOCK(sock));
@@ -182,7 +183,7 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	if (r != 0) {
 		isc__nm_closesocket(sock->fd);
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -191,7 +192,7 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 		if (r != 0) {
 			isc__nm_incstats(sock->mgr,
 					 sock->statsindex[STATID_BINDFAIL]);
-			goto failure;
+			goto done;
 		}
 	}
 
@@ -201,20 +202,25 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	if (r != 0) {
 		isc__nm_incstats(sock->mgr,
 				 sock->statsindex[STATID_CONNECTFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
 
 	atomic_store(&sock->connected, true);
 
-	return (ISC_R_SUCCESS);
-
-failure:
-	atomic_store(&sock->active, false);
+done:
+	result = isc__nm_uverr2result(r);
 
-	isc__nm_tcp_close(sock);
+	LOCK(&sock->lock);
+	sock->result = result;
+	SIGNAL(&sock->cond);
+	if (!atomic_load(&sock->active)) {
+		WAIT(&sock->scond, &sock->lock);
+	}
+	INSIST(atomic_load(&sock->active));
+	UNLOCK(&sock->lock);
 
-	return (isc__nm_uverr2result(r));
+	return (result);
 }
 
 void
@@ -234,22 +240,12 @@ isc__nm_async_tcpconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
 	REQUIRE(sock->tid == isc_nm_tid());
 
 	result = tcp_connect_direct(sock, req);
-	if (result == ISC_R_SUCCESS) {
-		atomic_store(&sock->connected, true);
-		/* The connect cb will be executed in tcp_connect_cb() */
-	} else {
+	if (result != ISC_R_SUCCESS) {
+		atomic_store(&sock->active, false);
+		isc__nm_tcp_close(sock);
 		isc__nm_uvreq_put(&req, sock);
 	}
 
-	LOCK(&sock->lock);
-	sock->result = result;
-	SIGNAL(&sock->cond);
-	if (!atomic_load(&sock->active)) {
-		WAIT(&sock->scond, &sock->lock);
-	}
-	INSIST(atomic_load(&sock->active));
-	UNLOCK(&sock->lock);
-
 	/*
 	 * The sock is now attached to the handle.
 	 */
@@ -334,7 +330,6 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 	sock = isc_mem_get(mgr->mctx, sizeof(*sock));
 	isc__nmsocket_init(sock, mgr, isc_nm_tcpsocket, local);
 
-	atomic_init(&sock->active, false);
 	sock->extrahandlesize = extrahandlesize;
 	sock->connect_timeout = timeout;
 	sock->result = ISC_R_DEFAULT;
@@ -360,6 +355,7 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 					 (isc__netievent_t *)ievent);
 		isc__nm_put_netievent_tcpconnect(mgr, ievent);
 	} else {
+		atomic_init(&sock->active, false);
 		sock->tid = isc_random_uniform(mgr->nworkers);
 		isc__nm_enqueue_ievent(&mgr->workers[sock->tid],
 				       (isc__netievent_t *)ievent);
@@ -534,7 +530,7 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 	if (r < 0) {
 		isc__nm_closesocket(sock->fd);
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -547,7 +543,7 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 				&sock->iface->addr.type.sa, flags);
 	if (r < 0) {
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
+		goto done;
 	}
 #else
 	if (sock->parent->fd == -1) {
@@ -556,7 +552,7 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 		if (r < 0) {
 			isc__nm_incstats(sock->mgr,
 					 sock->statsindex[STATID_BINDFAIL]);
-			goto failure;
+			goto done;
 		}
 		sock->parent->uv_handle.tcp.flags = sock->uv_handle.tcp.flags;
 		sock->parent->fd = sock->fd;
@@ -578,12 +574,12 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 			      "uv_listen failed: %s",
 			      isc_result_totext(isc__nm_uverr2result(r)));
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
+		goto done;
 	}
 
 	atomic_store(&sock->listening, true);
 
-failure:
+done:
 	result = isc__nm_uverr2result(r);
 	if (result != ISC_R_SUCCESS) {
 		sock->pquota = NULL;
diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index dcd24316ca0..edcbff6539c 100644
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -197,6 +197,7 @@ failed_connect_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req,
 static isc_result_t
 tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	isc__networker_t *worker = NULL;
+	isc_result_t result = ISC_R_DEFAULT;
 	int r;
 
 	REQUIRE(VALID_NMSOCK(sock));
@@ -221,7 +222,7 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	if (r != 0) {
 		isc__nm_closesocket(sock->fd);
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -234,7 +235,7 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 		if (r != 0 && r != UV_EINVAL) {
 			isc__nm_incstats(sock->mgr,
 					 sock->statsindex[STATID_BINDFAIL]);
-			goto failure;
+			goto done;
 		}
 	}
 
@@ -244,20 +245,25 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	if (r != 0) {
 		isc__nm_incstats(sock->mgr,
 				 sock->statsindex[STATID_CONNECTFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
 
 	atomic_store(&sock->connected, true);
 
-	return (ISC_R_SUCCESS);
-
-failure:
-	atomic_store(&sock->active, false);
+done:
+	result = isc__nm_uverr2result(r);
 
-	isc__nm_tcpdns_close(sock);
+	LOCK(&sock->lock);
+	sock->result = result;
+	SIGNAL(&sock->cond);
+	if (!atomic_load(&sock->active)) {
+		WAIT(&sock->scond, &sock->lock);
+	}
+	INSIST(atomic_load(&sock->active));
+	UNLOCK(&sock->lock);
 
-	return (isc__nm_uverr2result(r));
+	return (result);
 }
 
 void
@@ -277,22 +283,12 @@ isc__nm_async_tcpdnsconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
 	REQUIRE(sock->tid == isc_nm_tid());
 
 	result = tcpdns_connect_direct(sock, req);
-	if (result == ISC_R_SUCCESS) {
-		atomic_store(&sock->connected, true);
-		/* The connect cb will be executed in tcpdns_connect_cb() */
-	} else {
+	if (result != ISC_R_SUCCESS) {
+		atomic_store(&sock->active, false);
+		isc__nm_tcpdns_close(sock);
 		isc__nm_uvreq_put(&req, sock);
 	}
 
-	LOCK(&sock->lock);
-	sock->result = result;
-	SIGNAL(&sock->cond);
-	if (!atomic_load(&sock->active)) {
-		WAIT(&sock->scond, &sock->lock);
-	}
-	INSIST(atomic_load(&sock->active));
-	UNLOCK(&sock->lock);
-
 	/*
 	 * The sock is now attached to the handle.
 	 */
@@ -377,7 +373,6 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 	sock = isc_mem_get(mgr->mctx, sizeof(*sock));
 	isc__nmsocket_init(sock, mgr, isc_nm_tcpdnssocket, local);
 
-	atomic_init(&sock->active, false);
 	sock->extrahandlesize = extrahandlesize;
 	sock->connect_timeout = timeout;
 	sock->result = ISC_R_DEFAULT;
@@ -403,6 +398,7 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 					    (isc__netievent_t *)ievent);
 		isc__nm_put_netievent_tcpdnsconnect(mgr, ievent);
 	} else {
+		atomic_init(&sock->active, false);
 		sock->tid = isc_random_uniform(mgr->nworkers);
 		isc__nm_enqueue_ievent(&mgr->workers[sock->tid],
 				       (isc__netievent_t *)ievent);
@@ -579,7 +575,7 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 	if (r < 0) {
 		isc__nm_closesocket(sock->fd);
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -592,7 +588,7 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 				&sock->iface->addr.type.sa, flags);
 	if (r < 0) {
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
+		goto done;
 	}
 #else
 	if (sock->parent->fd == -1) {
@@ -601,7 +597,7 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 		if (r < 0) {
 			isc__nm_incstats(sock->mgr,
 					 sock->statsindex[STATID_BINDFAIL]);
-			goto failure;
+			goto done;
 		}
 		sock->parent->uv_handle.tcp.flags = sock->uv_handle.tcp.flags;
 		sock->parent->fd = sock->fd;
@@ -623,12 +619,12 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 			      "uv_listen failed: %s",
 			      isc_result_totext(isc__nm_uverr2result(r)));
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
+		goto done;
 	}
 
 	atomic_store(&sock->listening, true);
 
-failure:
+done:
 	result = isc__nm_uverr2result(r);
 	if (result != ISC_R_SUCCESS) {
 		sock->pquota = NULL;
diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c
index 22f96d135d5..85469a4a1cb 100644
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -261,7 +261,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 	if (r < 0) {
 		isc__nm_closesocket(sock->fd);
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -275,7 +275,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 				uv_bind_flags);
 	if (r < 0) {
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
+		goto done;
 	}
 #else
 	if (sock->parent->fd == -1) {
@@ -286,7 +286,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 		if (r < 0) {
 			isc__nm_incstats(sock->mgr,
 					 sock->statsindex[STATID_BINDFAIL]);
-			goto failure;
+			goto done;
 		}
 		sock->parent->uv_handle.udp.flags = sock->uv_handle.udp.flags;
 		sock->parent->fd = sock->fd;
@@ -307,12 +307,12 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
 	r = uv_udp_recv_start(&sock->uv_handle.udp, udp_alloc_cb, udp_recv_cb);
 	if (r != 0) {
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
+		goto done;
 	}
 
 	atomic_store(&sock->listening, true);
 
-failure:
+done:
 	result = isc__nm_uverr2result(r);
 	sock->parent->rchildren += 1;
 	if (sock->parent->result == ISC_R_DEFAULT) {
@@ -641,6 +641,7 @@ static isc_result_t
 udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	isc__networker_t *worker = NULL;
 	int uv_bind_flags = UV_UDP_REUSEADDR;
+	isc_result_t result = ISC_R_DEFAULT;
 	int r;
 
 	REQUIRE(isc__nm_in_netthread());
@@ -662,7 +663,7 @@ udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 	if (r != 0) {
 		isc__nm_closesocket(sock->fd);
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-		goto failure;
+		goto done;
 	}
 	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -674,17 +675,8 @@ udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 			uv_bind_flags);
 	if (r != 0) {
 		isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-		goto failure;
-	}
-
-	r = isc_uv_udp_connect(&sock->uv_handle.udp, &req->peer.type.sa);
-	if (r != 0) {
-		isc__nm_incstats(sock->mgr,
-				 sock->statsindex[STATID_CONNECTFAIL]);
-		goto failure;
+		goto done;
 	}
-	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
-	atomic_store(&sock->connecting, false);
 
 #ifdef ISC_RECV_BUFFER_SIZE
 	uv_recv_buffer_size(&sock->uv_handle.handle,
@@ -695,16 +687,30 @@ udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
 			    &(int){ ISC_SEND_BUFFER_SIZE });
 #endif
 
-	atomic_store(&sock->connected, true);
+	r = isc_uv_udp_connect(&sock->uv_handle.udp, &req->peer.type.sa);
+	if (r != 0) {
+		isc__nm_incstats(sock->mgr,
+				 sock->statsindex[STATID_CONNECTFAIL]);
+		goto done;
+	}
+	isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
 
-	return (ISC_R_SUCCESS);
+	atomic_store(&sock->connecting, false);
+	atomic_store(&sock->connected, true);
 
-failure:
-	atomic_store(&sock->active, false);
+done:
+	result = isc__nm_uverr2result(r);
 
-	isc__nm_udp_close(sock);
+	LOCK(&sock->lock);
+	sock->result = result;
+	SIGNAL(&sock->cond);
+	if (!atomic_load(&sock->active)) {
+		WAIT(&sock->scond, &sock->lock);
+	}
+	INSIST(atomic_load(&sock->active));
+	UNLOCK(&sock->lock);
 
-	return (isc__nm_uverr2result(r));
+	return (result);
 }
 
 /*
@@ -730,23 +736,14 @@ isc__nm_async_udpconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
 	req->handle = isc__nmhandle_get(sock, &req->peer, &sock->iface->addr);
 	result = udp_connect_direct(sock, req);
 	if (result != ISC_R_SUCCESS) {
+		atomic_store(&sock->active, false);
+		isc__nm_udp_close(sock);
 		isc__nm_uvreq_put(&req, sock);
-	}
-
-	LOCK(&sock->lock);
-	sock->result = result;
-	SIGNAL(&sock->cond);
-	if (!atomic_load(&sock->active)) {
-		WAIT(&sock->scond, &sock->lock);
-	}
-	INSIST(atomic_load(&sock->active));
-	UNLOCK(&sock->lock);
-
-	/*
-	 * The callback has to be called after the socket has been
-	 * initialized
-	 */
-	if (result == ISC_R_SUCCESS) {
+	} else {
+		/*
+		 * The callback has to be called after the socket has been
+		 * initialized
+		 */
 		isc__nm_connectcb(sock, req, ISC_R_SUCCESS);
 	}
 
@@ -785,7 +782,6 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 	sock = isc_mem_get(mgr->mctx, sizeof(isc_nmsocket_t));
 	isc__nmsocket_init(sock, mgr, isc_nm_udpsocket, local);
 
-	atomic_init(&sock->active, false);
 	sock->connect_cb = cb;
 	sock->connect_cbarg = cbarg;
 	sock->read_timeout = timeout;
@@ -822,6 +818,7 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
 					 (isc__netievent_t *)event);
 		isc__nm_put_netievent_udpconnect(mgr, event);
 	} else {
+		atomic_init(&sock->active, false);
 		sock->tid = isc_random_uniform(mgr->nworkers);
 		isc__nm_enqueue_ievent(&mgr->workers[sock->tid],
 				       (isc__netievent_t *)event);
diff --git a/util/copyrights b/util/copyrights
index bebc950682e..71420be2765 100644
--- a/util/copyrights
+++ b/util/copyrights
@@ -969,7 +969,6 @@
 ./bin/tests/system/xferquota/tests.sh		SH	2000,2001,2004,2007,2012,2016,2018,2019,2020
 ./bin/tests/system/zero/ans5/ans.pl		PERL	2016,2018,2019,2020
 ./bin/tests/system/zero/clean.sh		SH	2013,2014,2015,2016,2018,2019,2020
-./bin/tests/system/zero/prereq.sh		SH	2020
 ./bin/tests/system/zero/setup.sh		SH	2013,2014,2016,2018,2019,2020
 ./bin/tests/system/zero/tests.sh		SH	2013,2016,2017,2018,2019,2020
 ./bin/tests/system/zonechecks/clean.sh		SH	2004,2007,2012,2014,2015,2016,2018,2019,2020