[PATCH 0/2] io: fix crash in VNC websock server when client quits early

[PATCH 0/2] io: fix crash in VNC websock server when client quits early

[Daniel P. Berrangé]

See patch 2 for the description of the problem and reproducer

Daniel P. Berrangé (2):
  io: move websock resource release to close method
  io: fix use after free in websocket handshake code

 include/io/channel-websock.h |  1 +
 io/channel-websock.c         | 34 +++++++++++++++++++---------------
 2 files changed, 20 insertions(+), 15 deletions(-)

-- 
2.50.1

[PATCH 1/2] io: move websock resource release to close method

[Daniel P. Berrangé]

The QIOChannelWebsock object releases all its resources in the
finalize callback. This is too late, as callers expect to be
able to call qio_channel_close() to fully close a channel and
release resources related to I/O. Only releasing the underlying
QIOChannel transport can be delayed until finalize.

Furthermore the close callback must be robust against being
called multiple times. Thus when moving the code we now clear
the GSource ID using  g_clear_handle_id.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-websock.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 0a8c5c4712..56d53355d5 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -919,16 +919,7 @@ static void qio_channel_websock_finalize(Object *obj)
 {
     QIOChannelWebsock *ioc = QIO_CHANNEL_WEBSOCK(obj);
 
-    buffer_free(&ioc->encinput);
-    buffer_free(&ioc->encoutput);
-    buffer_free(&ioc->rawinput);
     object_unref(OBJECT(ioc->master));
-    if (ioc->io_tag) {
-        g_source_remove(ioc->io_tag);
-    }
-    if (ioc->io_err) {
-        error_free(ioc->io_err);
-    }
 }
 
 
@@ -1218,6 +1209,15 @@ static int qio_channel_websock_close(QIOChannel *ioc,
     QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
 
     trace_qio_channel_websock_close(ioc);
+    buffer_free(&wioc->encinput);
+    buffer_free(&wioc->encoutput);
+    buffer_free(&wioc->rawinput);
+    if (wioc->io_tag) {
+        g_clear_handle_id(&wioc->io_tag, g_source_remove);
+    }
+    if (wioc->io_err) {
+        g_clear_pointer(&wioc->io_err, error_free);
+    }
     return qio_channel_close(wioc->master, errp);
 }
 
-- 
2.50.1

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

[Daniel P. Berrangé]

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 1/2] io: move websock resource release to close method

[Marc-André Lureau]

Hi

On Tue, Sep 30, 2025 at 3:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The QIOChannelWebsock object releases all its resources in the
> finalize callback. This is too late, as callers expect to be
> able to call qio_channel_close() to fully close a channel and
> release resources related to I/O. Only releasing the underlying
> QIOChannel transport can be delayed until finalize.
>
> Furthermore the close callback must be robust against being
> called multiple times. Thus when moving the code we now clear
> the GSource ID using  g_clear_handle_id.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  io/channel-websock.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 0a8c5c4712..56d53355d5 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -919,16 +919,7 @@ static void qio_channel_websock_finalize(Object *obj)
>  {
>      QIOChannelWebsock *ioc = QIO_CHANNEL_WEBSOCK(obj);
>
> -    buffer_free(&ioc->encinput);
> -    buffer_free(&ioc->encoutput);
> -    buffer_free(&ioc->rawinput);
>      object_unref(OBJECT(ioc->master));
> -    if (ioc->io_tag) {
> -        g_source_remove(ioc->io_tag);
> -    }
> -    if (ioc->io_err) {
> -        error_free(ioc->io_err);
> -    }

Maybe finalize should call close() ? Otherwise, it's hard to guarantee
that there is no leak when doing simply init/finish.

>  }
>
>
> @@ -1218,6 +1209,15 @@ static int qio_channel_websock_close(QIOChannel *ioc,
>      QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
>
>      trace_qio_channel_websock_close(ioc);
> +    buffer_free(&wioc->encinput);
> +    buffer_free(&wioc->encoutput);
> +    buffer_free(&wioc->rawinput);
> +    if (wioc->io_tag) {
> +        g_clear_handle_id(&wioc->io_tag, g_source_remove);
> +    }
> +    if (wioc->io_err) {
> +        g_clear_pointer(&wioc->io_err, error_free);
> +    }
>      return qio_channel_close(wioc->master, errp);
>  }
>

otherwise lgtm

> --
> 2.50.1
>


-- 
Marc-André Lureau

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

[Marc-André Lureau]

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 1/2] io: move websock resource release to close method

[Daniel P. Berrangé]

On Tue, Sep 30, 2025 at 03:21:10PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 30, 2025 at 3:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The QIOChannelWebsock object releases all its resources in the
> > finalize callback. This is too late, as callers expect to be
> > able to call qio_channel_close() to fully close a channel and
> > release resources related to I/O. Only releasing the underlying
> > QIOChannel transport can be delayed until finalize.
> >
> > Furthermore the close callback must be robust against being
> > called multiple times. Thus when moving the code we now clear
> > the GSource ID using  g_clear_handle_id.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  io/channel-websock.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > index 0a8c5c4712..56d53355d5 100644
> > --- a/io/channel-websock.c
> > +++ b/io/channel-websock.c
> > @@ -919,16 +919,7 @@ static void qio_channel_websock_finalize(Object *obj)
> >  {
> >      QIOChannelWebsock *ioc = QIO_CHANNEL_WEBSOCK(obj);
> >
> > -    buffer_free(&ioc->encinput);
> > -    buffer_free(&ioc->encoutput);
> > -    buffer_free(&ioc->rawinput);
> >      object_unref(OBJECT(ioc->master));
> > -    if (ioc->io_tag) {
> > -        g_source_remove(ioc->io_tag);
> > -    }
> > -    if (ioc->io_err) {
> > -        error_free(ioc->io_err);
> > -    }
> 
> Maybe finalize should call close() ? Otherwise, it's hard to guarantee
> that there is no leak when doing simply init/finish.

I was in two minds about that. If close did happen implicitly during
finalize that is highly likely to be a bug in the usage, as you should
not finalize a I/O channel that is still in use. This patch matches the
approach we've taken in the TLS channel now.

> 
> >  }
> >
> >
> > @@ -1218,6 +1209,15 @@ static int qio_channel_websock_close(QIOChannel *ioc,
> >      QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
> >
> >      trace_qio_channel_websock_close(ioc);
> > +    buffer_free(&wioc->encinput);
> > +    buffer_free(&wioc->encoutput);
> > +    buffer_free(&wioc->rawinput);
> > +    if (wioc->io_tag) {
> > +        g_clear_handle_id(&wioc->io_tag, g_source_remove);
> > +    }
> > +    if (wioc->io_err) {
> > +        g_clear_pointer(&wioc->io_err, error_free);
> > +    }
> >      return qio_channel_close(wioc->master, errp);
> >  }
> >
> 
> otherwise lgtm
> 
> > --
> > 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 :|

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

[Daniel P. Berrangé]

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