From: Etienne Dechamps <etienne@edechamps.fr>
Date: Sat, 3 Dec 2016 22:52:30 +0000 (+0000)
Subject: Fix crash on Windows when a socket is available for both write and read.
X-Git-Tag: release-1.1pre15~40^2~1
X-Git-Url: https://git.tinc-vpn.org/git/browse?a=commitdiff_plain;h=1672dbd66b619f84cb86950298ad89df93e1dcba;p=tinc

Fix crash on Windows when a socket is available for both write and read.

Currently, if both write and read events fire at the same time on a
socket, the Windows-specific event loop will call both the write and
read callbacks, in that order. Problem is, the write callback could have
deleted the io handle, which makes the next call to the write callback a
use-after-free typically resulting in a hard crash.

In practice, this issue is triggered quite easily by putting the
computer to sleep, which basically freezes the tinc process. When the
computer wakes up and the process resumes, all TCP connections are
suddenly gone; as a result, the following sequence of events might
appear in the logs:

    Metadata socket read error for node1 (1.2.3.4 port 655): (10054) An existing connection was forcibly closed by the remote host.
    Closing connection with node1 (1.2.3.4 port 655)
    Sending DEL_EDGE to everyone (BROADCAST): 13 4bf6 mynode node1
    Sending 43 bytes of metadata to node2 (5.6.7.8 port 655)
    Could not send 10891 bytes of data to node2 (5.6.7.8 port 655): (10054) An existing connection was forcibly closed by the remote host.a
    Closing connection with node2 (5.6.7.8 port 655)
    <CRASH>

In this example the crash occurs because the socket to node2 was
signaled for reading *in addition* to writing, but since the connection
was terminated, the attempt to call the read callback crashed the
process.

This commit fixes the problem by not even attempting to fire the write
callback when the write event on the socket is signaled - instead, we
just rely on the part of the event loop that simulates level-triggered
write events. Arguably that's even cleaner and faster, because the code
being removed was technically redundant - we have to go through that
write check loop anyway.
---

diff --git a/src/event.c b/src/event.c
index 59b96e37..858e1d9d 100644
--- a/src/event.c
+++ b/src/event.c
@@ -357,10 +357,13 @@ bool event_loop(void) {
 			WSANETWORKEVENTS network_events;
 			if (WSAEnumNetworkEvents(io->fd, io->event, &network_events) != 0)
 				return false;
-			if (network_events.lNetworkEvents & WRITE_EVENTS)
-				io->cb(io->data, IO_WRITE);
 			if (network_events.lNetworkEvents & READ_EVENTS)
 				io->cb(io->data, IO_READ);
+			/*
+			    The fd might be available for write too. However, if we already fired the read callback, that
+			    callback might have deleted the io (e.g. through terminate_connection()), so we can't fire the
+			    write callback here. Instead, we loop back and let the writable io loop above handle it.
+			 */
 		}
 	}
 #endif