From: Etienne Dechamps <etienne@edechamps.fr>
Date: Sat, 28 Jun 2014 10:13:29 +0000 (+0100)
Subject: Fix connection event error handling.
X-Git-Tag: release-1.1pre11~95
X-Git-Url: https://tinc-vpn.org/git/browse?p=tinc;a=commitdiff_plain;h=cc284e7c5d298ca887c07f918da35e376bf98720

Fix connection event error handling.

Commit 86a99c6b999671ed444711139db1937617e802a0 changed the way we
handle connection events to protect against spurious event loop
callbacks. Unfortunately, it turns out that calling connect() twice on
the same socket results in different behaviors depending on the platform
(even though it seems well defined in POSIX). On Windows this resulted
in the connection handling code being unable to react to connection
errors (such as connection refused), always hitting the timeout; on
Linux this resulted in spurious error messages about connect() returning
success.

In POSIX and on Linux, using connect() on a socket where the previous
attempt failed will attempt to connect again, resulting in unnecessary
network activity. Using getsockopt(SO_ERROR) before connect() solves
that, but introduces a race condition if a connection failure happens
between the two calls.

For this reason, this commit switches from connect() to a zero-sized
send() call, which is more consistent (though not completely, see the
truth table in the comments) and simpler to use for that purpose. Note
that Windows explictly support empty send() calls; POSIX says nothing
on the subject, but testing shows it works at least on Linux.

(Surprisingly enough, Windows seems more POSIX-compliant than Linux on
this one!)
---

diff --git a/src/net_socket.c b/src/net_socket.c
index 0a4dd9a0..3b6399bf 100644
--- a/src/net_socket.c
+++ b/src/net_socket.c
@@ -401,30 +401,38 @@ static void handle_meta_io(void *data, int flags) {
 	connection_t *c = data;
 
 	if(c->status.connecting) {
-		/* The event loop does not protect against spurious events. Verify that we are actually connected. */
-		if (connect(c->socket, &c->address.sa, sizeof(c->address)) == 0)
-			logger(DEBUG_CONNECTIONS, LOG_DEBUG, "Error while connecting to %s (%s): redundant connect() unexpectedly succeeded", c->name, c->hostname);
-		else if (!sockisconn(sockerrno)) {
-			if (!sockalready(sockerrno)) {
-				logger(DEBUG_CONNECTIONS, LOG_DEBUG, "Error while checking connection status for %s (%s): %s", c->name, c->hostname, sockstrerror(sockerrno));
+		/*
+		   The event loop does not protect against spurious events. Verify that we are actually connected
+		   by issuing an empty send() call.
+
+		   Note that the behavior of send() on potentially unconnected sockets differ between platforms:
+		   +------------+-----------+-------------+-----------+
+		   |   Event    |   POSIX   |    Linux    |  Windows  |
+		   +------------+-----------+-------------+-----------+
+		   | Spurious   | ENOTCONN  | EWOULDBLOCK | ENOTCONN  |
+		   | Failed     | ENOTCONN  | (cause)     | ENOTCONN  |
+		   | Successful | (success) | (success)   | (success) |
+		   +------------+-----------+-------------+-----------+
+		*/
+		if (send(c->socket, NULL, 0, 0) != 0) {
+			if (sockwouldblock(sockerrno))
+				return;
+			int socket_error;
+			if (!socknotconn(sockerrno))
+				socket_error = sockerrno;
+			else {
+				int len = sizeof socket_error;
+				getsockopt(c->socket, SOL_SOCKET, SO_ERROR, (void *)&socket_error, &len);
+			}
+			if (socket_error) {
+				logger(DEBUG_CONNECTIONS, LOG_DEBUG, "Error while connecting to %s (%s): %s", c->name, c->hostname, sockstrerror(socket_error));
 				terminate_connection(c, false);
 			}
 			return;
 		}
 
 		c->status.connecting = false;
-
-		int result;
-		socklen_t len = sizeof result;
-		getsockopt(c->socket, SOL_SOCKET, SO_ERROR, (void *)&result, &len);
-
-		if(!result)
-			finish_connecting(c);
-		else {
-			logger(DEBUG_CONNECTIONS, LOG_DEBUG, "Error while connecting to %s (%s): %s", c->name, c->hostname, sockstrerror(result));
-			terminate_connection(c, false);
-			return;
-		}
+		finish_connecting(c);
 	}
 
 	if(flags & IO_WRITE)
diff --git a/src/utils.h b/src/utils.h
index a6adffb4..7e519f4a 100644
--- a/src/utils.h
+++ b/src/utils.h
@@ -37,8 +37,7 @@ extern const char *winerror(int);
 #define sockmsgsize(x) ((x) == WSAEMSGSIZE)
 #define sockinprogress(x) ((x) == WSAEINPROGRESS || (x) == WSAEWOULDBLOCK)
 #define sockinuse(x) ((x) == WSAEADDRINUSE)
-#define sockalready(x) ((x) == WSAEALREADY || (x) == WSAEINVAL || (x) == WSAEWOULDBLOCK) /* See MSDN for connect() */
-#define sockisconn(x) ((x) == WSAEISCONN)
+#define socknotconn(x) ((x) == WSAENOTCONN)
 #else
 #define sockerrno errno
 #define sockstrerror(x) strerror(x)
@@ -46,8 +45,7 @@ extern const char *winerror(int);
 #define sockmsgsize(x) ((x) == EMSGSIZE)
 #define sockinprogress(x) ((x) == EINPROGRESS)
 #define sockinuse(x) ((x) == EADDRINUSE)
-#define sockalready(x) ((x) == EALREADY)
-#define sockisconn(x) ((x) == EISCONN)
+#define socknotconn(x) ((x) == ENOTCONN)
 #endif
 
 extern unsigned int bitfield_to_int(const void *bitfield, size_t size);