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

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

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 1/2] io: move websock resource release to close method
Posted by Daniel P. Berrangé 1 month, 2 weeks ago 
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
Re: [PATCH 1/2] io: move websock resource release to close method
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:
>
> 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 1/2] io: move websock resource release to close method
Posted by Daniel P. Berrangé 1 month, 2 weeks ago 
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 :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.