From: Etienne Dechamps 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://git.tinc-vpn.org/git/browse?a=commitdiff_plain;h=cc284e7c5d298ca887c07f918da35e376bf98720;p=tinc 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);