Fix crash on Windows when a socket is available for both write and read.
authorEtienne Dechamps <etienne@edechamps.fr>
Sat, 3 Dec 2016 22:52:30 +0000 (22:52 +0000)
committerEtienne Dechamps <etienne@edechamps.fr>
Sat, 3 Dec 2016 23:21:25 +0000 (23:21 +0000)
commit1672dbd66b619f84cb86950298ad89df93e1dcba
tree2f7d2af92b6cf3ac20105e8d42f774ec4cecc348
parent979acc48aded5bb04f1574128d174d56550be302
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.
src/event.c