On Thu, May 15, 2025 at 07:20:14PM -0300, Fabiano Rosas wrote:
> It's possible for the hup_source to have its reference decremented by
> remove_hup_source() while it's still being added to the context,
> leading to asserts in glib:
IIUC this must mean that
tcp_chr_free_connection
is being called concurrently with
update_ioc_handlers
I'm wondering if that is really intended, or a sign of a deeper
bug that we'll just paper over if we add the mutex proposed here.
Are you able to provide stack traces showing the 2 concurrent
operations that are triggering this problem ?
>
> g_source_set_callback_indirect: assertion 'g_atomic_int_get
> (&source->ref_count) > 0'
>
> g_source_attach: assertion 'g_atomic_int_get (&source->ref_count) > 0'
> failed
>
> Add a lock to serialize removal and creation.
>
> Signed-off-by: Fabiano Rosas <faro...@suse.de>
> ---
> chardev/char-socket.c | 4 ++++
> chardev/char.c | 2 ++
> include/chardev/char.h | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index d16608f1ed..88db9acd0d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -374,7 +374,9 @@ static void tcp_chr_free_connection(Chardev *chr)
> s->read_msgfds_num = 0;
> }
>
> + qemu_mutex_lock(&chr->hup_source_lock);
> remove_hup_source(s);
> + qemu_mutex_unlock(&chr->hup_source_lock);
>
> tcp_set_msgfds(chr, NULL, 0);
> remove_fd_in_watch(chr);
> @@ -613,6 +615,7 @@ static void update_ioc_handlers(SocketChardev *s)
> tcp_chr_read, chr,
> chr->gcontext);
>
> + qemu_mutex_lock(&chr->hup_source_lock);
> remove_hup_source(s);
> s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
> /*
> @@ -634,6 +637,7 @@ static void update_ioc_handlers(SocketChardev *s)
> g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
> chr, NULL);
> g_source_attach(s->hup_source, chr->gcontext);
> + qemu_mutex_unlock(&chr->hup_source_lock);
> }
>
> static void tcp_chr_connect(void *opaque)
> diff --git a/chardev/char.c b/chardev/char.c
> index bbebd246c3..d03f698b38 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -279,6 +279,7 @@ static void char_init(Object *obj)
> chr->handover_yank_instance = false;
> chr->logfd = -1;
> qemu_mutex_init(&chr->chr_write_lock);
> + qemu_mutex_init(&chr->hup_source_lock);
>
> /*
> * Assume if chr_update_read_handler is implemented it will
> @@ -316,6 +317,7 @@ static void char_finalize(Object *obj)
> close(chr->logfd);
> }
> qemu_mutex_destroy(&chr->chr_write_lock);
> + qemu_mutex_destroy(&chr->hup_source_lock);
> }
>
> static const TypeInfo char_type_info = {
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 429852f8d9..064184153d 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -60,6 +60,7 @@ struct Chardev {
> Object parent_obj;
>
> QemuMutex chr_write_lock;
> + QemuMutex hup_source_lock;
> CharBackend *be;
> char *label;
> char *filename;
> --
> 2.35.3
>
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 :|
