From 7b61cc82772d994a3604f4599178d40d47b09b3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 19 Feb 2025 04:32:41 +0100 Subject: [PATCH 1/4] glib/gmain: Avoid potential dispose race on GSource due to refcount dance During g_source_unref() we might end up calling dispose from multiple threads, and due to the ref/unref dance we were doing we could end up initiating a GSource finalization while another thread was about to revive the source. This was because we were unreffing a source in a thread, and potentially re-reffing it, at the same time, but it was not guaranteed that the final decrement and test couldn't be followed by a further re-ref. To avoid this, do not do any ref/unref/re-ref/re-unref dance while we're about to dispose, but instead follow a bit more the g_object_unref() logic, and start the disposal phase only if we're about to drop the last reference, and only after the potential disposal call is done, we do an actual ref count decrement, which may lead to the finalization or not. We don't bother following the same logic at later point, since after disposal we should really have just one thread running and revival of a finalizing GSource isn't supported anyways. With this logic we can also avoid doing unneeded context locking when we've enough references on a GSource that disposal is unlikely to happen. Closes: #3612 --- glib/gmain.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 73b4ed93b..d9c84351f 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -2210,34 +2210,57 @@ g_source_unref_internal (GSource *source, { gpointer old_cb_data = NULL; GSourceCallbackFuncs *old_cb_funcs = NULL; + int old_ref; g_return_if_fail (source != NULL); + old_ref = g_atomic_int_get (&source->ref_count); + +retry_beginning: + if (old_ref > 1) + { + /* We have many references. If we can decrement the ref counter, we are done. */ + if (!g_atomic_int_compare_and_exchange_full ((int *) &source->ref_count, + old_ref, old_ref - 1, + &old_ref)) + goto retry_beginning; + + return; + } + if (!have_lock && context) LOCK_CONTEXT (context); - if (g_atomic_int_dec_and_test (&source->ref_count)) + /* We are about to drop the last reference, there's not guarantee at this + * point that another thread already changed the value at this point or + * that is also entering the disposal phase, but there is no much we can do + * and dropping the reference too early would be still risky since it could + * lead to a preventive finalization. + * So let's just get all the threads that reached this point to get in, while + * the final check on whether is the case or not to continue with the + * finalization will be done by a final unique atomic dec and test. + */ + if (old_ref == 1) { /* If there's a dispose function, call this first */ if (source->priv->dispose) { - /* Temporarily increase the ref count again so that GSource methods - * can be called from dispose(). */ - g_atomic_int_inc (&source->ref_count); if (context) UNLOCK_CONTEXT (context); source->priv->dispose (source); if (context) LOCK_CONTEXT (context); + } - /* Now the reference count might be bigger than 0 again, in which - * case we simply return from here before freeing the source */ - if (!g_atomic_int_dec_and_test (&source->ref_count)) - { - if (!have_lock && context) - UNLOCK_CONTEXT (context); - return; - } + /* At this point the source can have been revived by any of the threads + * acting on it or it's really ready for being finalized. + */ + if (!g_atomic_int_dec_and_test (&source->ref_count)) + { + if (!have_lock && context) + UNLOCK_CONTEXT (context); + + return; } TRACE (GLIB_SOURCE_BEFORE_FREE (source, context, From 6052374c785d86528fa8b7a155068af1dc245d5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 19 Feb 2025 16:28:12 +0100 Subject: [PATCH 2/4] gmain: Go back to using the standard unref logic if the final unref failed If going from 1 -> 0 references failed, then we should restart the unref process, by potentially re-entering in the dispose function again if instead something else re-references us and we're going to drop the last one again. --- glib/gmain.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index d9c84351f..fbeeb8c41 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -2255,12 +2255,13 @@ retry_beginning: /* At this point the source can have been revived by any of the threads * acting on it or it's really ready for being finalized. */ - if (!g_atomic_int_dec_and_test (&source->ref_count)) + if (!g_atomic_int_compare_and_exchange_full ((int *) &source->ref_count, + 1, 0, &old_ref)) { if (!have_lock && context) UNLOCK_CONTEXT (context); - return; + goto retry_beginning; } TRACE (GLIB_SOURCE_BEFORE_FREE (source, context, From 5d117b021a8817950b5acca707ce015aac922cee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 19 Feb 2025 05:09:34 +0100 Subject: [PATCH 3/4] gmain: Avoid unneeded atomic read on GSource ref In g_source_ref() we can just ensure that the value we've referenced was valid, instead of checking it before, since if get to such state the GSource is broken anyways, but at least we can avoid doing an unneeded atomic read. Reason why we don't bother resetting the reference in case of failure. --- glib/gmain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index fbeeb8c41..7258eade6 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -2191,12 +2191,13 @@ g_source_set_name_by_id (guint tag, GSource * g_source_ref (GSource *source) { + int old_ref G_GNUC_UNUSED; g_return_val_if_fail (source != NULL, NULL); + + old_ref = g_atomic_int_add (&source->ref_count, 1); /* We allow ref_count == 0 here to allow the dispose function to resurrect * the GSource if needed */ - g_return_val_if_fail (g_atomic_int_get (&source->ref_count) >= 0, NULL); - - g_atomic_int_inc (&source->ref_count); + g_return_val_if_fail (old_ref >= 0, NULL); return source; } From 709735959711a5b387721db2be6d22df6ab55768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 19 Feb 2025 05:18:44 +0100 Subject: [PATCH 4/4] gmain: Ensure old reference is valid during unref We were checking this already, leading to an extra atomic read that isn't needed. --- glib/gmain.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/glib/gmain.c b/glib/gmain.c index 7258eade6..c89ffdc73 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -2229,6 +2229,8 @@ retry_beginning: return; } + g_return_if_fail (old_ref > 0); + if (!have_lock && context) LOCK_CONTEXT (context); @@ -2359,7 +2361,7 @@ void g_source_unref (GSource *source) { g_return_if_fail (source != NULL); - g_return_if_fail (g_atomic_int_get (&source->ref_count) > 0); + /* refcount is checked inside g_source_unref_internal() */ g_source_unref_internal (source, source->context, FALSE); }