Andrew Geissler | 82c905d | 2020-04-13 13:39:40 -0500 | [diff] [blame] | 1 | From 611430a32a46d0dc806a829161e2dccf9c0196a8 Mon Sep 17 00:00:00 2001 |
| 2 | From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= <sebastian@centricular.com> |
| 3 | Date: Mon, 3 Feb 2020 15:35:51 +0200 |
| 4 | Subject: [PATCH] GMainContext - Fix memory leaks and memory corruption when |
| 5 | freeing sources while freeing a context |
| 6 | |
| 7 | Instead of destroying sources directly while freeing the context, and |
| 8 | potentially freeing them if this was the last reference to them, collect |
| 9 | new references of all sources in a separate list before and at the same |
| 10 | time invalidate their context so that they can't access it anymore. Only |
| 11 | once all sources have their context invalidated, destroy them while |
| 12 | still keeping a reference to them. Once all sources are destroyed we get |
| 13 | rid of the additional references and free them if nothing else keeps a |
| 14 | reference to them anymore. |
| 15 | |
| 16 | This fixes a regression introduced by 26056558be in 2012. |
| 17 | |
| 18 | The previous code that invalidated the context of each source and then |
| 19 | destroyed it before going to the next source without keeping an |
| 20 | additional reference caused memory leaks or memory corruption depending |
| 21 | on the order of the sources in the sources lists. |
| 22 | |
| 23 | If a source was destroyed it might happen that this was the last |
| 24 | reference to this source, and it would then be freed. This would cause |
| 25 | the finalize function to be called, which might destroy and unref |
| 26 | another source and potentially free it. This other source would then |
| 27 | either |
| 28 | - go through the normal free logic and change the intern linked list |
| 29 | between the sources, while other sources that are unreffed as part of |
| 30 | the main context freeing would not. As such the list would be in an |
| 31 | inconsistent state and we might dereference freed memory. |
| 32 | - go through the normal destroy and free logic but because the context |
| 33 | pointer was already invalidated it would simply mark the source as |
| 34 | destroyed without actually removing it from the context. This would |
| 35 | then cause a memory leak because the reference owned by the context is |
| 36 | not freed. |
| 37 | |
| 38 | Fixes https://github.com/gtk-rs/glib/issues/583 while still keeping |
| 39 | https://bugzilla.gnome.org/show_bug.cgi?id=661767 fixes. |
| 40 | |
| 41 | Upstream-Status: Backport [https://gitlab.gnome.org/GNOME/glib/-/commit/aa20167d419c649f34fed06a9463890b41b1eba0] |
| 42 | |
| 43 | --- |
| 44 | glib/gmain.c | 35 ++++++++++++++++++++++++++++++++++- |
| 45 | 1 file changed, 34 insertions(+), 1 deletion(-) |
| 46 | |
| 47 | diff --git a/glib/gmain.c b/glib/gmain.c |
| 48 | index a9a287d..10ba2f8 100644 |
| 49 | --- a/glib/gmain.c |
| 50 | +++ b/glib/gmain.c |
| 51 | @@ -538,6 +538,7 @@ g_main_context_unref (GMainContext *context) |
| 52 | GSourceIter iter; |
| 53 | GSource *source; |
| 54 | GList *sl_iter; |
| 55 | + GSList *s_iter, *remaining_sources = NULL; |
| 56 | GSourceList *list; |
| 57 | guint i; |
| 58 | |
| 59 | @@ -557,10 +558,30 @@ g_main_context_unref (GMainContext *context) |
| 60 | |
| 61 | /* g_source_iter_next() assumes the context is locked. */ |
| 62 | LOCK_CONTEXT (context); |
| 63 | - g_source_iter_init (&iter, context, TRUE); |
| 64 | + |
| 65 | + /* First collect all remaining sources from the sources lists and store a |
| 66 | + * new reference in a separate list. Also set the context of the sources |
| 67 | + * to NULL so that they can't access a partially destroyed context anymore. |
| 68 | + * |
| 69 | + * We have to do this first so that we have a strong reference to all |
| 70 | + * sources and destroying them below does not also free them, and so that |
| 71 | + * none of the sources can access the context from their finalize/dispose |
| 72 | + * functions. */ |
| 73 | + g_source_iter_init (&iter, context, FALSE); |
| 74 | while (g_source_iter_next (&iter, &source)) |
| 75 | { |
| 76 | source->context = NULL; |
| 77 | + remaining_sources = g_slist_prepend (remaining_sources, g_source_ref (source)); |
| 78 | + } |
| 79 | + g_source_iter_clear (&iter); |
| 80 | + |
| 81 | + /* Next destroy all sources. As we still hold a reference to all of them, |
| 82 | + * this won't cause any of them to be freed yet and especially prevents any |
| 83 | + * source that unrefs another source from its finalize function to be freed. |
| 84 | + */ |
| 85 | + for (s_iter = remaining_sources; s_iter; s_iter = s_iter->next) |
| 86 | + { |
| 87 | + source = s_iter->data; |
| 88 | g_source_destroy_internal (source, context, TRUE); |
| 89 | } |
| 90 | UNLOCK_CONTEXT (context); |
| 91 | @@ -585,6 +606,18 @@ g_main_context_unref (GMainContext *context) |
| 92 | g_cond_clear (&context->cond); |
| 93 | |
| 94 | g_free (context); |
| 95 | + |
| 96 | + /* And now finally get rid of our references to the sources. This will cause |
| 97 | + * them to be freed unless something else still has a reference to them. Due |
| 98 | + * to setting the context pointers in the sources to NULL above, this won't |
| 99 | + * ever access the context or the internal linked list inside the GSource. |
| 100 | + * We already removed the sources completely from the context above. */ |
| 101 | + for (s_iter = remaining_sources; s_iter; s_iter = s_iter->next) |
| 102 | + { |
| 103 | + source = s_iter->data; |
| 104 | + g_source_unref_internal (source, NULL, FALSE); |
| 105 | + } |
| 106 | + g_slist_free (remaining_sources); |
| 107 | } |
| 108 | |
| 109 | /* Helper function used by mainloop/overflow test. |