Skip to content

Commit

Permalink
[asan] Properly clean up state in aura::Window::CleanupGestureState.
Browse files Browse the repository at this point in the history
A guard was added in commit 5a2a306
that exited from CleanupGestureState when the window was destroyed from
the gesture cancellation. However it exited too early, before the
consumer state was cleaned up. This causes later drag and drop gestures
to trigger use-after-free errors.

This change moves the exit condition until after all related states in
the gesture recognizer is cleaned up.

#asan #uaf

Bug: 1297643
Change-Id: I31c82ab3723c379515579cc32d8bed2ec93ec955
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3514815
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Addison Luh <aluh@chromium.org>
Auto-Submit: Addison Luh <aluh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982965}
  • Loading branch information
Addison Luh authored and Chromium LUCI CQ committed Mar 18, 2022
1 parent e730e1e commit 9cab337
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 1 deletion.
2 changes: 1 addition & 1 deletion ui/aura/window.cc
Expand Up @@ -1258,9 +1258,9 @@ bool Window::CleanupGestureState() {
bool state_modified = false;
Env* env = Env::GetInstance();
state_modified |= env->gesture_recognizer()->CancelActiveTouches(this);
state_modified |= env->gesture_recognizer()->CleanupStateForConsumer(this);
if (!tracking_this.Contains(this))
return state_modified;
state_modified |= env->gesture_recognizer()->CleanupStateForConsumer(this);
// Potentially event handlers for CancelActiveTouches() within
// CleanupGestureState may change the window hierarchy (or reorder the
// |children_|), and therefore iterating over |children_| is not safe. Use
Expand Down
66 changes: 66 additions & 0 deletions ui/aura/window_unittest.cc
Expand Up @@ -3646,6 +3646,72 @@ TEST_F(WindowTest, CleanupGestureStateDeleteOtherWindows) {
child2.reset();
}

class TestTouchWindowDelegate : public TestWindowDelegate {
public:
explicit TestTouchWindowDelegate() = default;
TestTouchWindowDelegate(const TestTouchWindowDelegate&) = delete;
TestTouchWindowDelegate& operator=(const TestTouchWindowDelegate&) = delete;
~TestTouchWindowDelegate() override = default;

void OnTouchEvent(ui::TouchEvent* event) override {
if (event->type() == ui::ET_TOUCH_CANCELLED) {
if (on_touch_cancel_callback_) {
std::move(on_touch_cancel_callback_)
.Run(static_cast<Window*>(event->target()));
}
}
}

void SetOnTouchCancelCallback(base::OnceCallback<void(Window*)> callback) {
on_touch_cancel_callback_ = std::move(callback);
}

private:
base::OnceCallback<void(Window*)> on_touch_cancel_callback_;
};

// Test for use-after-free in crbug.com/1297643.
TEST_F(WindowTest, DeleteWindowWhenCancellingTouch) {
TestTouchWindowDelegate window_delegate;
auto window = std::make_unique<Window>(&window_delegate);
window->Init(ui::LAYER_NOT_DRAWN);
root_window()->AddChild(window.get());
window->SetBounds(gfx::Rect(0, 0, 200, 200));
window->Show();

window_delegate.SetOnTouchCancelCallback(
base::BindLambdaForTesting([&window](Window* target) {
if (target == window.get()) {
window.reset();
}
}));

ui::test::EventGenerator event_generator(root_window(), window.get());
event_generator.PressTouch();
event_generator.MoveTouchBy(10, 10);

auto* gesture_recognizer = Env::GetInstance()->gesture_recognizer();
EXPECT_TRUE(gesture_recognizer->DoesConsumerHaveActiveTouch(window.get()));

// Hide window which should cancel touch.
window->Hide();
// Window should've been deleted.
EXPECT_FALSE(window);

// Create two new windows to transfer between.
Window window1(nullptr);
window1.Init(ui::LAYER_NOT_DRAWN);
root_window()->AddChild(&window1);

Window window2(nullptr);
window2.Init(ui::LAYER_NOT_DRAWN);
root_window()->AddChild(&window2);

// This should not cause use-after-free.
gesture_recognizer->TransferEventsTo(
&window1, &window2, ui::TransferTouchesBehavior::kDontCancel);
}

class WindowActualScreenBoundsTest
: public WindowTest,
public testing::WithParamInterface<
Expand Down

0 comments on commit 9cab337

Please sign in to comment.