exo: extended-drag: Fix a UAF issue in session cancellation handling

Chrome crashes when the tab drag session is cancelled (ie: pressing esc)
in attached mode. The crash happens because a use-after-free in
ExtendedDragSource's destructor, when attempting to notify registered
observers about its destruction.

More specifically, the issue happens because DragDropOperation instance
doesn't remove itself from the extended drag source's observers list
when it's destroyed in response to a session cancellation. This patch
fixes it and, additionally, cleans up how DragDropOperation access to
the extended drag source instance is done, by associating it with its
seat instead of the data source, simplifying code handling (ext-)source
destruction, etc.

R=oshima@chromium.org

Bug: 1099418
Change-Id: I265379fb0aed0742674d3dcd97815b1cffdc709f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2514360
Commit-Queue: Nick Yamane <nickdiego@igalia.com>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824060}

components/exo/data_source.h

@@ -17,7 +17,6 @@ namespace exo {
 
 class DataSourceDelegate;
 class DataSourceObserver;
-class ExtendedDragSource;
 enum class DndAction;
 
 // Object representing transferred data offered by a client.
@@ -82,11 +81,6 @@ class DataSource {
 
   bool CanBeDataSourceForCopy(Surface* surface) const;
 
-  ExtendedDragSource* extended_drag_source() { return extended_drag_source_; }
-  void set_extended_drag_source(ExtendedDragSource* extended_drag_source) {
-    extended_drag_source_ = extended_drag_source;
-  }
-
  private:
   // Reads data from the source. Then |callback| is invoked with read data. If
   // Cancelled() is invoked or DataSource is destroyed before completion,
@@ -113,8 +107,6 @@ class DataSource {
 
   base::flat_set<DndAction> dnd_actions_;
 
-  ExtendedDragSource* extended_drag_source_ = nullptr;
-
   base::WeakPtrFactory<DataSource> read_data_weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(DataSource);

components/exo/drag_drop_operation.cc

@@ -85,9 +85,10 @@ base::WeakPtr<DragDropOperation> DragDropOperation::Create(
     Surface* origin,
     Surface* icon,
     const gfx::PointF& drag_start_point,
-    ui::mojom::DragEventSource event_source) {
+    ui::mojom::DragEventSource event_source,
+    ExtendedDragSource* extended_drag_source) {
   auto* dnd_op = new DragDropOperation(source, origin, icon, drag_start_point,
-                                       event_source);
+                                       event_source, extended_drag_source);
   return dnd_op->weak_ptr_factory_.GetWeakPtr();
 }
 
@@ -95,14 +96,15 @@ DragDropOperation::DragDropOperation(DataSource* source,
                                      Surface* origin,
                                      Surface* icon,
                                      const gfx::PointF& drag_start_point,
-                                     ui::mojom::DragEventSource event_source)
+                                     ui::mojom::DragEventSource event_source,
+                                     ExtendedDragSource* extended_drag_source)
     : SurfaceTreeHost("ExoDragDropOperation"),
       source_(std::make_unique<ScopedDataSource>(source, this)),
       origin_(std::make_unique<ScopedSurface>(origin, this)),
       drag_start_point_(drag_start_point),
       os_exchange_data_(std::make_unique<ui::OSExchangeData>()),
       event_source_(event_source),
-      weak_ptr_factory_(this) {
+      extended_drag_source_(extended_drag_source) {
   aura::Window* root_window = origin_->get()->window()->GetRootWindow();
   DCHECK(root_window);
 #if defined(OS_CHROMEOS)
@@ -118,11 +120,12 @@ DragDropOperation::DragDropOperation(DataSource* source,
 
   drag_drop_controller_->AddObserver(this);
 
-  if (auto* ext_drag_source = source_->get()->extended_drag_source()) {
+  if (extended_drag_source_) {
 #if defined(OS_CHROMEOS)
-    drag_drop_controller_->set_toplevel_window_drag_delegate(ext_drag_source);
+    drag_drop_controller_->set_toplevel_window_drag_delegate(
+        extended_drag_source_);
 #endif
-    ext_drag_source->AddObserver(this);
+    extended_drag_source_->AddObserver(this);
   }
 
   if (icon)
@@ -159,6 +162,11 @@ DragDropOperation::~DragDropOperation() {
 
   if (drag_drop_controller_->IsDragDropInProgress() && started_by_this_object_)
     drag_drop_controller_->DragCancel();
+
+  if (extended_drag_source_) {
+    extended_drag_source_->RemoveObserver(this);
+    extended_drag_source_ = nullptr;
+  }
 }
 
 void DragDropOperation::AbortIfPending() {
@@ -281,20 +289,13 @@ void DragDropOperation::StartDragDropOperation() {
     source_->get()->DndFinished();
 
     // Reset |source_| so it the destructor doesn't try to cancel it.
-    ResetSource();
+    source_.reset();
   }
 
   // On failure the destructor will handle canceling the data source.
   delete this;
 }
 
-void DragDropOperation::ResetSource() {
-  DCHECK(source_);
-  if (source_->get()->extended_drag_source())
-    source_->get()->extended_drag_source()->RemoveObserver(this);
-  source_.reset();
-}
-
 void DragDropOperation::OnDragStarted() {
   if (!started_by_this_object_)
     delete this;
@@ -323,13 +324,12 @@ void DragDropOperation::OnDragActionsChanged(int actions) {
 
 void DragDropOperation::OnExtendedDragSourceDestroying(
     ExtendedDragSource* source) {
+  DCHECK(extended_drag_source_);
+  extended_drag_source_->RemoveObserver(this);
 #if defined(OS_CHROMEOS)
   drag_drop_controller_->set_toplevel_window_drag_delegate(nullptr);
 #endif
-  if (source_) {
-    DCHECK(source_->get()->extended_drag_source());
-    source_->get()->extended_drag_source()->RemoveObserver(this);
-  }
+  extended_drag_source_ = nullptr;
 }
 
 void DragDropOperation::OnSurfaceDestroying(Surface* surface) {
@@ -339,7 +339,8 @@ void DragDropOperation::OnSurfaceDestroying(Surface* surface) {
 
 void DragDropOperation::OnDataSourceDestroying(DataSource* source) {
   DCHECK_EQ(source, source_->get());
-  ResetSource();
+  source_.reset();
   delete this;
 }
+
 }  // namespace exo

components/exo/drag_drop_operation.h

@@ -35,6 +35,7 @@ class CopyOutputResult;
 }
 
 namespace exo {
+class ExtendedDragSource;
 class ScopedDataSource;
 
 // This class represents an ongoing drag-drop operation started by an exo
@@ -54,7 +55,8 @@ class DragDropOperation : public DataSourceObserver,
       Surface* origin,
       Surface* icon,
       const gfx::PointF& drag_start_point,
-      ui::mojom::DragEventSource event_source);
+      ui::mojom::DragEventSource event_source,
+      ExtendedDragSource* extended_drag_source);
 
   // Abort the operation if it hasn't been started yet, otherwise do nothing.
   void AbortIfPending();
@@ -85,7 +87,8 @@ class DragDropOperation : public DataSourceObserver,
                     Surface* origin,
                     Surface* icon,
                     const gfx::PointF& drag_start_point,
-                    ui::mojom::DragEventSource event_source);
+                    ui::mojom::DragEventSource event_source,
+                    ExtendedDragSource* extended_drag_source);
   ~DragDropOperation() override;
 
   void CaptureDragIcon();
@@ -129,7 +132,9 @@ class DragDropOperation : public DataSourceObserver,
 
   ui::mojom::DragEventSource event_source_;
 
-  base::WeakPtrFactory<DragDropOperation> weak_ptr_factory_;
+  ExtendedDragSource* extended_drag_source_;
+
+  base::WeakPtrFactory<DragDropOperation> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(DragDropOperation);
 };

components/exo/drag_drop_operation_unittest.cc

@@ -105,7 +105,7 @@ TEST_F(DragDropOperationTest, DeleteDuringDragging) {
 
   auto operation = DragDropOperation::Create(
       data_source.get(), origin_surface.get(), icon_surface.get(),
-      gfx::PointF(), ui::mojom::DragEventSource::kMouse);
+      gfx::PointF(), ui::mojom::DragEventSource::kMouse, nullptr);
   icon_surface->Commit();
 
   base::RunLoop run_loop;

components/exo/extended_drag_source.cc

@@ -14,6 +14,7 @@
 #include "base/notreached.h"
 #include "base/optional.h"
 #include "components/exo/data_source.h"
+#include "components/exo/seat.h"
 #include "components/exo/surface.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window_observer.h"
@@ -96,13 +97,15 @@ class ExtendedDragSource::DraggedWindowHolder : public aura::WindowObserver {
   aura::Window* toplevel_window_ = nullptr;
 };
 
-ExtendedDragSource::ExtendedDragSource(DataSource* source, Delegate* delegate)
-    : delegate_(delegate), source_(source) {
+ExtendedDragSource::ExtendedDragSource(Seat* seat,
+                                       DataSource* source,
+                                       Delegate* delegate)
+    : seat_(seat), source_(source), delegate_(delegate) {
+  DCHECK(seat_);
   DCHECK(source_);
   DCHECK(delegate_);
-  DVLOG(1) << "ExtendedDragSource created. wl_source=" << source_;
 
-  source_->set_extended_drag_source(this);
+  seat_->set_extended_drag_source(this);
   source_->AddObserver(this);
 }
 
@@ -111,10 +114,9 @@ ExtendedDragSource::~ExtendedDragSource() {
   for (auto& observer : observers_)
     observer.OnExtendedDragSourceDestroying(this);
 
-  if (source_) {
-    source_->set_extended_drag_source(nullptr);
+  seat_->set_extended_drag_source(nullptr);
+  if (source_)
     source_->RemoveObserver(this);
-  }
 }
 
 void ExtendedDragSource::AddObserver(Observer* observer) {

components/exo/extended_drag_source.h

@@ -33,6 +33,7 @@ class LocatedEvent;
 namespace exo {
 
 class DataSource;
+class Seat;
 class Surface;
 
 class ExtendedDragSource : public DataSourceObserver,
@@ -59,7 +60,7 @@ class ExtendedDragSource : public DataSourceObserver,
     virtual ~Observer() = default;
   };
 
-  ExtendedDragSource(DataSource* source, Delegate* delegate);
+  ExtendedDragSource(Seat* seat, DataSource* source, Delegate* delegate);
   ExtendedDragSource(const ExtendedDragSource&) = delete;
   ExtendedDragSource& operator=(const ExtendedDragSource&) = delete;
   ~ExtendedDragSource() override;
@@ -95,11 +96,13 @@ class ExtendedDragSource : public DataSourceObserver,
   gfx::Point CalculateOrigin(aura::Window* target) const;
   void Cleanup();
 
+  Seat* const seat_;
+  DataSource* source_ = nullptr;
+
   // Created and destroyed at wayland/zcr_extended_drag.cc and its lifetime is
   // tied to the zcr_extended_drag_source_v1 object it's attached to.
   Delegate* const delegate_;
 
-  DataSource* source_ = nullptr;
   gfx::PointF pointer_location_;
   ui::mojom::DragEventSource drag_event_source_;
   bool cursor_locked_ = false;

components/exo/extended_drag_source_unittest.cc

@@ -112,17 +112,20 @@ class ExtendedDragSourceTest : public test::ExoTestBase {
     drag_drop_controller_->set_should_block_during_drag_drop(false);
     drag_drop_controller_->set_enabled(true);
 
+    seat_ = std::make_unique<Seat>();
     data_source_ = std::make_unique<DataSource>(new TestDataSourceDelegate);
-    extended_drag_source_ = std::make_unique<ExtendedDragSource>(
-        data_source_.get(), new TestExtendedDragSourceDelegate(
-                                /*allow_drop_no_target=*/true,
-                                /*lock_cursor=*/true));
+    extended_drag_source_ =
+        std::make_unique<ExtendedDragSource>(seat_.get(), data_source_.get(),
+                                             new TestExtendedDragSourceDelegate(
+                                                 /*allow_drop_no_target=*/true,
+                                                 /*lock_cursor=*/true));
   }
 
   void TearDown() override {
-    test::ExoTestBase::TearDown();
     extended_drag_source_.reset();
     data_source_.reset();
+    seat_.reset();
+    test::ExoTestBase::TearDown();
   }
 
  protected:
@@ -145,6 +148,7 @@ class ExtendedDragSourceTest : public test::ExoTestBase {
   }
 
   ash::DragDropController* drag_drop_controller_ = nullptr;
+  std::unique_ptr<Seat> seat_;
   std::unique_ptr<DataSource> data_source_;
   std::unique_ptr<ExtendedDragSource> extended_drag_source_;
 };
@@ -152,20 +156,19 @@ class ExtendedDragSourceTest : public test::ExoTestBase {
 }  // namespace
 
 TEST_F(ExtendedDragSourceTest, DestroySource) {
-  Seat seat;
   Surface origin;
 
   // Give |origin| a root window and start DragDropOperation.
   GetContext()->AddChild(origin.window());
-  seat.StartDrag(data_source_.get(), &origin, /*icon=*/nullptr,
-                 ui::mojom::DragEventSource::kMouse);
+  seat_->StartDrag(data_source_.get(), &origin, /*icon=*/nullptr,
+                   ui::mojom::DragEventSource::kMouse);
 
   // Ensure that destroying the data source invalidates its extended_drag_source
   // counterpart for the rest of its lifetime.
-  EXPECT_TRUE(seat.get_drag_drop_operation_for_testing());
+  EXPECT_TRUE(seat_->get_drag_drop_operation_for_testing());
   EXPECT_TRUE(extended_drag_source_->IsActive());
   data_source_.reset();
-  EXPECT_FALSE(seat.get_drag_drop_operation_for_testing());
+  EXPECT_FALSE(seat_->get_drag_drop_operation_for_testing());
   EXPECT_FALSE(extended_drag_source_->IsActive());
 }
 

components/exo/seat.cc

@@ -20,6 +20,7 @@
 #include "base/task/post_task.h"
 #include "components/exo/data_source.h"
 #include "components/exo/drag_drop_operation.h"
+#include "components/exo/extended_drag_source.h"
 #include "components/exo/mime_utils.h"
 #include "components/exo/seat_observer.h"
 #include "components/exo/shell_surface_util.h"
@@ -110,8 +111,9 @@ void Seat::StartDrag(DataSource* source,
                      Surface* icon,
                      ui::mojom::DragEventSource event_source) {
   // DragDropOperation manages its own lifetime.
-  drag_drop_operation_ = DragDropOperation::Create(
-      source, origin, icon, last_pointer_location_, event_source);
+  drag_drop_operation_ =
+      DragDropOperation::Create(source, origin, icon, last_pointer_location_,
+                                event_source, extended_drag_source_);
 }
 
 void Seat::SetLastPointerLocation(const gfx::PointF& last_pointer_location) {

components/exo/seat.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_EXO_SEAT_H_
 #define COMPONENTS_EXO_SEAT_H_
 
+#include "base/check.h"
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
@@ -30,6 +31,7 @@ class KeyEvent;
 
 namespace exo {
 class DragDropOperation;
+class ExtendedDragSource;
 class ScopedDataSource;
 class SeatObserver;
 class Surface;
@@ -114,6 +116,11 @@ class Seat : public aura::client::FocusChangeObserver,
   void OnKeyboardLayoutNameChanged(const std::string& layout_name) override;
 #endif
 
+  void set_extended_drag_source(ExtendedDragSource* extended_drag_source) {
+    DCHECK(!extended_drag_source || !extended_drag_source_);
+    extended_drag_source_ = extended_drag_source;
+  }
+
   void set_physical_code_for_currently_processing_event_for_testing(
       ui::DomCode physical_code_for_currently_processing_event) {
     physical_code_for_currently_processing_event_ =
@@ -179,6 +186,8 @@ class Seat : public aura::client::FocusChangeObserver,
   std::unique_ptr<XkbTracker> xkb_tracker_;
 #endif  // defined(OS_CHROMEOS)
 
+  ExtendedDragSource* extended_drag_source_ = nullptr;
+
   base::WeakPtrFactory<Seat> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(Seat);

components/exo/wayland/zcr_extended_drag.cc

@@ -16,6 +16,7 @@
 #include "components/exo/display.h"
 #include "components/exo/extended_drag_offer.h"
 #include "components/exo/extended_drag_source.h"
+#include "components/exo/seat.h"
 #include "components/exo/surface.h"
 #include "components/exo/wayland/server_util.h"
 #include "ui/gfx/geometry/vector2d.h"
@@ -143,6 +144,7 @@ void extended_drag_get_extended_drag_source(wl_client* client,
                                             uint32_t id,
                                             wl_resource* data_source_resource,
                                             uint32_t settings) {
+  Display* display = GetUserDataAs<Display>(resource);
   DataSource* source = GetUserDataAs<DataSource>(data_source_resource);
 
   wl_resource* extended_drag_source_resource =
@@ -152,8 +154,9 @@ void extended_drag_get_extended_drag_source(wl_client* client,
   SetImplementation(extended_drag_source_resource,
                     &extended_drag_source_implementation,
                     std::make_unique<ExtendedDragSource>(
-                        source, new ZcrExtendedDragSourceDelegate(
-                                    extended_drag_source_resource, settings)));
+                        display->seat(), source,
+                        new ZcrExtendedDragSourceDelegate(
+                            extended_drag_source_resource, settings)));
 }
 
 void extended_drag_get_extended_drag_offer(wl_client* client,