Fix #9730: [Network] connections can use an invalid socket due to a race condition

A race condition happens when an IPv6 connection takes more than
250ms to report an error, but does return before the IPv4 connection
is established.
In result, an invalid socket might be used for that connection.

1 files changed, 15 insertions, 18 deletions

diff --git a/src/network/core/tcp_connect.cpp b/src/network/core/tcp_connect.cpp
index f9c40c202..8ef41ebf0 100644
--- a/src/network/core/tcp_connect.cpp
+++ b/src/network/core/tcp_connect.cpp
@@ -363,7 +363,10 @@ bool TCPConnecter::CheckActivity()
 		return true;
 	}
 
-	/* Check for errors on any of the sockets. */
+	/* If a socket is writeable, it is either in error-state or connected.
+	 * Remove all sockets that are in error-state and mark the first that is
+	 * not in error-state as the socket we will use for our connection. */
+	SOCKET connected_socket = INVALID_SOCKET;
 	for (auto it = this->sockets.begin(); it != this->sockets.end(); /* nothing */) {
 		NetworkError socket_error = GetSocketError(*it);
 		if (socket_error.HasError()) {
@@ -371,34 +374,28 @@ bool TCPConnecter::CheckActivity()
 			closesocket(*it);
 			this->sock_to_address.erase(*it);
 			it = this->sockets.erase(it);
-		} else {
-			it++;
+			continue;
 		}
-	}
 
-	/* In case all sockets had an error, queue a new one. */
-	if (this->sockets.empty()) {
-		if (!this->TryNextAddress()) {
-			/* There were no more addresses to try, so we failed. */
-			this->OnFailure();
-			return true;
+		/* No error but writeable means connected. */
+		if (connected_socket == INVALID_SOCKET && FD_ISSET(*it, &write_fd)) {
+			connected_socket = *it;
 		}
-		return false;
+
+		it++;
 	}
 
-	/* At least one socket is connected. The first one that does is the one
-	 * we will be using, and we close all other sockets. */
-	SOCKET connected_socket = INVALID_SOCKET;
+	/* All the writable sockets were in error state. So nothing is connected yet. */
+	if (connected_socket == INVALID_SOCKET) return false;
+
+	/* Close all sockets except the one we picked for our connection. */
 	for (auto it = this->sockets.begin(); it != this->sockets.end(); /* nothing */) {
-		if (connected_socket == INVALID_SOCKET && FD_ISSET(*it, &write_fd)) {
-			connected_socket = *it;
-		} else {
+		if (connected_socket != *it) {
 			closesocket(*it);
 		}
 		this->sock_to_address.erase(*it);
 		it = this->sockets.erase(it);
 	}
-	assert(connected_socket != INVALID_SOCKET);
 
 	Debug(net, 3, "Connected to {}", this->connection_string);
 	if (_debug_net_level >= 5) {