From 802d4e0cf87b739df3f6c6c6080690606b91399a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20=C3=85dahl?= Date: Tue, 28 Sep 2021 11:26:25 +0200 Subject: [PATCH] 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 Part-of: --- 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 6cbec0b70..2c69f0ad6 100644 --- a/clutter/clutter/clutter-actor.c +++ b/clutter/clutter/clutter-actor.c @@ -843,6 +843,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 @@ -15708,7 +15709,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; } @@ -15721,6 +15736,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 @@ -16033,7 +16053,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 abb2cebb0..198b6cd49 100644 --- a/src/tests/stage-view-tests.c +++ b/src/tests/stage-view-tests.c @@ -1255,6 +1255,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_context_get_backend (test_context)); + + 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 (void) { @@ -1286,6 +1337,8 @@ init_tests (void) meta_test_actor_stage_views_queue_frame_drawn); 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