Merge branch 'main-context-freeing-of-sources' into 'master'

GMainContext - Fix memory leaks and memory corruption when freeing sources while freeing a context

See merge request GNOME/glib!1353

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,13 +559,32 @@ 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);
 
   for (sl_iter = context->source_lists; sl_iter; sl_iter = sl_iter->next)
     {
@@ -575,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);
@@ -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.
@@ -1004,13 +1037,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;

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);