tap-win32: disable broken async write path

The code under the TUN_ASYNCHRONOUS_WRITES path makes two incorrect
assumptions about the behaviour of the WriteFile API for overlapped
file handles. First, WriteFile does not update the
lpNumberOfBytesWritten parameter when the write completes
asynchronously (the number of bytes written is known only when the
operation completes). Second, the buffer shouldn't be touched (or
freed) until the operation completes. This led to at least one bug
where tap_win32_write returned zero bytes written, which in turn
caused further writes ("receives") to be disabled for that device.

This change disables the asynchronous write path, while keeping most
of the code around in case someone sees value in resurrecting it. It
also adds some conditional debug output, similar to the read path.

Change-Id: I0407ff238d9f8af50d2f957c1751037362020f87
Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
Acked-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Jason Wang <jasowang@redhat.com>
(cherry picked from commit 8e760673dfc8bfe867d03b9f63d6c56351c44635)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 68200f8f2a1230f2fe81ca59ab064abb17332604..b27f8ecf3c4be8f19746eaf0f372b6cce97fc401 100644 (file)
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -77,7 +77,12 @@
 
 //#define DEBUG_TAP_WIN32
 
-#define TUN_ASYNCHRONOUS_WRITES 1
+/* FIXME: The asynch write path appears to be broken at
+ * present. WriteFile() ignores the lpNumberOfBytesWritten parameter
+ * for overlapped writes, with the result we return zero bytes sent,
+ * and after handling a single packet, receive is disabled for this
+ * interface. */
+/* #define TUN_ASYNCHRONOUS_WRITES 1 */
 
 #define TUN_BUFFER_SIZE 1560
 #define TUN_MAX_BUFFER_COUNT 32
@@ -461,27 +466,48 @@ static int tap_win32_write(tap_win32_overlapped_t *overlapped,
     BOOL result;
     DWORD error;
 
+#ifdef TUN_ASYNCHRONOUS_WRITES
     result = GetOverlappedResult( overlapped->handle, &overlapped->write_overlapped,
                                   &write_size, FALSE);
 
     if (!result && GetLastError() == ERROR_IO_INCOMPLETE)
         WaitForSingleObject(overlapped->write_event, INFINITE);
+#endif
 
     result = WriteFile(overlapped->handle, buffer, size,
                        &write_size, &overlapped->write_overlapped);
 
+#ifdef TUN_ASYNCHRONOUS_WRITES
+    /* FIXME: we can't sensibly set write_size here, without waiting
+     * for the IO to complete! Moreover, we can't return zero,
+     * because that will disable receive on this interface, and we
+     * also can't assume it will succeed and return the full size,
+     * because that will result in the buffer being reclaimed while
+     * the IO is in progress. */
+#error Async writes are broken. Please disable TUN_ASYNCHRONOUS_WRITES.
+#else /* !TUN_ASYNCHRONOUS_WRITES */
     if (!result) {
-        switch (error = GetLastError())
-        {
-        case ERROR_IO_PENDING:
-#ifndef TUN_ASYNCHRONOUS_WRITES
-            WaitForSingleObject(overlapped->write_event, INFINITE);
-#endif
-            break;
-        default:
-            return -1;
+        error = GetLastError();
+        if (error == ERROR_IO_PENDING) {
+            result = GetOverlappedResult(overlapped->handle,
+                                         &overlapped->write_overlapped,
+                                         &write_size, TRUE);
         }
     }
+#endif
+
+    if (!result) {
+#ifdef DEBUG_TAP_WIN32
+        LPTSTR msgbuf;
+        error = GetLastError();
+        FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM,
+                      NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                      &msgbuf, 0, NULL);
+        fprintf(stderr, "Tap-Win32: Error WriteFile %d - %s\n", error, msgbuf);
+        LocalFree(msgbuf);
+#endif
+        return 0;
+    }
 
     return write_size;
 }