[X11] Fix UAF in XDragContext and XDragDropClient during XDND events [chromium/src : main]
0 views
Skip to first unread message
Thomas Anderson (Gerrit)
Jul 13, 2026, 10:43:12 AM (10 days ago) Jul 13
to Lei Zhang, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org
Attention needed from Lei Zhang
Thomas Anderson voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Lei Zhang
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Jul 13, 2026, 12:12:59 PM (10 days ago) Jul 13
to Thomas Anderson, Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org
Attention needed from Thomas Anderson
Lei Zhang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
4 comments
Commit Message
File ui/base/x/x11_drag_drop_client.cc
Line 482, Patchset 2 (Latest):
base::WeakPtr<XDragDropClient> alive = weak_factory_.GetWeakPtr();Lei Zhang . unresolved
Can swap with the next line and have a narrower scope.
File ui/ozone/platform/x11/test/x11_drag_drop_client_unittest.cc
Line 1020, Patchset 2 (Latest):
EXPECT_EQ(delegate.client(), nullptr);Lei Zhang . unresolved
`EXPECT_FALSE(delegate.client());`. Similarly for the next test case.
Line 1031, Patchset 2 (Latest):
EXPECT_NE(delegate.client(), nullptr);Lei Zhang . unresolved
Needs to be ASSERT_TRUE() to avoid potentially crashing on the next line.
Related details
Attention is currently required from:
- Thomas Anderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Jul 13, 2026, 12:28:02 PM (10 days ago) Jul 13
to Lei Zhang, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org
Thomas Anderson voted and added 4 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Auto-Submit | +0 |
| Commit-Queue | +2 |
4 comments
Commit Message
File ui/base/x/x11_drag_drop_client.cc
Line 482, Patchset 2:
base::WeakPtr<XDragDropClient> alive = weak_factory_.GetWeakPtr();Lei Zhang . resolved
Can swap with the next line and have a narrower scope.
Thomas Anderson
Done
File ui/ozone/platform/x11/test/x11_drag_drop_client_unittest.cc
`EXPECT_FALSE(delegate.client());`. Similarly for the next test case.
Thomas Anderson
Done
Needs to be ASSERT_TRUE() to avoid potentially crashing on the next line.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 13, 2026, 1:29:06 PM (10 days ago) Jul 13
to Thomas Anderson, Lei Zhang, chromium...@chromium.org, Sadrul Chowdhury, ozone-...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/ozone/platform/x11/test/x11_drag_drop_client_unittest.cc
Insertions: 3, Deletions: 3.
@@ -1017,7 +1017,7 @@
delegate.destroy_during_update_drag_ = true;
DriveSelectionNotifyPath(delegate.client(), static_cast<x11::Window>(1));
- EXPECT_EQ(delegate.client(), nullptr);
+ EXPECT_FALSE(delegate.client());
}
TEST_F(X11DragDropClientTest,
@@ -1028,8 +1028,8 @@
delegate.SetClient(std::move(client));
DriveSelectionNotifyPath(delegate.client(), static_cast<x11::Window>(1));
- EXPECT_NE(delegate.client(), nullptr);
- EXPECT_EQ(delegate.client()->target_current_context(), nullptr);
+ ASSERT_TRUE(delegate.client());
+ EXPECT_FALSE(delegate.client()->target_current_context());
}
} // namespace ui
```
```
The name of the file: ui/base/x/x11_drag_drop_client.cc
Insertions: 4, Deletions: 5.
@@ -479,13 +479,12 @@
void XDragDropClient::OnSelectionNotify(
const x11::SelectionNotifyEvent& xselection) {
DVLOG(1) << "OnSelectionNotify";
- base::WeakPtr<XDragDropClient> alive = weak_factory_.GetWeakPtr();
if (target_current_context_) {
+ base::WeakPtr<XDragDropClient> alive = weak_factory_.GetWeakPtr();
target_current_context_->OnSelectionNotify(xselection);
- }
-
- if (!alive) {
- return;
+ if (!alive) {
+ return;
+ }
}
// ICCCM requires us to delete the property passed into SelectionNotify.
```
Change information
Commit message:
[X11] Fix UAF in XDragContext and XDragDropClient during XDND events
During nested run loop execution triggered by drag-and-drop delegates,
re-entrant events (e.g. XdndLeave or window closure) can destroy the
active XDragContext or its parent XDragDropClient. Upon returning to
outer stack frames, this leads to unguarded UAF access.
This CL fixes this by:
1. Clearing `drag_drop_client_` and snapshotting it before calling
`CompleteXdndPosition()` in
`XDragContext::RequestNextTargetOrComplete()`. This ensures that no
member of `this` is accessed after the re-entrant loop returns.
2. Using a `WeakPtr` guard in `XDragDropClient::OnSelectionNotify()`
to avoid accessing the freed client when deleting the selection
property.
Change-Id: I6567b83abf16d57b289aa95682fc25ce3fb54993
Fixed: 533603842
Commit-Queue: Thomas Anderson <thomasa...@chromium.org>
Reviewed-by: Lei Zhang <the...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1661204}
Files:
- M ui/base/x/x11_drag_context.cc
- M ui/base/x/x11_drag_drop_client.cc
- M ui/ozone/platform/x11/test/x11_drag_drop_client_unittest.cc
Change size: M
Delta: 3 files changed, 111 insertions(+), 1 deletion(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Lei Zhang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages