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

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

[Grant Millar | Cylo]

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

Add a NULL check before calling qio_channel_close() to prevent the crash.

Signed-off-by: Rid <rid@cylo.io>
---
 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

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

[Marc-André Lureau]

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.

Could you provide a backtrace?

> 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

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

[Grant Millar | Cylo]

Sure, no problem. This is on 10.0.3 (Debian 1:10.0.3+ds-0+deb13u1).

Sometimes I have to run the reproducer a few times for the race
condition to occur.

#0  object_get_class (obj=obj@entry=0x0) at qom/object.c:1043
#1  0x0000556a3b671655 in QIO_CHANNEL_GET_CLASS (obj=0x0) at
./include/io/channel.h:29
#2  qio_channel_close (ioc=0x0, errp=0x0) at io/channel.c:380
#3  0x0000556a3b2036d9 in vnc_disconnect_start (vs=0x556a463d96a0) at
ui/vnc.c:1310
#4  0x0000556a3b2063d5 in vnc_disconnect_start (vs=0x556a463d96a0) at
ui/vnc.c:1398
#5  0x0000556a3b21b096 in vncws_handshake_done (task=<optimized out>,
user_data=0x556a463d96a0) at ui/vnc-ws.c:105
#6  0x0000556a3b673f54 in qio_task_complete (task=0x556a469a7fc0) at
io/task.c:197
#7  0x0000556a3b670a50 in qio_channel_websock_handshake_io
(ioc=0x556a45ec60e0, condition=<optimized out>,
user_data=0x556a469a7fc0) at io/channel-websock.c:588
#8  0x00007f4f6dca9385 in ??? () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#9  0x00007f4f6dcabc78 in g_main_context_dispatch () at
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x0000556a3b8015b8 in glib_pollfds_poll () at util/main-loop.c:287
#11 os_host_main_loop_wait (timeout=0) at util/main-loop.c:310
#12 main_loop_wait (nonblocking=nonblocking@entry=0) at util/main-loop.c:589
#13 0x0000556a3b44b360 in qemu_main_loop () at system/runstate.c:835
#14 0x0000556a3b746cb0 in qemu_default_main (opaque=opaque@entry=0x0)
at system/main.c:50
#15 0x0000556a3b1b4319 in main (argc=<optimized out>, argv=<optimized
out>) at system/main.c:80

On Tue, 30 Sept 2025 at 11:38, Marc-André Lureau
<marcandre.lureau@gmail.com> 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.
>
> Could you provide a backtrace?
>
> > 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

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

[Daniel P. Berrangé]

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

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

[Daniel P. Berrangé]

On Tue, Sep 30, 2025 at 10:35:01AM +0100, Grant Millar | Cylo 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
> 
> Add a NULL check before calling qio_channel_close() to prevent the crash.
> 
> Signed-off-by: Rid <rid@cylo.io>
> ---
>  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;

The NULL here is  just a symptom of a bigger problem earlier on and
thus the wrong thing to fix.

The QIOChannelWebsock is not unregistering the GSource callback when
it is closed. So we have closed the QIOChannel client connection,
freed the VncState struct, but still have a (now closed) FD registered
with the event loop for poll(). This eventually triggers the callback
which does a use-after-free on VncState which happens to not crash,
but returns a NULL QIOChannel which is passed to vnc_disconnect_start

We need to fix QIOChannelWebsock to remove the GSource.


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