Hey Daniel, thanks for the review! On Tue, 23 Sept 2025 at 14:34, Daniel P. Berrangé <[email protected]> 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 <[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.
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
