[PATCH v4 0/2] tap-win32: fix multiple tap support

[PATCH v4 0/2] tap-win32: fix multiple tap support

[Gal Horowitz]

Currently when more than one tap is created on Windows, QEMU immediately
crashes with a null-deref since the code incorrectly uses a static global
for the tap state.

Instead, this series allocates a structure for each tap at startup.
We also take care of cleaning up when the tap device is close.

NOTE: Checkpatch has a false positive on the first commit, where it
mistakenly flags a cast before an address-of operator as a bitwise
and operator missing spaces.

Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
---
Changes in v4:
- Added commit descriptions
- Added check of CreateThread's return value
- Link to v3: https://lore.kernel.org/qemu-devel/20250924-fix-win32-multiple-taps-v3-0-9335df866c14@gmail.com

Changes in v3:
- Split to multiple commits
- Link to v2: https://lore.kernel.org/qemu-devel/20250923-fix-win32-multiple-taps-v2-1-d497e5ac446f@gmail.com

Changes in v2:
- Add cleanup of the structure fields
- Terminate the thread before freeing the structure
- Link to v1: https://lore.kernel.org/qemu-devel/20250920-fix-win32-multiple-taps-v1-1-bee41dcc213d@gmail.com

---
Gal Horowitz (2):
      tap-win32: cleanup leaked handles on tap close
      tap-win32: allocate separate tap state for each instance

 net/tap-win32.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)
---
base-commit: 18f6f30b0089b470f3e737637a86dfb81ebd6eae
change-id: 20250920-fix-win32-multiple-taps-ed16ccefbd17

Best regards,
-- 
Gal Horowitz <galush.horowitz@gmail.com>

[PATCH v4 1/2] tap-win32: cleanup leaked handles on tap close

[Gal Horowitz]

Currently, all handles owned by a win32 tap are leaked on cleanup.
This commit ensures that the handles are properly closed upon tap
cleanup. A check for the return value of CreateThread is also added.

Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
---
 net/tap-win32.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 38baf90e0b3f121f74eb32f1bff779c84ce03114..ae25e5a883343bf9f570d0f382b8c64712d95242 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -104,6 +104,7 @@ typedef struct tap_win32_overlapped {
     HANDLE output_queue_semaphore;
     HANDLE free_list_semaphore;
     HANDLE tap_semaphore;
+    HANDLE thread_handle;
     CRITICAL_SECTION output_queue_cs;
     CRITICAL_SECTION free_list_cs;
     OVERLAPPED read_overlapped;
@@ -589,6 +590,26 @@ static void tap_win32_free_buffer(tap_win32_overlapped_t *overlapped,
     put_buffer_on_free_list(overlapped, buffer);
 }
 
+static void tap_win32_close(tap_win32_overlapped_t *overlapped)
+{
+    TerminateThread(overlapped->thread_handle, 0);
+    CloseHandle(overlapped->thread_handle);
+
+    CloseHandle(overlapped->tap_semaphore);
+    CloseHandle(overlapped->free_list_semaphore);
+    CloseHandle(overlapped->output_queue_semaphore);
+
+    DeleteCriticalSection(&overlapped->free_list_cs);
+    DeleteCriticalSection(&overlapped->output_queue_cs);
+
+    CloseHandle(overlapped->write_event);
+    CloseHandle(overlapped->read_event);
+
+    CloseHandle(overlapped->handle);
+
+    g_free(overlapped);
+}
+
 static int tap_win32_open(tap_win32_overlapped_t **phandle,
                           const char *preferred_name)
 {
@@ -604,7 +625,6 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
         unsigned long debug;
     } version;
     DWORD version_len;
-    DWORD idThread;
 
     if (preferred_name != NULL) {
         snprintf(name_buffer, sizeof(name_buffer), "%s", preferred_name);
@@ -642,15 +662,22 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
     }
 
     if (!tap_win32_set_status(handle, TRUE)) {
+        CloseHandle(handle);
         return -1;
     }
 
     tap_win32_overlapped_init(&tap_overlapped, handle);
 
-    *phandle = &tap_overlapped;
+    tap_overlapped.thread_handle = CreateThread(NULL, 0,
+        tap_win32_thread_entry, (LPVOID)&tap_overlapped, 0, NULL);
+
+    if (tap_overlapped->thread_handle == NULL) {
+        tap_win32_close(tap_overlapped);
+        return -1;
+    }
+
+    *phandle = tap_overlapped;
 
-    CreateThread(NULL, 0, tap_win32_thread_entry,
-                 (LPVOID)&tap_overlapped, 0, &idThread);
     return 0;
 }
 
@@ -667,9 +694,8 @@ static void tap_cleanup(NetClientState *nc)
 
     qemu_del_wait_object(s->handle->tap_semaphore, NULL, NULL);
 
-    /* FIXME: need to kill thread and close file handle:
-       tap_win32_close(s);
-    */
+    tap_win32_close(s->handle);
+    s->handle = NULL;
 }
 
 static ssize_t tap_receive(NetClientState *nc, const uint8_t *buf, size_t size)

-- 
2.34.1

[PATCH v4 2/2] tap-win32: allocate separate tap state for each instance

[Gal Horowitz]

A static global is incorrectly used as the tap state for all win32 taps
created. Instead, we now allocate per win32 tap.

Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
---
 net/tap-win32.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index ae25e5a883343bf9f570d0f382b8c64712d95242..3d7227fcf47ff45bbdbf60cd5b86ab8f8913b941 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -115,8 +115,6 @@ typedef struct tap_win32_overlapped {
     tun_buffer_t* output_queue_back;
 } tap_win32_overlapped_t;
 
-static tap_win32_overlapped_t tap_overlapped;
-
 static tun_buffer_t* get_buffer_from_free_list(tap_win32_overlapped_t* const overlapped)
 {
     tun_buffer_t* buffer = NULL;
@@ -403,8 +401,10 @@ static int tap_win32_set_status(HANDLE handle, int status)
                 &status, sizeof (status), &len, NULL);
 }
 
-static void tap_win32_overlapped_init(tap_win32_overlapped_t* const overlapped, const HANDLE handle)
+static tap_win32_overlapped_t *tap_win32_overlapped_new(const HANDLE handle)
 {
+    tap_win32_overlapped_t *overlapped = g_new0(tap_win32_overlapped_t, 1);
+
     overlapped->handle = handle;
 
     overlapped->read_event = CreateEvent(NULL, FALSE, FALSE, NULL);
@@ -455,6 +455,8 @@ static void tap_win32_overlapped_init(tap_win32_overlapped_t* const overlapped,
     overlapped->tap_semaphore = CreateSemaphore(NULL, 0, TUN_MAX_BUFFER_COUNT, NULL);
     if(!overlapped->tap_semaphore)
         fprintf(stderr, "error creating tap_semaphore.\n");
+
+    return overlapped;
 }
 
 static int tap_win32_write(tap_win32_overlapped_t *overlapped,
@@ -625,6 +627,7 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
         unsigned long debug;
     } version;
     DWORD version_len;
+    tap_win32_overlapped_t *tap_overlapped = NULL;
 
     if (preferred_name != NULL) {
         snprintf(name_buffer, sizeof(name_buffer), "%s", preferred_name);
@@ -666,10 +669,10 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
         return -1;
     }
 
-    tap_win32_overlapped_init(&tap_overlapped, handle);
+    tap_overlapped = tap_win32_overlapped_new(handle);
 
-    tap_overlapped.thread_handle = CreateThread(NULL, 0,
-        tap_win32_thread_entry, (LPVOID)&tap_overlapped, 0, NULL);
+    tap_overlapped->thread_handle = CreateThread(NULL, 0,
+        tap_win32_thread_entry, (LPVOID)tap_overlapped, 0, NULL);
 
     if (tap_overlapped->thread_handle == NULL) {
         tap_win32_close(tap_overlapped);

-- 
2.34.1