From b06c48de7554607ff3fb58d6c0510cfa5088e909 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 3 Feb 2020 15:38:28 +0200 Subject: [PATCH 1/4] GMainContext - Fix GSource iterator if iteration can modify the list We first have to ref the next source and then unref the previous one. This might be the last reference to the previous source, and freeing the previous source might unref and free the next one which would then leave use with a dangling pointer here. Fixes https://gitlab.gnome.org/GNOME/glib/issues/2031 --- glib/gmain.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 1b4260b113..0e65aac14d 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -1004,13 +1004,17 @@ g_source_iter_next (GSourceIter *iter, GSource **source) * GSourceList to be removed from source_lists (if iter->source is * the only source in its list, and it is destroyed), so we have to * keep it reffed until after we advance iter->current_list, above. + * + * Also we first have to ref the next source before unreffing the + * previous one as unreffing the previous source can potentially + * free the next one. */ + if (next_source && iter->may_modify) + g_source_ref (next_source); if (iter->source && iter->may_modify) g_source_unref_internal (iter->source, iter->context, TRUE); iter->source = next_source; - if (iter->source && iter->may_modify) - g_source_ref (iter->source); *source = iter->source; return *source != NULL; -- GitLab From aa20167d419c649f34fed06a9463890b41b1eba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 3 Feb 2020 15:35:51 +0200 Subject: [PATCH 2/4] GMainContext - Fix memory leaks and memory corruption when freeing sources while freeing a context Instead of destroying sources directly while freeing the context, and potentially freeing them if this was the last reference to them, collect new references of all sources in a separate list before and at the same time invalidate their context so that they can't access it anymore. Only once all sources have their context invalidated, destroy them while still keeping a reference to them. Once all sources are destroyed we get rid of the additional references and free them if nothing else keeps a reference to them anymore. This fixes a regression introduced by 26056558be in 2012. The previous code that invalidated the context of each source and then destroyed it before going to the next source without keeping an additional reference caused memory leaks or memory corruption depending on the order of the sources in the sources lists. If a source was destroyed it might happen that this was the last reference to this source, and it would then be freed. This would cause the finalize function to be called, which might destroy and unref another source and potentially free it. This other source would then either - go through the normal free logic and change the intern linked list between the sources, while other sources that are unreffed as part of the main context freeing would not. As such the list would be in an inconsistent state and we might dereference freed memory. - go through the normal destroy and free logic but because the context pointer was already invalidated it would simply mark the source as destroyed without actually removing it from the context. This would then cause a memory leak because the reference owned by the context is not freed. Fixes https://github.com/gtk-rs/glib/issues/583 while still keeping https://bugzilla.gnome.org/show_bug.cgi?id=661767 fixes. --- glib/gmain.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/glib/gmain.c b/glib/gmain.c index 0e65aac14d..41f1370d70 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -539,6 +539,7 @@ g_main_context_unref (GMainContext *context) GSourceIter iter; GSource *source; GList *sl_iter; + GSList *s_iter, *remaining_sources = NULL; GSourceList *list; guint i; @@ -558,10 +559,30 @@ g_main_context_unref (GMainContext *context) /* g_source_iter_next() assumes the context is locked. */ LOCK_CONTEXT (context); - g_source_iter_init (&iter, context, TRUE); + + /* First collect all remaining sources from the sources lists and store a + * new reference in a separate list. Also set the context of the sources + * to NULL so that they can't access a partially destroyed context anymore. + * + * We have to do this first so that we have a strong reference to all + * sources and destroying them below does not also free them, and so that + * none of the sources can access the context from their finalize/dispose + * functions. */ + g_source_iter_init (&iter, context, FALSE); while (g_source_iter_next (&iter, &source)) { source->context = NULL; + remaining_sources = g_slist_prepend (remaining_sources, g_source_ref (source)); + } + g_source_iter_clear (&iter); + + /* Next destroy all sources. As we still hold a reference to all of them, + * this won't cause any of them to be freed yet and especially prevents any + * source that unrefs another source from its finalize function to be freed. + */ + for (s_iter = remaining_sources; s_iter; s_iter = s_iter->next) + { + source = s_iter->data; g_source_destroy_internal (source, context, TRUE); } UNLOCK_CONTEXT (context); @@ -586,6 +607,18 @@ g_main_context_unref (GMainContext *context) g_cond_clear (&context->cond); g_free (context); + + /* And now finally get rid of our references to the sources. This will cause + * them to be freed unless something else still has a reference to them. Due + * to setting the context pointers in the sources to NULL above, this won't + * ever access the context or the internal linked list inside the GSource. + * We already removed the sources completely from the context above. */ + for (s_iter = remaining_sources; s_iter; s_iter = s_iter->next) + { + source = s_iter->data; + g_source_unref_internal (source, NULL, FALSE); + } + g_slist_free (remaining_sources); } /* Helper function used by mainloop/overflow test. -- GitLab From 730a75fc8e8271c38fbd5363d1f77a00876b9ddc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Tue, 11 Feb 2020 09:34:38 +0200 Subject: [PATCH 3/4] GMainContext - Move mutex unlocking in destructor right before freeing the mutex This does not have any behaviour changes but is cleaner. The mutex is only unlocked now after all operations on the context are done and right before freeing the mutex and the context itself. --- glib/gmain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gmain.c b/glib/gmain.c index 41f1370d70..ee23a6519f 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -585,7 +585,6 @@ g_main_context_unref (GMainContext *context) source = s_iter->data; g_source_destroy_internal (source, context, TRUE); } - UNLOCK_CONTEXT (context); for (sl_iter = context->source_lists; sl_iter; sl_iter = sl_iter->next) { @@ -596,6 +595,7 @@ g_main_context_unref (GMainContext *context) g_hash_table_destroy (context->sources); + UNLOCK_CONTEXT (context); g_mutex_clear (&context->mutex); g_ptr_array_free (context->pending_dispatches, TRUE); -- GitLab From aaa7640f7cebbdbe4b65bdd56fdb7e35fac7ea2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 3 Feb 2020 18:00:12 +0200 Subject: [PATCH 4/4] GMainContext - Add tests for !1353 and related situations --- glib/tests/mainloop.c | 174 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/glib/tests/mainloop.c b/glib/tests/mainloop.c index a6dcc794fe..1368e9bc05 100644 --- a/glib/tests/mainloop.c +++ b/glib/tests/mainloop.c @@ -1832,14 +1832,188 @@ test_maincontext_source_finalization (void) g_assert_true (source_finalize_called); } +/* GSource implementation which optionally keeps a strong reference to another + * GSource until finalization, when it destroys and unrefs the other source. + */ +typedef struct { + GSource source; + + GSource *other_source; +} SourceWithSource; + +static void +finalize_source_with_source (GSource *source) +{ + SourceWithSource *s = (SourceWithSource *) source; + + if (s->other_source) + { + g_source_destroy (s->other_source); + g_source_unref (s->other_source); + s->other_source = NULL; + } +} + +static GSourceFuncs source_with_source_funcs = { + NULL, + NULL, + NULL, + finalize_source_with_source +}; + +static void +test_maincontext_source_finalization_from_source (gconstpointer user_data) +{ + GMainContext *c = g_main_context_new (); + GSource *s1, *s2; + + g_test_summary ("Tests if freeing a GSource as part of another GSource " + "during main context destruction works."); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/merge_requests/1353"); + + s1 = g_source_new (&source_with_source_funcs, sizeof (SourceWithSource)); + s2 = g_source_new (&source_with_source_funcs, sizeof (SourceWithSource)); + ((SourceWithSource *)s1)->other_source = g_source_ref (s2); + + /* Attach sources in a different order so they end up being destroyed + * in a different order by the main context */ + if (GPOINTER_TO_INT (user_data) < 5) + { + g_source_attach (s1, c); + g_source_attach (s2, c); + } + else + { + g_source_attach (s2, c); + g_source_attach (s1, c); + } + + /* Test a few different permutations here */ + if (GPOINTER_TO_INT (user_data) % 5 == 0) + { + /* Get rid of our references so that destroying the context + * will unref them immediately */ + g_source_unref (s1); + g_source_unref (s2); + g_main_context_unref (c); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 1) + { + /* Destroy and free the sources before the context */ + g_source_destroy (s1); + g_source_unref (s1); + g_source_destroy (s2); + g_source_unref (s2); + g_main_context_unref (c); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 2) + { + /* Destroy and free the sources before the context */ + g_source_destroy (s2); + g_source_unref (s2); + g_source_destroy (s1); + g_source_unref (s1); + g_main_context_unref (c); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 3) + { + /* Destroy and free the context before the sources */ + g_main_context_unref (c); + g_source_unref (s2); + g_source_unref (s1); + } + else if (GPOINTER_TO_INT (user_data) % 5 == 4) + { + /* Destroy and free the context before the sources */ + g_main_context_unref (c); + g_source_unref (s1); + g_source_unref (s2); + } +} + +static gboolean +dispatch_source_with_source (GSource *source, GSourceFunc callback, gpointer user_data) +{ + return G_SOURCE_REMOVE; +} + +static GSourceFuncs source_with_source_funcs_dispatch = { + NULL, + NULL, + dispatch_source_with_source, + finalize_source_with_source +}; + +static void +test_maincontext_source_finalization_from_dispatch (gconstpointer user_data) +{ + GMainContext *c = g_main_context_new (); + GSource *s1, *s2; + + g_test_summary ("Tests if freeing a GSource as part of another GSource " + "during main context iteration works."); + + s1 = g_source_new (&source_with_source_funcs_dispatch, sizeof (SourceWithSource)); + s2 = g_source_new (&source_with_source_funcs_dispatch, sizeof (SourceWithSource)); + ((SourceWithSource *)s1)->other_source = g_source_ref (s2); + + g_source_attach (s1, c); + g_source_attach (s2, c); + + if (GPOINTER_TO_INT (user_data) == 0) + { + /* This finalizes s1 as part of the iteration, which then destroys and + * frees s2 too */ + g_source_set_ready_time (s1, 0); + } + else if (GPOINTER_TO_INT (user_data) == 1) + { + /* This destroys s2 as part of the iteration but does not free it as + * it's still referenced by s1 */ + g_source_set_ready_time (s2, 0); + } + else if (GPOINTER_TO_INT (user_data) == 2) + { + /* This destroys both s1 and s2 as part of the iteration */ + g_source_set_ready_time (s1, 0); + g_source_set_ready_time (s2, 0); + } + + /* Get rid of our references so only the main context has one now */ + g_source_unref (s1); + g_source_unref (s2); + + /* Iterate as long as there are sources to dispatch */ + while (g_main_context_iteration (c, FALSE)) + { + /* Do nothing here */ + } + + g_main_context_unref (c); +} + int main (int argc, char *argv[]) { + gint i; + g_test_init (&argc, &argv, NULL); g_test_bug_base ("http://bugzilla.gnome.org/"); g_test_add_func ("/maincontext/basic", test_maincontext_basic); g_test_add_func ("/maincontext/source_finalization", test_maincontext_source_finalization); + for (i = 0; i < 10; i++) + { + gchar *name = g_strdup_printf ("/maincontext/source_finalization_from_source/%d", i); + g_test_add_data_func (name, GINT_TO_POINTER (i), test_maincontext_source_finalization_from_source); + g_free (name); + } + for (i = 0; i < 3; i++) + { + gchar *name = g_strdup_printf ("/maincontext/source_finalization_from_dispatch/%d", i); + g_test_add_data_func (name, GINT_TO_POINTER (i), test_maincontext_source_finalization_from_dispatch); + g_free (name); + } g_test_add_func ("/mainloop/basic", test_mainloop_basic); g_test_add_func ("/mainloop/timeouts", test_timeouts); g_test_add_func ("/mainloop/priorities", test_priorities); -- GitLab