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.

diff --git a/src/mingw/device.c b/src/mingw/device.c
index 73da2f1..4b821df 100644 (file)
--- 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 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_read_packet;

+static vpn_packet_t device_write_packet;

 char *device = NULL;
 char *iface = NULL;
 static char *device_info = NULL;
 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);
 
        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();
 }
 
        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()));
        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_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);
 
        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;
 
 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);
 
 
        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;
        }
                logger(DEBUG_ALWAYS, LOG_ERR, "Error while writing to %s %s: %s", device_info, device, winerror(GetLastError()));
                return false;
        }