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:
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 <farosas@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.3On 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 <farosas@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 :|
Daniel P. Berrangé <berrange@redhat.com> writes:
> 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.
>
Yeah... I can't tell, I'm new to this code. But I agree that this smells
of a bug somewhere else.
> Are you able to provide stack traces showing the 2 concurrent
> operations that are triggering this problem ?
>
I wasn't able to, it triggers in the glib subprocess which is a pain to
debug. I'll give it another try now that there's fixes for the other
bugs.
>>
>> 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 <farosas@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
© 2016 - 2025 Red Hat, Inc.