From e3ce9901de36bda58a50b1c5005dc5100b8d0991 Mon Sep 17 00:00:00 2001
From: Tom Anderson <thomasanderson@chromium.org>
Date: Tue, 26 May 2026 17:46:19 -0700
Subject: [PATCH] ozone/x11: Fix UAF inside X11Window::GetBoundsInPixels()

When GetBoundsInPixels() triggers synchronous nested message loops
within GeometryCache, the calling X11Window instance can be
synchronously destroyed. Subsequent operations in X11Window continue
executing on the freed this context, leading to potential memory
corruption in the browser process.

This CL resolves the issue by:
1. Making weak_ptr_factory_ mutable in X11Window.
2. Guarding GetBoundsInPixels() calls in all critical X11Window methods
   using base::WeakPtrFactory to prevent executing subsequent code if
   this has been destroyed.
3. Adding a regression/POC unit test verifying the safety of
   SetBoundsInPixels under re-entrancy deletion conditions.

Fixed: 516653777
Change-Id: I812afc95f56e77770cda51ad783e206ba0f1b4c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7875980
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Auto-Submit: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1636605}
---

diff --git a/ui/ozone/platform/x11/x11_window.cc b/ui/ozone/platform/x11/x11_window.cc
index eddc5aee..2459a05 100644
--- a/ui/ozone/platform/x11/x11_window.cc
+++ b/ui/ozone/platform/x11/x11_window.cc
@@ -540,10 +540,16 @@
   gfx::Rect new_bounds_in_pixels(bounds.origin(),
                                  AdjustSizeForDisplay(bounds.size()));
 
+  auto weak_this = weak_ptr_factory_.GetWeakPtr();
+  const gfx::Rect current_bounds = GetBoundsInPixels();
+  if (!weak_this) {
+    return;
+  }
+
   const bool size_changed =
-      GetBoundsInPixels().size() != new_bounds_in_pixels.size();
+      current_bounds.size() != new_bounds_in_pixels.size();
   const bool origin_changed =
-      GetBoundsInPixels().origin() != new_bounds_in_pixels.origin();
+      current_bounds.origin() != new_bounds_in_pixels.origin();
 
   // Assume that the resize will go through as requested, which should be the
   // case if we're running without a window manager.  If there's a window
@@ -601,6 +607,10 @@
   platform_window_delegate_->OnBoundsChanged({origin_changed});
 }
 
+// Note: geometry_cache_->GetBoundsPx() can dispatch pending X server events
+// synchronously, which can trigger window destruction and invalidate `this`.
+// Callers should retrieve a WeakPtr before calling GetBoundsInPixels() and
+// check its validity afterward if they access any member of `this`.
 gfx::Rect X11Window::GetBoundsInPixels() const {
   return bounds_wm_sync_ || !geometry_cache_ ? last_set_bounds_px_
                                              : geometry_cache_->GetBoundsPx();
@@ -612,7 +622,12 @@
 }
 
 gfx::Rect X11Window::GetBoundsInDIP() const {
-  return platform_window_delegate_->ConvertRectToDIP(GetBoundsInPixels());
+  auto weak_this = weak_ptr_factory_.GetWeakPtr();
+  const gfx::Rect bounds = GetBoundsInPixels();
+  if (!weak_this) {
+    return {};
+  }
+  return platform_window_delegate_->ConvertRectToDIP(bounds);
 }
 
 void X11Window::SetTitle(const std::u16string& title) {
@@ -702,7 +717,11 @@
   // - works around Flash content which expects to have the size updated
   //   synchronously.
   // See https://crbug.com/361408
+  auto weak_this = weak_ptr_factory_.GetWeakPtr();
   gfx::Rect new_bounds_px = GetBoundsInPixels();
+  if (!weak_this) {
+    return;
+  }
   if (fullscreen) {
     restored_bounds_in_pixels_ = new_bounds_px;
     if (x11_extension_delegate_) {
@@ -725,7 +744,11 @@
 
   // Pretend the bounds changed immediately, and wait for a WM sync to use the
   // server's bounds.
-  bool origin_changed = GetBoundsInPixels().origin() != new_bounds_px.origin();
+  const gfx::Rect current_bounds = GetBoundsInPixels();
+  if (!weak_this) {
+    return;
+  }
+  bool origin_changed = current_bounds.origin() != new_bounds_px.origin();
   SetBoundsWithWmSync(new_bounds_px);
 
   // This must be the final call in this function, as `this` may be deleted
@@ -734,26 +757,40 @@
 }
 
 void X11Window::Maximize() {
+  auto weak_this = weak_ptr_factory_.GetWeakPtr();
   if (IsFullscreen()) {
     // Unfullscreen the window if it is fullscreen.
     SetFullscreen(false, display::kInvalidDisplayId);
+    if (!weak_this) {
+      return;
+    }
 
     // Resize the window so that it does not have the same size as a monitor.
     // (Otherwise, some window managers immediately put the window back in
     // fullscreen mode).
     gfx::Rect bounds_in_pixels = GetBoundsInPixels();
+    if (!weak_this) {
+      return;
+    }
     gfx::Rect adjusted_bounds_in_pixels(
         bounds_in_pixels.origin(),
         AdjustSizeForDisplay(bounds_in_pixels.size()));
     if (adjusted_bounds_in_pixels != bounds_in_pixels) {
       SetBoundsInPixels(adjusted_bounds_in_pixels);
+      if (!weak_this) {
+        return;
+      }
     }
   }
 
   // When we are in the process of requesting to maximize a window, we can
   // accurately keep track of our restored bounds instead of relying on the
   // heuristics that are in the PropertyNotify and ConfigureNotify handlers.
-  restored_bounds_in_pixels_ = GetBoundsInPixels();
+  gfx::Rect bounds_in_pixels = GetBoundsInPixels();
+  if (!weak_this) {
+    return;
+  }
+  restored_bounds_in_pixels_ = bounds_in_pixels;
 
   // Some WMs do not respect maximization hints on unmapped windows, so we
   // save this one for later too.
@@ -890,10 +927,15 @@
 }
 
 void X11Window::MoveCursorTo(const gfx::Point& location_px) {
+  auto weak_this = weak_ptr_factory_.GetWeakPtr();
+  const gfx::Rect bounds = GetBoundsInPixels();
+  if (!weak_this) {
+    return;
+  }
   connection_->WarpPointer(x11::WarpPointerRequest{
       .dst_window = x_root_window_,
-      .dst_x = static_cast<int16_t>(GetBoundsInPixels().x() + location_px.x()),
-      .dst_y = static_cast<int16_t>(GetBoundsInPixels().y() + location_px.y()),
+      .dst_x = static_cast<int16_t>(bounds.x() + location_px.x()),
+      .dst_y = static_cast<int16_t>(bounds.y() + location_px.y()),
   });
   // The cached cursor location is no longer valid.
   X11EventSource::GetInstance()->ClearLastCursorLocation();
@@ -906,7 +948,12 @@
     return;
   }
 
-  gfx::Rect barrier = bounds + GetBoundsInPixels().OffsetFromOrigin();
+  auto weak_this = weak_ptr_factory_.GetWeakPtr();
+  const gfx::Vector2d offset = GetBoundsInPixels().OffsetFromOrigin();
+  if (!weak_this) {
+    return;
+  }
+  gfx::Rect barrier = bounds + offset;
 
   auto make_barrier = [&](uint16_t x1, uint16_t y1, uint16_t x2, uint16_t y2,
                           x11::XFixes::BarrierDirections directions) {
@@ -1412,9 +1459,17 @@
          event->type() == ui::EventType::kTouchPressed)) {
       // Another X11Window has installed itself as capture. Translate the
       // event's location and dispatch to the other.
-      ConvertEventLocationToTargetWindowLocation(
-          located_events_grabber->GetBoundsInPixels().origin(),
-          GetBoundsInPixels().origin(), event->AsLocatedEvent());
+      const gfx::Point target_origin =
+          located_events_grabber->GetBoundsInPixels().origin();
+      if (!weak_this) {
+        return;
+      }
+      const gfx::Point current_origin = GetBoundsInPixels().origin();
+      if (!weak_this) {
+        return;
+      }
+      ConvertEventLocationToTargetWindowLocation(target_origin, current_origin,
+                                                 event->AsLocatedEvent());
     }
     return located_events_grabber->DispatchUiEvent(event, xev);
   }
@@ -1936,8 +1991,13 @@
   x11::SizeHints size_hints = {};
   connection_->GetWmNormalHints(xwindow_, &size_hints);

diff --git a/ui/ozone/platform/x11/x11_window_ozone_unittest.cc b/ui/ozone/platform/x11/x11_window_ozone_unittest.cc
index a45afbf..e759196 100644
--- a/ui/ozone/platform/x11/x11_window_ozone_unittest.cc
+++ b/ui/ozone/platform/x11/x11_window_ozone_unittest.cc
@@ -283,6 +283,90 @@
   gfx::Rect guessed_size_px_;
 };
 
+// Verifies that SetBoundsInPixels() is safe against the X11Window being
+// synchronously destroyed during the GetBoundsInPixels() call. This can happen
+// if GeometryCache::GetBoundsPx() processes synchronous X server replies and
+// dispatches an OnBoundsChanged event that closes the widget (e.g., as in
+// crbug.com/1068755).
+TEST_F(X11WindowOzoneTest, SetBoundsInPixelsUseAfterFreeViaGeometryCache) {
+  testing::NiceMock<MockPlatformWindowDelegate> delegate;
+  gfx::AcceleratedWidget widget;
+  constexpr gfx::Rect bounds(30, 80, 800, 600);
+  std::unique_ptr<PlatformWindow> window =
+      CreatePlatformWindow(&delegate, bounds, &widget, nullptr);
+
+  auto* connection = x11::Connection::Get();
+  auto xwindow = static_cast<x11::Window>(widget);
+
+  // Step 1: Force the X11Window's GeometryCache (and its parent chain) to
+  // become Ready and record last_notified_geometry_. This goes through the
+  // synchronous DispatchNow() path. The resulting X11Window::OnBoundsChanged
+  // sees a size change and only posts a delayed-resize task, so the delegate
+  // is not called synchronously here.
+  window->GetBoundsInPixels();
+
+  // Step 2: Create a real parent window at a non-zero offset so that after
+  // reparenting, only the absolute origin of |xwindow| changes (size stays
+  // 800x600). override_redirect avoids any WM interference.
+  x11::Window new_parent = connection->GenerateId<x11::Window>();
+  connection->CreateWindow({
+      .wid = new_parent,
+      .parent = connection->default_root(),
+      .x = 200,
+      .y = 200,
+      .width = 1000,
+      .height = 1000,
+      .c_class = x11::WindowClass::InputOnly,
+      .override_redirect = x11::Bool32(true),
+  });
+
+  // Step 3: Synthesize the ReparentNotify the X server would send for a WM
+  // reparent. GeometryCache::OnEvent replaces parent_ with a fresh un-Ready
+  // GeometryCache for |new_parent|, leaving the leaf cache's chain not Ready.
+  x11::ReparentNotifyEvent reparent{};
+  reparent.event = xwindow;
+  reparent.window = xwindow;
+  reparent.parent = new_parent;
+  reparent.x = 0;
+  reparent.y = 0;
+  x11::Event reparent_event(/*send_event=*/false, std::move(reparent));
+  connection->DispatchEvent(reparent_event);
+
+  // Step 4: Arm the delegate so that the *next* synchronous OnBoundsChanged
+  // (which will fire from inside SetBoundsInPixels → GetBoundsInPixels →
+  // GetBoundsPx → DispatchNow → OnBoundsChanged → NotifyBoundsChanged) frees
+  // the X11Window — modelling Widget::CloseNow → SetPlatformWindow(nullptr).
+  bool armed = true;
+  bool freed = false;
+  EXPECT_CALL(delegate, OnBoundsChanged(_))
+      .WillRepeatedly([&](const PlatformWindowDelegate::BoundsChange&) {
+        if (armed) {
+          armed = false;
+          freed = true;
+          // ~X11Window → PrepareForShutdown → Close → CloseXWindow →
+          // geometry_cache_.reset(). All GeometryCache weak_ptrs are
+          // invalidated, so every GetBoundsPx() frame on the stack will
+          // return {}, and SetBoundsInPixels() should guard against this
+          // deletion.
+          window.reset();
+        }
+      });
+
+  // Step 5: Call SetBoundsInPixels(). Inside, GetBoundsInPixels() recurses
+  // into the un-Ready parent cache, which processes replies synchronously via
+  // DispatchNow(). The resulting OnBoundsChanged event triggers the observer,
+  // which destroys the window. X11Window must safely return early instead
+  // of accessing freed members or the destroyed delegate.
+  PlatformWindow* raw = window.get();
+  raw->SetBoundsInPixels(gfx::Rect(40, 90, 800, 600));
+
+  // If we got here without ASAN tripping, the synchronous-free path was not
+  // exercised; surface that as a test failure rather than a silent pass.
+  EXPECT_TRUE(freed) << "delegate was never invoked synchronously";
+
+  connection->DestroyWindow({new_parent});
+}
+
 // Verifies X11Window sets fullscreen bounds in pixels when going to fullscreen.
 TEST_F(X11WindowOzoneTest, SetFullscreen) {
   constexpr gfx::Rect screen_bounds_in_px(640, 480, 1280, 720);