1. Patchew
  2. QEMU
  3. View series

[PATCH] tap-win32: fix multiple tap support

Gal Horowitz posted 1 patch 1 week, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250920-fix-win32-multiple-taps-v1-1-bee41dcc213d@gmail.com
Maintainers: Jason Wang <jasowang@redhat.com>, Stefan Weil <sw@weilnetz.de>
There is a newer version of this series
net/tap-win32.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
[PATCH] tap-win32: fix multiple tap support
Posted by Gal Horowitz 1 week, 1 day ago 
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 patch allocates a structure for each tap at startup.

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

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 38baf90e0b3f121f74eb32f1bff779c84ce03114..217a43cc2f5effdd92e1bf49466fe8d2cd0490e6 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -114,8 +114,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;
@@ -605,6 +603,7 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
     } version;
     DWORD version_len;
     DWORD idThread;
+    tap_win32_overlapped_t *tap_overlapped = NULL;
 
     if (preferred_name != NULL) {
         snprintf(name_buffer, sizeof(name_buffer), "%s", preferred_name);
@@ -645,12 +644,14 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
         return -1;
     }
 
-    tap_win32_overlapped_init(&tap_overlapped, handle);
+    tap_overlapped = g_new0(tap_win32_overlapped_t, 1);
+
+    tap_win32_overlapped_init(tap_overlapped, handle);
 
-    *phandle = &tap_overlapped;
+    *phandle = tap_overlapped;
 
     CreateThread(NULL, 0, tap_win32_thread_entry,
-                 (LPVOID)&tap_overlapped, 0, &idThread);
+                 (LPVOID)tap_overlapped, 0, &idThread);
     return 0;
 }
 
@@ -670,6 +671,9 @@ static void tap_cleanup(NetClientState *nc)
     /* FIXME: need to kill thread and close file handle:
        tap_win32_close(s);
     */
+
+   g_free(s->handle);
+   s->handle = NULL;
 }
 
 static ssize_t tap_receive(NetClientState *nc, const uint8_t *buf, size_t size)

---
base-commit: ab8008b231e758e03c87c1c483c03afdd9c02e19
change-id: 20250920-fix-win32-multiple-taps-ed16ccefbd17

Best regards,
-- 
Gal Horowitz <galush.horowitz@gmail.com>
Re: [PATCH] tap-win32: fix multiple tap support
Posted by Daniel P. Berrangé 5 days, 3 hours ago 
On Sat, Sep 20, 2025 at 03:01:39PM +0300, Gal Horowitz 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 patch allocates a structure for each tap at startup.
> 
> Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
> ---
>  net/tap-win32.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 38baf90e0b3f121f74eb32f1bff779c84ce03114..217a43cc2f5effdd92e1bf49466fe8d2cd0490e6 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -114,8 +114,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;
> @@ -605,6 +603,7 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
>      } version;
>      DWORD version_len;
>      DWORD idThread;
> +    tap_win32_overlapped_t *tap_overlapped = NULL;
>  
>      if (preferred_name != NULL) {
>          snprintf(name_buffer, sizeof(name_buffer), "%s", preferred_name);
> @@ -645,12 +644,14 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
>          return -1;
>      }
>  
> -    tap_win32_overlapped_init(&tap_overlapped, handle);
> +    tap_overlapped = g_new0(tap_win32_overlapped_t, 1);
> +
> +    tap_win32_overlapped_init(tap_overlapped, handle);

I'd suggest chaing tap_win32_overlapped_init to be
tap_win32_overlapped_new. Have it be responsible
for the g_new0 call and returning the allocate struct
instead of passing it in as a param. 

>  
> -    *phandle = &tap_overlapped;
> +    *phandle = tap_overlapped;

eg so this becomes

  *phandle = tap_win32_overlapped_new(handle);

>  
>      CreateThread(NULL, 0, tap_win32_thread_entry,
> -                 (LPVOID)&tap_overlapped, 0, &idThread);
> +                 (LPVOID)tap_overlapped, 0, &idThread);
>      return 0;
>  }
>  
> @@ -670,6 +671,9 @@ static void tap_cleanup(NetClientState *nc)
>      /* FIXME: need to kill thread and close file handle:
>         tap_win32_close(s);
>      */
> +
> +   g_free(s->handle);
> +   s->handle = NULL;

The tap_overlapped_t struct contains many HANDLE fields. If we just
free the struct, then those handles are all leaked. There are also
some allocated pointers. We'd hope they would all be released already
but who knows ?

This is a pre-existing problem as the current code did not attempt
to free anything, but with your changes the leak stands out more.

At the same time though, the FIXME comment points out a risk here.

The thread is still running and yet we're freeing the 's->handle'
that the thread has access to. So if we don't stop the thread, we
are at risk of a use-after-free.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] tap-win32: fix multiple tap support
Posted by Gal Horowitz 4 days, 19 hours ago 
Hey Daniel, thanks for the review!

On Tue, 23 Sept 2025 at 14:34, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Sat, Sep 20, 2025 at 03:01:39PM +0300, Gal Horowitz 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 patch allocates a structure for each tap at startup.
> >
> > Signed-off-by: Gal Horowitz <galush.horowitz@gmail.com>
> > ---
> >  net/tap-win32.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/tap-win32.c b/net/tap-win32.c
> > index 38baf90e0b3f121f74eb32f1bff779c84ce03114..217a43cc2f5effdd92e1bf49466fe8d2cd0490e6 100644
> > --- a/net/tap-win32.c
> > +++ b/net/tap-win32.c
> > @@ -114,8 +114,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;
> > @@ -605,6 +603,7 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
> >      } version;
> >      DWORD version_len;
> >      DWORD idThread;
> > +    tap_win32_overlapped_t *tap_overlapped = NULL;
> >
> >      if (preferred_name != NULL) {
> >          snprintf(name_buffer, sizeof(name_buffer), "%s", preferred_name);
> > @@ -645,12 +644,14 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
> >          return -1;
> >      }
> >
> > -    tap_win32_overlapped_init(&tap_overlapped, handle);
> > +    tap_overlapped = g_new0(tap_win32_overlapped_t, 1);
> > +
> > +    tap_win32_overlapped_init(tap_overlapped, handle);
>
> I'd suggest chaing tap_win32_overlapped_init to be
> tap_win32_overlapped_new. Have it be responsible
> for the g_new0 call and returning the allocate struct
> instead of passing it in as a param.

Will do.

>
> >
> > -    *phandle = &tap_overlapped;
> > +    *phandle = tap_overlapped;
>
> eg so this becomes
>
>   *phandle = tap_win32_overlapped_new(handle);
>
> >
> >      CreateThread(NULL, 0, tap_win32_thread_entry,
> > -                 (LPVOID)&tap_overlapped, 0, &idThread);
> > +                 (LPVOID)tap_overlapped, 0, &idThread);
> >      return 0;
> >  }
> >
> > @@ -670,6 +671,9 @@ static void tap_cleanup(NetClientState *nc)
> >      /* FIXME: need to kill thread and close file handle:
> >         tap_win32_close(s);
> >      */
> > +
> > +   g_free(s->handle);
> > +   s->handle = NULL;
>
> The tap_overlapped_t struct contains many HANDLE fields. If we just
> free the struct, then those handles are all leaked. There are also
> some allocated pointers. We'd hope they would all be released already
> but who knows ?
>
> This is a pre-existing problem as the current code did not attempt
> to free anything, but with your changes the leak stands out more.
>
> At the same time though, the FIXME comment points out a risk here.
>
> The thread is still running and yet we're freeing the 's->handle'
> that the thread has access to. So if we don't stop the thread, we
> are at risk of a use-after-free.
>

I'll add a best-effort to clean up handles and such. I'll also
terminate the thread before freeing.

With Regards,
Gal

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.