Daniel P. Berrangé <berra...@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 <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
