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.

diff --git a/src/mingw/device.c b/src/mingw/device.c
index 19719a7a0b4fc808d4924b6a5cfc8b66c5308132..73da2f1316696dc0bcdbec0e9d5d45a4906a88d5 100644 (file)
--- 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);
 }