[mutter/gnome-40] clutter/actor: First clear all stage views before emitting they changed
- From: Jonas Ã…dahl <jadahl src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [mutter/gnome-40] clutter/actor: First clear all stage views before emitting they changed
- Date: Wed, 17 Nov 2021 08:42:45 +0000 (UTC)
commit 09a078f4c13c9b4fbe8f260606eceedc09f6b08b
Author: Jonas Ã…dahl <jadahl gmail com>
Date: Tue Sep 28 11:26:25 2021 +0200
clutter/actor: First clear all stage views before emitting they changed
If one would end up with an actor attached to mapped actor, where the
attached actor doesn't itself have an up to date stage view list while
listening on the stage for updating, when clearing the stage views of
the list, anything that would query the stage views list at this time
would end up accessing freed memory.
This could happen if
1) An actor was added to a newly created container actor attached to
the stage
2) The actor got a timeline attached to it
3) The actor was moved to a container that already was mapped
4) A hotplug happened
After (1) both the container and actor would not have any stage views.
After (2) the timeline would listen on the stage for stage views
updates. After (3) the actor would still listen on the stage for stage
views updates. When (4) happened, the actor would be signalled when the
stage got its stage view cleared, at which point it would traverse up
its actor's tree finding an appropriate stage view to base its animation
on. The problem here would be that it'd query the already mapped
container and its yet-to-be-cleared stage view list, resulting in
use-after free, resulting in for example the following backtrace:
0) g_type_check_instance_cast ()
1) CLUTTER_STAGE_VIEW ()
2) clutter_actor_pick_frame_clock ()
3) clutter_actor_pick_frame_clock ()
4) update_frame_clock ()
5) on_frame_clock_actor_stage_views_changed ()
6) g_closure_invoke ()
7) signal_emit_unlocked_R ()
8) g_signal_emit_valist ()
9) g_signal_emit ()
10) clear_stage_views_cb ()
11) _clutter_actor_traverse_depth ()
12) _clutter_actor_traverse ()
13) clutter_actor_clear_stage_views_recursive ()
14) clutter_stage_clear_stage_views ()
...
Avoid this issue by making sure that we don't emit 'stage-views-changed'
signals while the actor tree is in an invalid state. While we now end up
traversing tree twice, it doesn't change the Big-O notation. It has not
been measured whether this has any noticible performance impact.
Closes: https://gitlab.gnome.org/GNOME/mutter/-/issues/1950
clutter/clutter/clutter-actor.c | 24 +++++++++++++++++--
src/tests/stage-view-tests.c | 53 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+), 2 deletions(-)
---
diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c
index 989e479a82..64cf291f8b 100644
--- a/clutter/clutter/clutter-actor.c
+++ b/clutter/clutter/clutter-actor.c
@@ -844,6 +844,7 @@ struct _ClutterActorPrivate
guint needs_paint_volume_update : 1;
guint had_effects_on_last_paint_volume_update : 1;
guint needs_update_stage_views : 1;
+ guint clear_stage_views_needs_stage_views_changed : 1;
};
enum
@@ -15656,7 +15657,21 @@ clear_stage_views_cb (ClutterActor *actor,
old_stage_views = g_steal_pointer (&actor->priv->stage_views);
if (old_stage_views)
- g_signal_emit (actor, actor_signals[STAGE_VIEWS_CHANGED], 0);
+ actor->priv->clear_stage_views_needs_stage_views_changed = TRUE;
+
+ return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE;
+}
+
+static ClutterActorTraverseVisitFlags
+maybe_emit_stage_views_changed_cb (ClutterActor *actor,
+ int depth,
+ gpointer user_data)
+{
+ if (actor->priv->clear_stage_views_needs_stage_views_changed)
+ {
+ actor->priv->clear_stage_views_needs_stage_views_changed = FALSE;
+ g_signal_emit (actor, actor_signals[STAGE_VIEWS_CHANGED], 0);
+ }
return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE;
}
@@ -15669,6 +15684,11 @@ clutter_actor_clear_stage_views_recursive (ClutterActor *self)
clear_stage_views_cb,
NULL,
NULL);
+ _clutter_actor_traverse (self,
+ CLUTTER_ACTOR_TRAVERSE_DEPTH_FIRST,
+ maybe_emit_stage_views_changed_cb,
+ NULL,
+ NULL);
}
float
@@ -15981,7 +16001,7 @@ clutter_actor_pick_frame_clock (ClutterActor *self,
for (l = stage_views_list; l; l = l->next)
{
- ClutterStageView *view = l->data;
+ ClutterStageView *view = CLUTTER_STAGE_VIEW (l->data);
float refresh_rate;
refresh_rate = clutter_stage_view_get_refresh_rate (view);
diff --git a/src/tests/stage-view-tests.c b/src/tests/stage-view-tests.c
index 70909a2b9a..344423d926 100644
--- a/src/tests/stage-view-tests.c
+++ b/src/tests/stage-view-tests.c
@@ -1157,6 +1157,57 @@ meta_test_timeline_actor_destroyed (void)
clutter_actor_destroy (persistent_actor);
}
+static void
+meta_test_timeline_actor_tree_clear (void)
+{
+ ClutterActor *stage;
+ ClutterActor *container1;
+ ClutterActor *container2;
+ g_autoptr (ClutterActor) floating = NULL;
+ g_autoptr (ClutterTimeline) timeline = NULL;
+ GList *stage_views;
+
+ stage = meta_backend_get_stage (meta_get_backend ());
+
+ ensure_view_count (1);
+
+ container1 = clutter_actor_new ();
+ clutter_actor_set_size (container1, 100, 100);
+ clutter_actor_add_child (stage, container1);
+
+ wait_for_paint (stage);
+
+ container2 = clutter_actor_new ();
+ clutter_actor_set_size (container2, 100, 100);
+ clutter_actor_add_child (stage, container2);
+
+ floating = g_object_ref_sink (clutter_actor_new ());
+ clutter_actor_set_size (floating, 100, 100);
+
+ clutter_actor_add_child (container2, floating);
+ timeline = clutter_timeline_new_for_actor (floating, 100);
+ clutter_actor_remove_child (container2, floating);
+
+ clutter_actor_add_child (container1, floating);
+
+ ensure_view_count (1);
+
+ is_on_stage_views (container1, 0);
+ is_on_stage_views (container2, 0);
+ is_on_stage_views (floating, 0);
+
+ wait_for_paint (stage);
+
+ stage_views = clutter_stage_peek_stage_views (CLUTTER_STAGE (stage));
+ is_on_stage_views (container1, 1, stage_views->data);
+ is_on_stage_views (container2, 1, stage_views->data);
+ is_on_stage_views (floating, 1, stage_views->data);
+
+ clutter_actor_destroy (floating);
+ clutter_actor_destroy (container1);
+ clutter_actor_destroy (container2);
+}
+
static void
init_tests (int argc, char **argv)
{
@@ -1186,6 +1237,8 @@ init_tests (int argc, char **argv)
meta_test_actor_stage_views_and_frame_clocks_freed);
g_test_add_func ("/stage-views/timeline/actor-destroyed",
meta_test_timeline_actor_destroyed);
+ g_test_add_func ("/stage-views/timeline/tree-clear",
+ meta_test_timeline_actor_tree_clear);
}
int
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]