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 <[email protected]> > --- > 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 :|
