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

[PATCH v3 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 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 | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)
---
base-commit: ab8008b231e758e03c87c1c483c03afdd9c02e19
change-id: 20250920-fix-win32-multiple-taps-ed16ccefbd17

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

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

[Gal Horowitz]

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

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 38baf90e0b3f121f74eb32f1bff779c84ce03114..1a79e35c2708eae32ccdbc873908aa1ccc7a03f0 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;
@@ -604,7 +605,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);
@@ -647,13 +647,31 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
 
     tap_win32_overlapped_init(&tap_overlapped, handle);
 
+    tap_overlapped.thread_handle = CreateThread(NULL, 0,
+        tap_win32_thread_entry, (LPVOID)&tap_overlapped, 0, NULL);
+
     *phandle = &tap_overlapped;
 
-    CreateThread(NULL, 0, tap_win32_thread_entry,
-                 (LPVOID)&tap_overlapped, 0, &idThread);
     return 0;
 }
 
+static void tap_win32_close(tap_win32_overlapped_t *overlapped)
+{
+    TerminateThread(overlapped->thread_handle, 0);
+
+    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);
+}
+
 /********************************************/
 
  typedef struct TAPState {
@@ -667,9 +685,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 v3 2/2] tap-win32: allocate separate tap state for each instance

[Gal Horowitz]

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

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 1a79e35c2708eae32ccdbc873908aa1ccc7a03f0..1b83423191c5a6f248c771d5eb5582cc80e8abcb 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,
@@ -605,6 +607,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);
@@ -645,12 +648,12 @@ 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);
 
-    *phandle = &tap_overlapped;
+    *phandle = tap_overlapped;
 
     return 0;
 }
@@ -670,6 +673,8 @@ static void tap_win32_close(tap_win32_overlapped_t *overlapped)
     CloseHandle(overlapped->read_event);
 
     CloseHandle(overlapped->handle);
+
+    g_free(overlapped);
 }
 
 /********************************************/

-- 
2.34.1

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

[Gal Horowitz]

Hey, friendly ping for review.
Patchew: https://patchew.org/QEMU/20250924-fix-win32-multiple-taps-v3-0-9335df866c14@gmail.com/

Best Regards,
Gal Horowitz

On Wed, 24 Sept 2025 at 17:50, Gal Horowitz <galush.horowitz@gmail.com> wrote:
>
> 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 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 | 44 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> ---
> base-commit: ab8008b231e758e03c87c1c483c03afdd9c02e19
> change-id: 20250920-fix-win32-multiple-taps-ed16ccefbd17
>
> Best regards,
> --
> Gal Horowitz <galush.horowitz@gmail.com>
>

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

[Jason Wang]

On Wed, Sep 24, 2025 at 10:50 PM Gal Horowitz <galush.horowitz@gmail.com> wrote:
>
> Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
> ---

Please add a commit log for this and next patch. E.g what is being
fixed and why.

Thanks

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

[Jason Wang]

On Wed, Sep 24, 2025 at 10:50 PM Gal Horowitz <galush.horowitz@gmail.com> wrote:
>
> Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
> ---
>  net/tap-win32.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 38baf90e0b3f121f74eb32f1bff779c84ce03114..1a79e35c2708eae32ccdbc873908aa1ccc7a03f0 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;
> @@ -604,7 +605,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);
> @@ -647,13 +647,31 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
>
>      tap_win32_overlapped_init(&tap_overlapped, handle);
>
> +    tap_overlapped.thread_handle = CreateThread(NULL, 0,
> +        tap_win32_thread_entry, (LPVOID)&tap_overlapped, 0, NULL);

Should we check the return value of CreateThread() ?

> +
>      *phandle = &tap_overlapped;
>
> -    CreateThread(NULL, 0, tap_win32_thread_entry,
> -                 (LPVOID)&tap_overlapped, 0, &idThread);
>      return 0;
>  }
>
> +static void tap_win32_close(tap_win32_overlapped_t *overlapped)
> +{
> +    TerminateThread(overlapped->thread_handle, 0);
> +
> +    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);
> +}
> +
>  /********************************************/
>
>   typedef struct TAPState {
> @@ -667,9 +685,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
>

Thanks