Re: [PATCH] ui/vnc: Fix NULL pointer dereference in vnc_disconnect_start

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

On Tue, Sep 30, 2025 at 02:38:40PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 30, 2025 at 1:59 PM Grant Millar | Cylo <rid@cylo.io> wrote:
> >
> > From 0d1c4ac000a66ef22b4a0cd0c4bedd840192096a Mon Sep 17 00:00:00 2001
> > From: Rid <rid@cylo.io>
> > Date: Tue, 30 Sep 2025 10:23:58 +0100
> > Subject: [PATCH] ui/vnc: Fix NULL pointer dereference in vnc_disconnect_start
> >
> > When a WebSocket connection fails during the handshake, vs->ioc can be
> > NULL when vnc_disconnect_start() is called, leading to a segmentation
> > fault when qio_channel_close() tries to dereference it.
> >
> > This can be reproduced by sending incomplete HTTP requests to the
> > WebSocket port:
> >
> >   for i in {1..100}; do
> >     (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 <IP> <PORT> &
> >   done
> >
> 
> I tried to reproduce without success.

This loop sometimes needs to be run 3 or 4 or more times before it
will trigger.

> Could you provide a backtrace?

With valgrind the problem shows up as:

==2523108== Invalid read of size 4
==2523108==    at 0x4054A24: vnc_disconnect_start (vnc.c:1296)
==2523108==    by 0x4054A24: vnc_client_error (vnc.c:1392)
==2523108==    by 0x4068A09: vncws_handshake_done (vnc-ws.c:105)
==2523108==    by 0x44863B4: qio_task_complete (task.c:197)
==2523108==    by 0x448343D: qio_channel_websock_handshake_io (channel-websock.c:588)
==2523108==    by 0x6EDB862: UnknownInlinedFun (gmain.c:3398)
==2523108==    by 0x6EDB862: g_main_context_dispatch_unlocked.lto_priv.0 (gmain.c:4249)
==2523108==    by 0x6EDBAE4: g_main_context_dispatch (gmain.c:4237)
==2523108==    by 0x45EC79F: glib_pollfds_poll (main-loop.c:287)
==2523108==    by 0x45EC79F: os_host_main_loop_wait (main-loop.c:310)
==2523108==    by 0x45EC79F: main_loop_wait (main-loop.c:589)
==2523108==    by 0x423A56D: qemu_main_loop (runstate.c:835)
==2523108==    by 0x454F300: qemu_default_main (main.c:37)
==2523108==    by 0x73D6574: (below main) (libc_start_call_main.h:58)
==2523108==  Address 0x57a6e0dc is 28 bytes inside a block of size 103,608 free'd
==2523108==    at 0x5F2FE43: free (vg_replace_malloc.c:989)
==2523108==    by 0x6EDC444: g_free (gmem.c:208)
==2523108==    by 0x4053F23: vnc_update_client (vnc.c:1153)
==2523108==    by 0x4053F23: vnc_refresh (vnc.c:3225)
==2523108==    by 0x4042881: dpy_refresh (console.c:880)
==2523108==    by 0x4042881: gui_update (console.c:90)
==2523108==    by 0x45EFA1B: timerlist_run_timers.part.0 (qemu-timer.c:562)
==2523108==    by 0x45EFC8F: timerlist_run_timers (qemu-timer.c:495)
==2523108==    by 0x45EFC8F: qemu_clock_run_timers (qemu-timer.c:576)
==2523108==    by 0x45EFC8F: qemu_clock_run_all_timers (qemu-timer.c:663)
==2523108==    by 0x45EC765: main_loop_wait (main-loop.c:600)
==2523108==    by 0x423A56D: qemu_main_loop (runstate.c:835)
==2523108==    by 0x454F300: qemu_default_main (main.c:37)
==2523108==    by 0x73D6574: (below main) (libc_start_call_main.h:58)
==2523108==  Block was alloc'd at
==2523108==    at 0x5F343F3: calloc (vg_replace_malloc.c:1675)
==2523108==    by 0x6EE2F81: g_malloc0 (gmem.c:133)
==2523108==    by 0x4057DA3: vnc_connect (vnc.c:3245)
==2523108==    by 0x448591B: qio_net_listener_channel_func (net-listener.c:54)
==2523108==    by 0x6EDB862: UnknownInlinedFun (gmain.c:3398)
==2523108==    by 0x6EDB862: g_main_context_dispatch_unlocked.lto_priv.0 (gmain.c:4249)
==2523108==    by 0x6EDBAE4: g_main_context_dispatch (gmain.c:4237)
==2523108==    by 0x45EC79F: glib_pollfds_poll (main-loop.c:287)
==2523108==    by 0x45EC79F: os_host_main_loop_wait (main-loop.c:310)
==2523108==    by 0x45EC79F: main_loop_wait (main-loop.c:589)
==2523108==    by 0x423A56D: qemu_main_loop (runstate.c:835)
==2523108==    by 0x454F300: qemu_default_main (main.c:37)
==2523108==    by 0x73D6574: (below main) (libc_start_call_main.h:58)
==2523108== 

I'm working on a fix for QIOChannelWebsock...  This VNC change is
wrong.

> 
> > Add a NULL check before calling qio_channel_close() to prevent the crash.
> >
> > Signed-off-by: Rid <rid@cylo.io>
> 
> Your mail is not properly formatted. git am fails.
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html
> 
 > 
> thanks
> 
> > ---
> >  ui/vnc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 77c823bf2e..1669ed1b80 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -1301,7 +1301,9 @@ static void vnc_disconnect_start(VncState *vs)
> >          g_source_remove(vs->ioc_tag);
> >          vs->ioc_tag = 0;
> >      }
> > -    qio_channel_close(vs->ioc, NULL);
> > +    if (vs->ioc) {
> > +        qio_channel_close(vs->ioc, NULL);
> > +    }
> >      vs->disconnecting = TRUE;
> >  }
> >
> > --
> > 2.39.5
> >
> 
> 
> -- 
> Marc-André Lureau
> 

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