[go: up one dir, main page]

[Merge to M-96] capture mode: Fix UAF when a dimmed widget is closed

When a widget gets activated on top of the recorded window,
it gets dimmed. When the widget is closed, it loses activation
but is not removed from the window hierarchy until its native
widget is actually deleted when the animation finishes.

When its window dimmer is destroyed in OnWindowDestroying(),
we must be careful not to access any of its members so as not
to trigger a use-after-free.

(cherry picked from commit 84d60042e8b182b055e91645ec94e7c7a08a2f93)

Fixed: 1273197
Test: Manually, added a new test that triggers the ASAN failure.
Change-Id: I7ae89c5ec9b4790522b3d58623f2dab222cbdd75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3308201
Reviewed-by: Min Chen <minch@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#946566}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3312743
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Ahmed Fakhry <afakhry@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1205}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
diff --git a/ash/capture_mode/capture_mode_unittests.cc b/ash/capture_mode/capture_mode_unittests.cc
index f1177b34..3c358ee5 100644
--- a/ash/capture_mode/capture_mode_unittests.cc
+++ b/ash/capture_mode/capture_mode_unittests.cc
@@ -43,6 +43,7 @@
 #include "ash/shell.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/test_widget_builder.h"
 #include "ash/wm/cursor_manager_chromeos.h"
 #include "ash/wm/desks/desk.h"
 #include "ash/wm/desks/desks_controller.h"
@@ -1940,6 +1941,25 @@
   EXPECT_FALSE(window->subtree_capture_id().is_valid());
 }
 
+TEST_F(CaptureModeTest, ClosingDimmedWidgetAboveRecordedWindow) {
+  views::Widget* widget = TestWidgetBuilder().BuildOwnedByNativeWidget();
+  auto* window = widget->GetNativeWindow();
+  auto recorded_window = CreateTestWindow(gfx::Rect(200, 200));
+
+  auto* controller = StartSessionAndRecordWindow(recorded_window.get());
+  EXPECT_TRUE(controller->is_recording_in_progress());
+  auto* recording_watcher = controller->video_recording_watcher_for_testing();
+
+  // Activate the window so that it becomes on top of the recorded window, and
+  // expect it gets dimmed.
+  wm::ActivateWindow(window);
+  EXPECT_TRUE(recording_watcher->IsWindowDimmedForTesting(window));
+
+  // Close the widget, this should not lead to any use-after-free. See
+  // https://crbug.com/1273197.
+  widget->Close();
+}
+
 TEST_F(CaptureModeTest, DimmingOfUnRecordedWindows) {
   auto win1 = CreateTestWindow(gfx::Rect(200, 200));
   auto win2 = CreateTestWindow(gfx::Rect(200, 200));
diff --git a/ash/wm/window_dimmer.cc b/ash/wm/window_dimmer.cc
index 7f7fb57f..a49019d 100644
--- a/ash/wm/window_dimmer.cc
+++ b/ash/wm/window_dimmer.cc
@@ -84,9 +84,11 @@
 void WindowDimmer::OnWindowDestroying(aura::Window* window) {
   if (window == parent_) {
     parent_->RemoveObserver(this);
-    if (delegate_)
-      delegate_->OnDimmedWindowDestroying(window);
     parent_ = nullptr;
+    if (delegate_) {
+      delegate_->OnDimmedWindowDestroying(window);
+      // `this` can be deleted above. So don't access any member after this.
+    }
   } else {
     DCHECK_EQ(window_, window);
     window_->RemoveObserver(this);