* [PATCH 0/2] io: fix crash in VNC websock server when client quits early @ 2025-09-30 11:08 Daniel P. Berrangé 2025-09-30 11:08 ` [PATCH 1/2] io: move websock resource release to close method Daniel P. Berrangé 2025-09-30 11:08 ` [PATCH 2/2] io: fix use after free in websocket handshake code Daniel P. Berrangé 0 siblings, 2 replies; 7+ messages in thread From: Daniel P. Berrangé @ 2025-09-30 11:08 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Grant Millar | Cylo, Marc-André Lureau 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] io: move websock resource release to close method 2025-09-30 11:08 [PATCH 0/2] io: fix crash in VNC websock server when client quits early Daniel P. Berrangé @ 2025-09-30 11:08 ` Daniel P. Berrangé 2025-09-30 11:21 ` Marc-André Lureau 2025-09-30 11:08 ` [PATCH 2/2] io: fix use after free in websocket handshake code Daniel P. Berrangé 1 sibling, 1 reply; 7+ messages in thread From: Daniel P. Berrangé @ 2025-09-30 11:08 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Grant Millar | Cylo, Marc-André Lureau 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io: move websock resource release to close method 2025-09-30 11:08 ` [PATCH 1/2] io: move websock resource release to close method Daniel P. Berrangé @ 2025-09-30 11:21 ` Marc-André Lureau 2025-09-30 11:28 ` Daniel P. Berrangé 0 siblings, 1 reply; 7+ messages in thread From: Marc-André Lureau @ 2025-09-30 11:21 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: qemu-devel, Grant Millar | Cylo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io: move websock resource release to close method 2025-09-30 11:21 ` Marc-André Lureau @ 2025-09-30 11:28 ` Daniel P. Berrangé 0 siblings, 0 replies; 7+ messages in thread From: Daniel P. Berrangé @ 2025-09-30 11:28 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, Grant Millar | Cylo 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 :| ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] io: fix use after free in websocket handshake code 2025-09-30 11:08 [PATCH 0/2] io: fix crash in VNC websock server when client quits early Daniel P. Berrangé 2025-09-30 11:08 ` [PATCH 1/2] io: move websock resource release to close method Daniel P. Berrangé @ 2025-09-30 11:08 ` Daniel P. Berrangé 2025-09-30 11:26 ` Marc-André Lureau 1 sibling, 1 reply; 7+ messages in thread From: Daniel P. Berrangé @ 2025-09-30 11:08 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Grant Millar | Cylo, Marc-André Lureau 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] io: fix use after free in websocket handshake code 2025-09-30 11:08 ` [PATCH 2/2] io: fix use after free in websocket handshake code Daniel P. Berrangé @ 2025-09-30 11:26 ` Marc-André Lureau 2025-09-30 11:30 ` Daniel P. Berrangé 0 siblings, 1 reply; 7+ messages in thread From: Marc-André Lureau @ 2025-09-30 11:26 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: qemu-devel, Grant Millar | Cylo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] io: fix use after free in websocket handshake code 2025-09-30 11:26 ` Marc-André Lureau @ 2025-09-30 11:30 ` Daniel P. Berrangé 0 siblings, 0 replies; 7+ messages in thread From: Daniel P. Berrangé @ 2025-09-30 11:30 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, Grant Millar | Cylo 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 :| ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-09-30 11:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-30 11:08 [PATCH 0/2] io: fix crash in VNC websock server when client quits early Daniel P. Berrangé 2025-09-30 11:08 ` [PATCH 1/2] io: move websock resource release to close method Daniel P. Berrangé 2025-09-30 11:21 ` Marc-André Lureau 2025-09-30 11:28 ` Daniel P. Berrangé 2025-09-30 11:08 ` [PATCH 2/2] io: fix use after free in websocket handshake code Daniel P. Berrangé 2025-09-30 11:26 ` Marc-André Lureau 2025-09-30 11:30 ` Daniel P. Berrangé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.