1. Patchew
  2. QEMU
  3. [PATCH 0/2] io: fix crash in VNC websock server when client quits early
  4. View patch

[PATCH 2/2] io: fix use after free in websocket handshake code

Daniel P. Berrangé posted 2 patches 1 month, 2 weeks ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>
There is a newer version of this series
[PATCH 2/2] io: fix use after free in websocket handshake code
Posted by Daniel P. Berrangé 1 month, 2 weeks ago 
If the QIOChannelWebsock object is freed while it is waiting to
complete a handshake, a GSource is leaked. This can lead to the
callback firing later on and triggering a use-after-free in the
use of the channel. This was observed in the VNC server with the
following trace from valgrind:

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

The above can be reproduced by launching QEMU with

  $ qemu-system-x86_64 -vnc localhost:0,websocket=5700

and then repeatedly running:

  for i in {1..100}; do
     (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 localhost 5700 &
  done

Reported-by: Grant Millar | Cylo <rid@cylo.io>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/io/channel-websock.h |  1 +
 io/channel-websock.c         | 16 ++++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index e180827c57..d1e760e449 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -61,6 +61,7 @@ struct QIOChannelWebsock {
     size_t payload_remain;
     size_t pong_remain;
     QIOChannelWebsockMask mask;
+    guint hs_io_tag;
     guint io_tag;
     Error *io_err;
     gboolean io_eof;
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 56d53355d5..588c313dfb 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -597,7 +597,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
     error_propagate(&wioc->io_err, err);
 
     trace_qio_channel_websock_handshake_reply(ioc);
-    qio_channel_add_watch(
+    wioc->hs_io_tag = qio_channel_add_watch(
         wioc->master,
         G_IO_OUT,
         qio_channel_websock_handshake_send,
@@ -907,11 +907,12 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc,
 
     trace_qio_channel_websock_handshake_start(ioc);
     trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN);
-    qio_channel_add_watch(ioc->master,
-                          G_IO_IN,
-                          qio_channel_websock_handshake_io,
-                          task,
-                          NULL);
+    ioc->hs_io_tag = qio_channel_add_watch(
+        ioc->master,
+        G_IO_IN,
+        qio_channel_websock_handshake_io,
+        task,
+        NULL);
 }
 
 
@@ -1212,6 +1213,9 @@ static int qio_channel_websock_close(QIOChannel *ioc,
     buffer_free(&wioc->encinput);
     buffer_free(&wioc->encoutput);
     buffer_free(&wioc->rawinput);
+    if (wioc->hs_io_tag) {
+        g_clear_handle_id(&wioc->hs_io_tag, g_source_remove);
+    }
     if (wioc->io_tag) {
         g_clear_handle_id(&wioc->io_tag, g_source_remove);
     }
-- 
2.50.1
Re: [PATCH 2/2] io: fix use after free in websocket handshake code
Posted by Marc-André Lureau 1 month, 2 weeks ago 
Hi

On Tue, Sep 30, 2025 at 3:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> If the QIOChannelWebsock object is freed while it is waiting to
> complete a handshake, a GSource is leaked. This can lead to the
> callback firing later on and triggering a use-after-free in the
> use of the channel. This was observed in the VNC server with the
> following trace from valgrind:
>
> ==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==
>
> The above can be reproduced by launching QEMU with
>
>   $ qemu-system-x86_64 -vnc localhost:0,websocket=5700
>
> and then repeatedly running:
>
>   for i in {1..100}; do
>      (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 localhost 5700 &
>   done
>
> Reported-by: Grant Millar | Cylo <rid@cylo.io>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/io/channel-websock.h |  1 +
>  io/channel-websock.c         | 16 ++++++++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
> index e180827c57..d1e760e449 100644
> --- a/include/io/channel-websock.h
> +++ b/include/io/channel-websock.h
> @@ -61,6 +61,7 @@ struct QIOChannelWebsock {
>      size_t payload_remain;
>      size_t pong_remain;
>      QIOChannelWebsockMask mask;
> +    guint hs_io_tag;
>      guint io_tag;

I think it's worth a comment on why it needs two tags, and how they
relate to each other.

>      Error *io_err;
>      gboolean io_eof;
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 56d53355d5..588c313dfb 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -597,7 +597,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
>      error_propagate(&wioc->io_err, err);
>
>      trace_qio_channel_websock_handshake_reply(ioc);
> -    qio_channel_add_watch(
> +    wioc->hs_io_tag = qio_channel_add_watch(
>          wioc->master,
>          G_IO_OUT,
>          qio_channel_websock_handshake_send,
> @@ -907,11 +907,12 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc,
>
>      trace_qio_channel_websock_handshake_start(ioc);
>      trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN);
> -    qio_channel_add_watch(ioc->master,
> -                          G_IO_IN,
> -                          qio_channel_websock_handshake_io,
> -                          task,
> -                          NULL);
> +    ioc->hs_io_tag = qio_channel_add_watch(
> +        ioc->master,
> +        G_IO_IN,
> +        qio_channel_websock_handshake_io,
> +        task,
> +        NULL);

There is still an untracked qio_channel_add_watch(). I suppose you
checked that, it could use a comment explaining why it's left.

>  }
>
>
> @@ -1212,6 +1213,9 @@ static int qio_channel_websock_close(QIOChannel *ioc,
>      buffer_free(&wioc->encinput);
>      buffer_free(&wioc->encoutput);
>      buffer_free(&wioc->rawinput);
> +    if (wioc->hs_io_tag) {
> +        g_clear_handle_id(&wioc->hs_io_tag, g_source_remove);
> +    }
>      if (wioc->io_tag) {
>          g_clear_handle_id(&wioc->io_tag, g_source_remove);
>      }
> --
> 2.50.1
>


-- 
Marc-André Lureau
Re: [PATCH 2/2] io: fix use after free in websocket handshake code
Posted by Daniel P. Berrangé 1 month, 2 weeks ago 
On Tue, Sep 30, 2025 at 03:26:30PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 30, 2025 at 3:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > If the QIOChannelWebsock object is freed while it is waiting to
> > complete a handshake, a GSource is leaked. This can lead to the
> > callback firing later on and triggering a use-after-free in the
> > use of the channel. This was observed in the VNC server with the
> > following trace from valgrind:
> >
> > ==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==
> >
> > The above can be reproduced by launching QEMU with
> >
> >   $ qemu-system-x86_64 -vnc localhost:0,websocket=5700
> >
> > and then repeatedly running:
> >
> >   for i in {1..100}; do
> >      (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 localhost 5700 &
> >   done
> >
> > Reported-by: Grant Millar | Cylo <rid@cylo.io>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/io/channel-websock.h |  1 +
> >  io/channel-websock.c         | 16 ++++++++++------
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
> > index e180827c57..d1e760e449 100644
> > --- a/include/io/channel-websock.h
> > +++ b/include/io/channel-websock.h
> > @@ -61,6 +61,7 @@ struct QIOChannelWebsock {
> >      size_t payload_remain;
> >      size_t pong_remain;
> >      QIOChannelWebsockMask mask;
> > +    guint hs_io_tag;
> >      guint io_tag;
> 
> I think it's worth a comment on why it needs two tags, and how they
> relate to each other.
> 
> >      Error *io_err;
> >      gboolean io_eof;
> > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > index 56d53355d5..588c313dfb 100644
> > --- a/io/channel-websock.c
> > +++ b/io/channel-websock.c
> > @@ -597,7 +597,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
> >      error_propagate(&wioc->io_err, err);
> >
> >      trace_qio_channel_websock_handshake_reply(ioc);
> > -    qio_channel_add_watch(
> > +    wioc->hs_io_tag = qio_channel_add_watch(
> >          wioc->master,
> >          G_IO_OUT,
> >          qio_channel_websock_handshake_send,
> > @@ -907,11 +907,12 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc,
> >
> >      trace_qio_channel_websock_handshake_start(ioc);
> >      trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN);
> > -    qio_channel_add_watch(ioc->master,
> > -                          G_IO_IN,
> > -                          qio_channel_websock_handshake_io,
> > -                          task,
> > -                          NULL);
> > +    ioc->hs_io_tag = qio_channel_add_watch(
> > +        ioc->master,
> > +        G_IO_IN,
> > +        qio_channel_websock_handshake_io,
> > +        task,
> > +        NULL);
> 
> There is still an untracked qio_channel_add_watch(). I suppose you
> checked that, it could use a comment explaining why it's left.

I don't see any untracked add_watch remaining ? (If you're just doing a
grep, you'll miss the prior line context where it captures the return
value)

> 
> >  }
> >
> >
> > @@ -1212,6 +1213,9 @@ static int qio_channel_websock_close(QIOChannel *ioc,
> >      buffer_free(&wioc->encinput);
> >      buffer_free(&wioc->encoutput);
> >      buffer_free(&wioc->rawinput);
> > +    if (wioc->hs_io_tag) {
> > +        g_clear_handle_id(&wioc->hs_io_tag, g_source_remove);
> > +    }
> >      if (wioc->io_tag) {
> >          g_clear_handle_id(&wioc->io_tag, g_source_remove);
> >      }
> > --
> > 2.50.1
> >
> 
> 
> -- 
> 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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.