From 675142c7d88c9d325c0ca0bc5761072a5d810c75 Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sat, 14 Mar 2015 17:27:14 +0000 Subject: [PATCH 1/2] When disabling the Windows device, wait for pending reads to complete. On Windows, when disabling the device, tinc uses the CancelIo() to cancel the pending read operation, and then proceeds to delete the event handle immediately. This assumes that CancelIo() blocks until the pending read request is completely torn down and no references to it remain. While MSDN is not completely clear on that subject, it does suggest that this is not the case: http://msdn.microsoft.com/en-us/library/windows/desktop/aa363791.aspx If the function succeeds [...] the cancel operation for all pending I/O operations issued by the calling thread for the specified file handle was successfully requested. This implies that cancellation was merely "requested", and that there are no guarantees as to the state of the operation when CancelIo() returns. Therefore, care must be taken not to close event handles prematurely. While I'm no aware of this potential race condition causing any problems in practice, I don't want to take any chances. --- src/mingw/device.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/mingw/device.c b/src/mingw/device.c index 19719a7a..73da2f13 100644 --- a/src/mingw/device.c +++ b/src/mingw/device.c @@ -210,10 +210,18 @@ static void disable_device(void) { io_del(&device_read_io); CancelIo(device_handle); + + /* According to MSDN, CancelIo() does not necessarily wait for the operation to complete. + To prevent race conditions, make sure the operation is complete + before we close the event it's referencing. */ + + DWORD len; + if(!GetOverlappedResult(device_handle, &device_read_overlapped, &len, TRUE) && GetLastError() != ERROR_OPERATION_ABORTED) + logger(DEBUG_ALWAYS, LOG_ERR, "Could not wait for %s %s read to cancel: %s", device_info, device, winerror(GetLastError())); + CloseHandle(device_read_overlapped.hEvent); ULONG status = 0; - DWORD len; DeviceIoControl(device_handle, TAP_IOCTL_SET_MEDIA_STATUS, &status, sizeof status, &status, sizeof status, &len, NULL); } From 89715454c083aaeb4dc73340f2d0ab9a3d9503e0 Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sat, 14 Mar 2015 18:19:22 +0000 Subject: [PATCH 2/2] Fix Windows device asynchronous write behavior. Write operations to the Windows device do not necessarily complete immediately; in fact, with the latest TAP-Win32 drivers, this never seems to be the case. write_packet() does not handle that case correctly, because the OVERLAPPED structure and the packet data go out of scope before the write operation completes, resulting in race conditions. This commit fixes the issue by making sure these data structures are kept in global scope, and by dropping any packets that may arrive while the previous write operation is still pending. --- src/mingw/device.c | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/src/mingw/device.c b/src/mingw/device.c index 73da2f13..4b821df7 100644 --- a/src/mingw/device.c +++ b/src/mingw/device.c @@ -38,7 +38,9 @@ int device_fd = -1; static HANDLE device_handle = INVALID_HANDLE_VALUE; static io_t device_read_io; static OVERLAPPED device_read_overlapped; +static OVERLAPPED device_write_overlapped; static vpn_packet_t device_read_packet; +static vpn_packet_t device_write_packet; char *device = NULL; char *iface = NULL; static char *device_info = NULL; @@ -200,8 +202,12 @@ static void enable_device(void) { DWORD len; DeviceIoControl(device_handle, TAP_IOCTL_SET_MEDIA_STATUS, &status, sizeof status, &status, sizeof status, &len, NULL); - io_add_event(&device_read_io, device_handle_read, NULL, CreateEvent(NULL, TRUE, FALSE, NULL)); - device_read_overlapped.hEvent = device_read_io.event; + /* We don't use the write event directly, but GetOverlappedResult() does, internally. */ + + device_read_overlapped.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL); + device_write_overlapped.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL); + + io_add_event(&device_read_io, device_handle_read, NULL, device_read_overlapped.hEvent); device_issue_read(); } @@ -218,8 +224,12 @@ static void disable_device(void) { DWORD len; if(!GetOverlappedResult(device_handle, &device_read_overlapped, &len, TRUE) && GetLastError() != ERROR_OPERATION_ABORTED) logger(DEBUG_ALWAYS, LOG_ERR, "Could not wait for %s %s read to cancel: %s", device_info, device, winerror(GetLastError())); + if(device_write_packet.len > 0 && !GetOverlappedResult(device_handle, &device_write_overlapped, &len, TRUE) && GetLastError() != ERROR_OPERATION_ABORTED) + logger(DEBUG_ALWAYS, LOG_ERR, "Could not wait for %s %s write to cancel: %s", device_info, device, winerror(GetLastError())); + device_write_packet.len = 0; CloseHandle(device_read_overlapped.hEvent); + CloseHandle(device_write_overlapped.hEvent); ULONG status = 0; DeviceIoControl(device_handle, TAP_IOCTL_SET_MEDIA_STATUS, &status, sizeof status, &status, sizeof status, &len, NULL); @@ -239,12 +249,29 @@ static bool read_packet(vpn_packet_t *packet) { static bool write_packet(vpn_packet_t *packet) { DWORD outlen; - OVERLAPPED overlapped = {0}; logger(DEBUG_TRAFFIC, LOG_DEBUG, "Writing packet of %d bytes to %s", packet->len, device_info); - if(!WriteFile(device_handle, DATA(packet), packet->len, &outlen, &overlapped)) { + if(device_write_packet.len > 0) { + /* Make sure the previous write operation is finished before we start the next one; + otherwise we end up with multiple write ops referencing the same OVERLAPPED structure, + which according to MSDN is a no-no. */ + + if(!GetOverlappedResult(device_handle, &device_write_overlapped, &outlen, FALSE)) { + int log_level = (GetLastError() == ERROR_IO_INCOMPLETE) ? DEBUG_TRAFFIC : DEBUG_ALWAYS; + logger(log_level, LOG_ERR, "Error while checking previous write to %s %s: %s", device_info, device, winerror(GetLastError())); + return false; + } + } + + /* Copy the packet, since the write operation might still be ongoing after we return. */ + + memcpy(&device_write_packet, packet, sizeof *packet); + + if(WriteFile(device_handle, DATA(&device_write_packet), device_write_packet.len, &outlen, &device_write_overlapped)) + device_write_packet.len = 0; + else if (GetLastError() != ERROR_IO_PENDING) { logger(DEBUG_ALWAYS, LOG_ERR, "Error while writing to %s %s: %s", device_info, device, winerror(GetLastError())); return false; }