ozone/wayland: dnd: Fix UAF crashes when the entered window is destroyed

The case where the entered surface in a drag session (through
wl_data_device::enter event) is suddenly destroyed at client-side, was
not being properly handled, causing use-after-free crashes. This fixes
it as well as adds a unit test to prevent future similar regressions.

Bug: 1143707
Test: ozone_unittest --gtest_filter='*WaylandDataDragControllerTest.*'
Change-Id: I0fb0b7abbc257398c3a114f1c2d438ecc8d9adf5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2593572
Commit-Queue: Nick Yamane <nickdiego@igalia.com>
Reviewed-by: Maksim Sisov <msisov@igalia.com>
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#840688}

ui/ozone/platform/wayland/host/wayland_data_drag_controller.cc

@@ -64,6 +64,8 @@ WaylandDataDragController::WaylandDataDragController(
   DCHECK(window_manager_);
   DCHECK(data_device_manager_);
   DCHECK(data_device_);
+
+  window_manager_->AddObserver(this);
 }
 
 WaylandDataDragController::~WaylandDataDragController() = default;
@@ -183,16 +185,15 @@ void WaylandDataDragController::OnDragMotion(const gfx::PointF& location) {
 }
 
 void WaylandDataDragController::OnDragLeave() {
-  if (!window_)
-    return;
-
   if (state_ == State::kTransferring) {
     // We cannot leave until the transfer is finished.  Postponing.
     is_leave_pending_ = true;
     return;
   }
 
-  window_->OnDragLeave();
+  if (window_)
+    window_->OnDragLeave();
+
   window_ = nullptr;
   data_offer_.reset();
   is_leave_pending_ = false;
@@ -241,6 +242,11 @@ void WaylandDataDragController::OnDataSourceSend(const std::string& mime_type,
   }
 }
 
+void WaylandDataDragController::OnWindowRemoved(WaylandWindow* window) {
+  if (window == window_)
+    window_ = nullptr;
+}
+
 void WaylandDataDragController::Offer(const OSExchangeData& data,
                                       int operation) {
   DCHECK(data_source_);

ui/ozone/platform/wayland/host/wayland_data_drag_controller.h

@@ -15,6 +15,7 @@
 #include "ui/ozone/platform/wayland/common/wayland_object.h"
 #include "ui/ozone/platform/wayland/host/wayland_data_device.h"
 #include "ui/ozone/platform/wayland/host/wayland_data_source.h"
+#include "ui/ozone/platform/wayland/host/wayland_window_observer.h"
 
 struct wl_surface;
 class SkBitmap;
@@ -56,7 +57,8 @@ class WaylandShmBuffer;
 // event stops the transfer and cancels the operation; the window will not
 // receive anything at all.
 class WaylandDataDragController : public WaylandDataDevice::DragDelegate,
-                                  public WaylandDataSource::Delegate {
+                                  public WaylandDataSource::Delegate,
+                                  public WaylandWindowObserver {
  public:
   enum class State {
     kIdle,          // Doing nothing special
@@ -103,6 +105,9 @@ class WaylandDataDragController : public WaylandDataDevice::DragDelegate,
   void OnDataSourceSend(const std::string& mime_type,
                         std::string* contents) override;
 
+  // WaylandWindowObserver:
+  void OnWindowRemoved(WaylandWindow* window) override;
+
   void Offer(const OSExchangeData& data, int operation);
   void CreateIconSurfaceIfNeeded(const OSExchangeData& data);
   void HandleUnprocessedMimeTypes(base::TimeTicks start_time);

ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc

@@ -21,11 +21,13 @@
 #include "ui/events/base_event_utils.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/ozone/platform/wayland/common/data_util.h"
+#include "ui/ozone/platform/wayland/host/wayland_connection.h"
 #include "ui/ozone/platform/wayland/host/wayland_data_device.h"
 #include "ui/ozone/platform/wayland/host/wayland_data_device_manager.h"
 #include "ui/ozone/platform/wayland/host/wayland_data_drag_controller.h"
 #include "ui/ozone/platform/wayland/host/wayland_data_source.h"
 #include "ui/ozone/platform/wayland/host/wayland_toplevel_window.h"
+#include "ui/ozone/platform/wayland/host/wayland_window.h"
 #include "ui/ozone/platform/wayland/test/mock_surface.h"
 #include "ui/ozone/platform/wayland/test/test_data_device.h"
 #include "ui/ozone/platform/wayland/test/test_data_device_manager.h"
@@ -152,11 +154,48 @@ class WaylandDataDragControllerTest : public WaylandTest {
     return connection_->data_device_manager()->GetDevice();
   }
 
+  WaylandConnection* connection() { return connection_.get(); }
+
+  WaylandWindow* window() { return window_.get(); }
+
   base::string16 sample_text_for_dnd() const {
     static auto text = base::ASCIIToUTF16(kSampleTextForDragAndDrop);
     return text;
   }
 
+  uint32_t NextSerial() const {
+    static uint32_t serial = 0;
+    return ++serial;
+  }
+
+  // TODO(crbug.com/1163544): Deduplicate DnD test helper code.
+  void SendDndEnter(WaylandWindow* window, const gfx::Point& location) {
+    EXPECT_TRUE(data_device_manager_->data_source());
+
+    auto* surface = server_.GetObject<wl::MockSurface>(
+        window->root_surface()->GetSurfaceId());
+
+    // Emulate server sending an wl_data_device::offer event.
+    auto* data_offer = data_device_manager_->data_device()->OnDataOffer();
+    data_offer->OnOffer(
+        kMimeTypeText, ToClipboardData(std::string(kSampleTextForDragAndDrop)));
+
+    // Emulate server sending an wl_data_device::enter event.
+    data_device_manager_->data_device()->OnEnter(
+        NextSerial(), surface->resource(), wl_fixed_from_int(location.x()),
+        wl_fixed_from_int(location.y()), data_offer);
+  }
+
+  void SendDndLeave() {
+    EXPECT_TRUE(data_device_manager_->data_source());
+    data_device_manager_->data_device()->OnLeave();
+  }
+
+  void SendDndCancelled() {
+    EXPECT_TRUE(data_device_manager_->data_source());
+    data_device_manager_->data_source()->OnCancelled();
+  }
+
   void ReadDataWhenSourceIsReady() {
     Sync();
 
@@ -190,21 +229,33 @@ class WaylandDataDragControllerTest : public WaylandTest {
   }
 
   void ScheduleDragCancel() {
+    ScheduleTestTask(base::BindOnce(
+        [](WaylandDataDragControllerTest* self) {
+          self->SendDndCancelled();
+          self->Sync();
+        },
+        base::Unretained(this)));
+  }
+
+  void ScheduleTestTask(base::OnceClosure test_task) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&WaylandDataDragControllerTest::RunTestTask,
+                       base::Unretained(this), std::move(test_task)));
+  }
+
+  void RunTestTask(base::OnceClosure test_task) {
     Sync();
 
+    // The data source is created asynchronously by the data drag controller. If
+    // it is null at this point, it means that the task for that has not yet
+    // executed, and we have to try again a bit later.
     if (!data_device_manager_->data_source()) {
-      // The data source is created asynchronously by the data drag controller.
-      // If it is null at this point, it means that the task for that has not
-      // yet executed, and we have to try again a bit later.
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE,
-          base::BindOnce(&WaylandDataDragControllerTest::ScheduleDragCancel,
-                         base::Unretained(this)));
+      ScheduleTestTask(std::move(test_task));
       return;
     }
 
-    data_device_manager_->data_source()->OnCancelled();
-    Sync();
+    std::move(test_task).Run();
   }
 
  protected:
@@ -542,6 +593,57 @@ TEST_P(WaylandDataDragControllerTest, ForeignDragHandleAskAction) {
   data_device_manager_->data_device()->OnLeave();
 }
 
+// Regression test for https://crbug.com/1143707.
+TEST_P(WaylandDataDragControllerTest, DestroyEnteredSurface) {
+  auto* window_1 = static_cast<WaylandToplevelWindow*>(window_.get());
+  const bool restored_focus = window_1->has_pointer_focus();
+  window_1->SetPointerFocus(true);
+  ASSERT_EQ(PlatformWindowType::kWindow, window_1->type());
+
+  auto test = [](WaylandDataDragControllerTest* self) {
+    // Emulate server sending an dnd offer + enter events for |window_1|.
+    self->SendDndEnter(self->window(), gfx::Point(10, 10));
+
+    // Init and open |target_window|.
+    PlatformWindowInitProperties properties{gfx::Rect{80, 80}};
+    properties.type = PlatformWindowType::kWindow;
+    MockPlatformWindowDelegate delegate_2;
+    EXPECT_CALL(delegate_2, OnAcceleratedWidgetAvailable(_)).Times(1);
+    auto window_2 = WaylandWindow::Create(&delegate_2, self->connection(),
+                                          std::move(properties));
+    ASSERT_NE(gfx::kNullAcceleratedWidget, window_2->GetWidget());
+    self->Sync();
+
+    // Leave |window_1| and enter |window_2|.
+    self->SendDndLeave();
+    self->SendDndEnter(window_2.get(), gfx::Point(20, 20));
+    self->Sync();
+
+    // Destroy the entered window at client side and emulates a
+    // wl_data_device::leave to ensure no UAF happens.
+    window_2.reset();
+    self->SendDndLeave();
+    self->Sync();
+
+    // Emulate server sending an wl_data_source::cancelled event so the drag
+    // loop is finished.
+    self->SendDndCancelled();
+    self->Sync();
+  };
+
+  // Post test task to be performed asynchronously once the drag session gets
+  // started.
+  ScheduleTestTask(base::BindOnce(test, base::Unretained(this)));
+
+  // Request to start the drag session, which spins a nested run loop.
+  OSExchangeData os_exchange_data;
+  os_exchange_data.SetString(sample_text_for_dnd());
+  window_1->StartDrag(os_exchange_data, DragDropTypes::DRAG_COPY, {}, true,
+                      drag_handler_delegate_.get());
+  Sync();
+  window_1->SetPointerFocus(restored_focus);
+}
+
 INSTANTIATE_TEST_SUITE_P(XdgVersionStableTest,
                          WaylandDataDragControllerTest,
                          ::testing::Values(kXdgShellStable));