Re: [PATCH] tap-win32: fix multiple tap support

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|