Skip to content

Commit 0d3f57f

Browse files
VerteDindejkleinscClaude
authored
chore: cherry-pick 074d472db745 from chromium (#50449)
* chore: cherry-pick 074d472db745 from chromium * chore: update patches Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com> --------- Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Claude <svc-devxp-claude@slack-corp.com>
1 parent 6247116 commit 0d3f57f

2 files changed

Lines changed: 297 additions & 0 deletions

File tree

‎patches/chromium/.patches‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ fix_update_dbus_signal_signature_for_xdg_globalshortcuts_portal.patch
151151
patch_osr_control_screen_info.patch
152152
cherry-pick-12f932985275.patch
153153
fix_mac_high_res_icons.patch
154+
cherry-pick-074d472db745.patch
154155
cherry-pick-50b057660b4d.patch
155156
cherry-pick-45c5a70d984d.patch
156157
cherry-pick-05e4b544803c.patch

‎patches/chromium/cherry-pick-074d472db745.patch‎

Lines changed: 296 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,296 @@
1+
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
2+
From: Mikel Astiz <mastiz@chromium.org>
3+
Date: Tue, 10 Mar 2026 13:22:17 -0700
4+
Subject: [M146][base] Fix UAF in base::OnceCallbackList on re-entrant Notify()
5+
6+
Before this patch, `base::OnceCallbackList` was susceptible to a
7+
heap-use-after-free when `Notify()` was called re-entrantly.
8+
9+
The UAF occurred because `OnceCallbackList::RunCallback()` immediately
10+
spliced executed nodes out of `callbacks_` and into `null_callbacks_`.
11+
If a nested `Notify()` executed a node that an outer `Notify()` loop was
12+
already holding an iterator to, and that node's subscription was
13+
subsequently destroyed during the re-entrant cycle, the node would be
14+
physically erased from `null_callbacks_`. When control returned to the
15+
outer loop, it would attempt to evaluate the now-dangling iterator.
16+
17+
This CL fixes the bug by deferring list mutations until the outermost
18+
iteration completes:
19+
1. `RunCallback()` no longer splices nodes during iteration.
20+
2. Cancellation logic is pushed down to the subclasses via a new
21+
`CancelCallback()` hook, which is an extension to the pre-existing
22+
`CancelNullCallback()` with increased responsibilities and clearer
23+
semantics.
24+
3. If a subscription is destroyed while `is_iterating` is true,
25+
`OnceCallbackList` resets the node and stashes its iterator in
26+
`pending_erasures_`.
27+
4. A new `CleanUpNullCallbacksPostIteration()` phase runs at the end
28+
of the outermost `Notify()`, which safely splices executed nodes
29+
into `null_callbacks_` and physically erases the pending dead nodes.
30+
31+
As a side effect, the type-trait hack in `Notify()` based on
32+
`is_instantiation<CallbackType, OnceCallback>` can be removed, because
33+
this information is exposed directly by
34+
`OnceCallbackList::CleanUpNullCallbacksPostIteration()`.
35+
36+
The newly-added unit-test
37+
CallbackListTest.OnceCallbackListCancelDuringReentrantNotify reproduces
38+
the scenario and crashed before this patch.
39+
40+
(cherry picked from commit 36acd49636845be2419269acbe9a5137da3d5d96)
41+
42+
Change-Id: I6b1e2bcb97be1bc8d6a15e5ca7511992e00e1772
43+
Fixed: 489381399
44+
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7627506
45+
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
46+
Reviewed-by: Gabriel Charette <gab@chromium.org>
47+
Cr-Original-Commit-Position: refs/heads/main@{#1594520}
48+
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7653916
49+
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
50+
Cr-Commit-Position: refs/branch-heads/7680@{#2287}
51+
Cr-Branched-From: 76b7d80e5cda23fe6537eed26d68c92e995c7f39-refs/heads/main@{#1582197}
52+
53+
diff --git a/base/callback_list.h b/base/callback_list.h
54+
index 82cb11dc0ee02906b009cc383c41a056861199d0..d5f99cf685486f1ea74718b4e6b228a5d83f0c29 100644
55+
--- a/base/callback_list.h
56+
+++ b/base/callback_list.h
57+
@@ -9,6 +9,7 @@
58+
#include <list>
59+
#include <memory>
60+
#include <utility>
61+
+#include <vector>
62+
63+
#include "base/auto_reset.h"
64+
#include "base/base_export.h"
65+
@@ -16,7 +17,6 @@
66+
#include "base/functional/bind.h"
67+
#include "base/functional/callback.h"
68+
#include "base/memory/weak_ptr.h"
69+
-#include "base/types/is_instantiation.h"
70+
71+
// OVERVIEW:
72+
//
73+
@@ -240,17 +240,14 @@ class CallbackListBase {
74+
75+
// Any null callbacks remaining in the list were canceled due to
76+
// Subscription destruction during iteration, and can safely be erased now.
77+
- const size_t erased_callbacks =
78+
- std::erase_if(callbacks_, [](const auto& cb) { return cb.is_null(); });
79+
-
80+
- // Run |removal_callback_| if any callbacks were canceled. Note that we
81+
- // cannot simply compare list sizes before and after iterating, since
82+
- // notification may result in Add()ing new callbacks as well as canceling
83+
- // them. Also note that if this is a OnceCallbackList, the OnceCallbacks
84+
- // that were executed above have all been removed regardless of whether
85+
- // they're counted in |erased_callbacks_|.
86+
- if (removal_callback_ &&
87+
- (erased_callbacks || is_instantiation<CallbackType, OnceCallback>)) {
88+
+ const bool any_callbacks_erased = static_cast<CallbackListImpl*>(this)
89+
+ ->CleanUpNullCallbacksPostIteration();
90+
+
91+
+ // Run |removal_callback_| if any callbacks were canceled or executed. Note
92+
+ // that simply comparing list sizes before and after iterating cannot be
93+
+ // done, since notification may result in Add()ing new callbacks as well as
94+
+ // canceling them.
95+
+ if (removal_callback_ && any_callbacks_erased) {
96+
removal_callback_.Run(); // May delete |this|!
97+
}
98+
}
99+
@@ -264,21 +261,9 @@ class CallbackListBase {
100+
private:
101+
// Cancels the callback pointed to by |it|, which is guaranteed to be valid.
102+
void CancelCallback(const typename Callbacks::iterator& it) {
103+
- if (static_cast<CallbackListImpl*>(this)->CancelNullCallback(it)) {
104+
- return;
105+
- }
106+
-
107+
- if (iterating_) {
108+
- // Calling erase() here is unsafe, since the loop in Notify() may be
109+
- // referencing this same iterator, e.g. if adjacent callbacks'
110+
- // Subscriptions are both destroyed when the first one is Run(). Just
111+
- // reset the callback and let Notify() clean it up at the end.
112+
- it->Reset();
113+
- } else {
114+
- callbacks_.erase(it);
115+
- if (removal_callback_) {
116+
- removal_callback_.Run(); // May delete |this|!
117+
- }
118+
+ if (static_cast<CallbackListImpl*>(this)->CancelCallback(it, iterating_) &&
119+
+ removal_callback_) {
120+
+ removal_callback_.Run(); // May delete |this|!
121+
}
122+
}
123+
124+
@@ -304,23 +289,71 @@ class OnceCallbackList
125+
// Runs the current callback, which may cancel it or any other callbacks.
126+
template <typename... RunArgs>
127+
void RunCallback(typename Traits::Callbacks::iterator it, RunArgs&&... args) {
128+
- // OnceCallbacks still have Subscriptions with outstanding iterators;
129+
- // splice() removes them from |callbacks_| without invalidating those.
130+
- null_callbacks_.splice(null_callbacks_.end(), this->callbacks_, it);
131+
+ // Do not splice here. Splicing during iteration breaks re-entrant Notify()
132+
+ // by invalidating the outer loop's iterator. Splicing is deferred to
133+
+ // CleanUpNullCallbacksPostIteration(), which is called when the outermost
134+
+ // Notify() finishes.
135+
136+
// NOTE: Intentionally does not call std::forward<RunArgs>(args)...; see
137+
// comments in Notify().
138+
std::move(*it).Run(args...);
139+
}
140+
141+
- // If |it| refers to an already-canceled callback, does any necessary cleanup
142+
- // and returns true. Otherwise returns false.
143+
- bool CancelNullCallback(const typename Traits::Callbacks::iterator& it) {
144+
+ // Called during subscription destruction to cancel the callback. Returns true
145+
+ // if the callback was removed from the active list and the generic removal
146+
+ // callback should be executed. Returns false if the callback was already
147+
+ // executed, or if the erasure is deferred due to active iteration.
148+
+ bool CancelCallback(const typename Traits::Callbacks::iterator& it,
149+
+ bool is_iterating) {
150+
+ if (is_iterating) {
151+
+ // During iteration, nodes cannot be safely erased from |callbacks_|
152+
+ // without invalidating iterators. They also cannot be spliced into
153+
+ // |null_callbacks_| right now. Thus, the node is reset and tracked for
154+
+ // erasure in CleanUpNullCallbacksPostIteration().
155+
+ it->Reset();
156+
+ pending_erasures_.push_back(it);
157+
+ return false;
158+
+ }
159+
+
160+
if (it->is_null()) {
161+
+ // The callback already ran, so it's safely sitting in |null_callbacks_|.
162+
null_callbacks_.erase(it);
163+
- return true;
164+
+ return false;
165+
}
166+
- return false;
167+
+
168+
+ // The callback hasn't run yet, so it's still in |callbacks_|.
169+
+ this->callbacks_.erase(it);
170+
+ return true;
171+
+ }
172+
+
173+
+ // Performs post-iteration cleanup. Successfully executed callbacks (which
174+
+ // become null) are spliced into |null_callbacks_| to keep their
175+
+ // Subscriptions' iterators valid. Callbacks explicitly canceled during
176+
+ // iteration (tracked in |pending_erasures_|) are erased. Returns true if any
177+
+ // callbacks were erased or spliced out.
178+
+ bool CleanUpNullCallbacksPostIteration() {
179+
+ bool any_spliced = false;
180+
+ for (auto it = this->callbacks_.begin(); it != this->callbacks_.end();) {
181+
+ if (it->is_null()) {
182+
+ any_spliced = true;
183+
+ auto next = std::next(it);
184+
+ null_callbacks_.splice(null_callbacks_.end(), this->callbacks_, it);
185+
+ it = next;
186+
+ } else {
187+
+ ++it;
188+
+ }
189+
+ }
190+
+
191+
+ bool any_erased = !pending_erasures_.empty();
192+
+ for (auto pending_it : pending_erasures_) {
193+
+ // Note: `pending_it` was originally an iterator into `callbacks_`, but
194+
+ // the node it points to has just been spliced into `null_callbacks_`. The
195+
+ // iterator itself remains valid and can now be used for erasure from
196+
+ // `null_callbacks_`.
197+
+ null_callbacks_.erase(pending_it);
198+
+ }
199+
+ pending_erasures_.clear();
200+
+ return any_spliced || any_erased;
201+
}
202+
203+
// Holds null callbacks whose Subscriptions are still alive, so the
204+
@@ -328,6 +361,11 @@ class OnceCallbackList
205+
// OnceCallbacks, since RepeatingCallbacks are not canceled except by
206+
// Subscription destruction.
207+
typename Traits::Callbacks null_callbacks_;
208+
+
209+
+ // Holds iterators for callbacks canceled during iteration.
210+
+ // Erasure is deferred to CleanUpNullCallbacksPostIteration() when iteration
211+
+ // completes to prevent invalidating iterators that an outer loop might hold.
212+
+ std::vector<typename Traits::Callbacks::iterator> pending_erasures_;
213+
};
214+
215+
template <typename Signature>
216+
@@ -344,14 +382,29 @@ class RepeatingCallbackList
217+
it->Run(args...);
218+
}
219+
220+
- // If |it| refers to an already-canceled callback, does any necessary cleanup
221+
- // and returns true. Otherwise returns false.
222+
- bool CancelNullCallback(const typename Traits::Callbacks::iterator& it) {
223+
- // Because at most one Subscription can point to a given callback, and
224+
- // RepeatingCallbacks are only reset by CancelCallback(), no one should be
225+
- // able to request cancellation of a canceled RepeatingCallback.
226+
- DCHECK(!it->is_null());
227+
- return false;
228+
+ // Called during subscription destruction to cancel the callback. Returns true
229+
+ // if the callback was removed from the active list and the generic removal
230+
+ // callback should be executed. Returns false if the callback was already
231+
+ // executed, or if the erasure is deferred due to active iteration.
232+
+ bool CancelCallback(const typename Traits::Callbacks::iterator& it,
233+
+ bool is_iterating) {
234+
+ if (is_iterating) {
235+
+ // During iteration, nodes cannot be safely erased from |callbacks_|
236+
+ // without invalidating iterators. The node is reset and will be swept up
237+
+ // by CleanUpNullCallbacksPostIteration().
238+
+ it->Reset();
239+
+ return false;
240+
+ }
241+
+
242+
+ this->callbacks_.erase(it);
243+
+ return true;
244+
+ }
245+
+
246+
+ // Performs post-iteration cleanup by erasing all canceled callbacks. Returns
247+
+ // true if any callbacks were erased.
248+
+ bool CleanUpNullCallbacksPostIteration() {
249+
+ return std::erase_if(this->callbacks_,
250+
+ [](const auto& cb) { return cb.is_null(); }) > 0;
251+
}
252+
};
253+
254+
diff --git a/base/callback_list_unittest.cc b/base/callback_list_unittest.cc
255+
index 7474278525e5efecc0de903809a54d366896d524..a855443fbae862befbc3a2a484ea335632136e94 100644
256+
--- a/base/callback_list_unittest.cc
257+
+++ b/base/callback_list_unittest.cc
258+
@@ -10,6 +10,7 @@
259+
#include "base/functional/bind.h"
260+
#include "base/functional/callback_helpers.h"
261+
#include "base/memory/raw_ptr.h"
262+
+#include "base/test/bind.h"
263+
#include "base/test/test_future.h"
264+
#include "testing/gtest/include/gtest/gtest.h"
265+
266+
@@ -577,6 +578,30 @@ TEST(CallbackListTest, ReentrantNotify) {
267+
EXPECT_EQ(1, d.total());
268+
}
269+
270+
+// Regression test for crbug.com/489381399: Verifies Notify() can be called
271+
+// reentrantly for OnceCallbackList even if a callback is canceled during the
272+
+// reentrant notification.
273+
+TEST(CallbackListTest, OnceCallbackListCancelDuringReentrantNotify) {
274+
+ OnceClosureList cb_reg;
275+
+ CallbackListSubscription sub_a, sub_b;
276+
+
277+
+ auto cb_a = base::BindLambdaForTesting([&]() {
278+
+ // Re-entrant notification.
279+
+ cb_reg.Notify();
280+
+ // After re-entrant notification returns, sub_b has been run. Destroying it
281+
+ // now should be a no-op.
282+
+ sub_b = {};
283+
+ });
284+
+
285+
+ auto cb_b = base::DoNothing();
286+
+
287+
+ sub_a = cb_reg.Add(std::move(cb_a));
288+
+ sub_b = cb_reg.Add(std::move(cb_b));
289+
+
290+
+ // This should not crash.
291+
+ cb_reg.Notify();
292+
+}
293+
+
294+
TEST(CallbackListTest, ClearPreventsInvocation) {
295+
Listener listener;
296+
RepeatingClosureList cb_reg;

0 commit comments

Comments
 (0)