From: Etienne Dechamps 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) 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