Commit 4fd4b80a authored by Scott Violet's avatar Scott Violet Committed by Commit Bot

chromeos: fix possible uaf in AppsGridView

I'm working on a separate patch that seems to be tripping over this. The
specific sequence is:
. ItemRemoveAnimationDelegate is created for a view.
. We're in shutdown, meaning the widget is closing.
. ~AppsGridView is called
. ~AppsGridView calls RemoveAllChildViews(true).
. View::DoRemoveChildView() is called for the view that is animating.
. View::DoRemoveChildView() calls AppsGridView::ViewHierarchyChanged().
. This cancels the animation, deleting ~ItemRemoveAnimationDelegate.
. ~ItemRemoveAnimationDelegate is called, which deletes the view.
. View::DoRemoveChildView() continues on with a now deleted view.


The fix is to cancel the animations in ~AppsGridView before calling
RemoveAllChildViews(). Doing that means we aren't attempting to delete
a view while View is trying to clean it up.

As my other patch readily trips over this, I'm not adding a test.

BUG=none
TEST=none

Change-Id: I88f1640ed512d762f4f52efb6748317c048c64a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1605220Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658713}
parent 2709517d
...@@ -359,6 +359,12 @@ AppsGridView::~AppsGridView() { ...@@ -359,6 +359,12 @@ AppsGridView::~AppsGridView() {
if (item_list_) if (item_list_)
item_list_->RemoveObserver(this); item_list_->RemoveObserver(this);
// Cancel animations now, otherwise RemoveAllChildViews() may call back to
// ViewHierarchyChanged() during removal, which can lead to double deletes
// (because ViewHierarchyChanged() may attempt to delete a view that is part
// way through deletion).
bounds_animator_.Cancel();
view_model_.Clear(); view_model_.Clear();
RemoveAllChildViews(true); RemoveAllChildViews(true);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment