Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Commit 9a72949

Browse files
jasnelladuh95
authored andcommitted
quic: fixup UAFs in bindingdata, streams, and app
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6 PR-URL: #63267 Backport-PR-URL: #64675 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a4db121 commit 9a72949

4 files changed

Lines changed: 174 additions & 32 deletions

File tree

‎src/quic/application.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ void Session::Application::SendPendingData() {
369369
if (closed) return;
370370
// Flush any remaining accumulated packets before updating stats.
371371
flush_batch();
372-
if (session().is_destroyed()) [[unlikely]]return;
372+
if (session().is_destroyed()) [[unlikely]]
373+
return;
373374

374375
// Get a strong pointer to protect against potential destruction during
375376
// updating the time and data stats.

‎src/quic/bindingdata.cc‎

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,87 @@ void* Nghttp3Realloc(void* ptr, size_t size, void* ud) {
149149
}
150150
} // namespace
151151

152+
// ============================================================================
153+
// CheckWrap / CheckWrapHandle
154+
155+
voidCheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
voidCheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
voidCheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
voidCheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
voidCheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
voidCheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
voidCheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
voidCheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
voidCheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
voidCheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
voidCheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
voidCheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
voidCheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
voidCheckWrapHandle::CleanupHook(void* data) {
219+
static_cast<CheckWrapHandle*>(data)->Close();
220+
}
221+
222+
// ============================================================================
223+
152224
BindingData& BindingData::Get(Environment* env) {
153225
return *(env->principal_realm()->GetBindingData<BindingData>());
154226
}
155227

156228
BindingData::~BindingData() {
157229
quic_alloc_state.binding = nullptr;
158-
if (flush_check_initialized_) {
159-
uv_check_stop(&flush_check_);
160-
flush_check_started_ = false;
161-
// The check handle is closed inline here. Because BindingData destruction
162-
// happens during Environment cleanup, the handle will be finalized by
163-
// libuv's close phase.
164-
uv_close(reinterpret_cast<uv_handle_t*>(&flush_check_), nullptr);
165-
flush_check_initialized_ = false;
166-
}
230+
// flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
231+
// body completes. The inner CheckWrap (and its uv_check_t) will be freed
232+
// later by the uv_close callback, after CleanupHandles() runs uv_run().
167233
pending_flush_sessions_.clear();
168234
}
169235

@@ -230,13 +296,11 @@ void BindingData::RegisterExternalReferences(
230296
}
231297

232298
BindingData::BindingData(Realm* realm, Local<Object> object)
233-
: BaseObject(realm, object) {
299+
: BaseObject(realm, object),
300+
flush_check_(env(), [this]() { OnFlushCheck(); }) {
234301
MakeWeak();
235-
CHECK_EQ(uv_check_init(env()->event_loop(), &flush_check_), 0);
236-
flush_check_.data = this;
237302
// Unref so the check handle doesn't keep the event loop alive on its own.
238-
uv_unref(reinterpret_cast<uv_handle_t*>(&flush_check_));
239-
flush_check_initialized_ = true;
303+
flush_check_.Unref();
240304
}
241305

242306
SessionManager& BindingData::session_manager() {
@@ -249,27 +313,26 @@ SessionManager& BindingData::session_manager() {
249313
voidBindingData::ScheduleSessionFlush(const BaseObjectPtr<Session>& session) {
250314
pending_flush_sessions_.push_back(session);
251315
if (!flush_check_started_) {
252-
uv_check_start(&flush_check_, OnFlushCheck);
316+
flush_check_.Start();
253317
flush_check_started_ = true;
254318
}
255319
}
256320

257-
voidBindingData::OnFlushCheck(uv_check_t* handle) {
258-
auto* binding = static_cast<BindingData*>(handle->data);
259-
if (binding->pending_flush_sessions_.empty()) {
260-
uv_check_stop(&binding->flush_check_);
261-
binding->flush_check_started_ = false;
321+
voidBindingData::OnFlushCheck() {
322+
if (pending_flush_sessions_.empty()) {
323+
flush_check_.Stop();
324+
flush_check_started_ = false;
262325
return;
263326
}
264327

265-
HandleScope scope(binding->env()->isolate());
328+
HandleScope scope(env()->isolate());
266329

267330
// Swap to a local vector before iterating. SendPendingData may trigger
268331
// MakeCallback which runs JS that could cause more packet receives via
269332
// re-entry (e.g., a stream data callback that synchronously writes to
270333
// another session). Any sessions added during the flush remain in
271334
// pending_flush_sessions_ and are picked up on the next check tick.
272-
auto sessions = std::move(binding->pending_flush_sessions_);
335+
auto sessions = std::move(pending_flush_sessions_);
273336
for (auto& session : sessions) {
274337
session->pending_flush_ = false;
275338
if (!session->is_destroyed()) {
@@ -279,9 +342,9 @@ void BindingData::OnFlushCheck(uv_check_t* handle) {
279342

280343
// If no new sessions were added during the flush, stop the check
281344
// to avoid per-tick callback overhead when idle.
282-
if (binding->pending_flush_sessions_.empty()) {
283-
uv_check_stop(&binding->flush_check_);
284-
binding->flush_check_started_ = false;
345+
if (pending_flush_sessions_.empty()) {
346+
flush_check_.Stop();
347+
flush_check_started_ = false;
285348
}
286349
}
287350

‎src/quic/bindingdata.h‎

Lines changed: 79 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include<node_mem.h>
1313
#include<uv.h>
1414
#include<v8.h>
15+
#include<functional>
1516
#include<memory>
1617
#include<unordered_map>
1718
#include<vector>
@@ -157,6 +158,81 @@ class SessionManager;
157158
V(verify_private_key, "verifyPrivateKey") \
158159
V(version, "version")
159160

161+
// =============================================================================
162+
// Lightweight wrappers around uv_check_t that ensure safe handle closure.
163+
// The check handle is embedded in a heap-allocated CheckWrap whose destruction
164+
// is deferred until the uv_close callback fires, preventing use-after-free
165+
// when the owning object is destroyed before libuv finishes closing the handle.
166+
// Follows the same two-layer pattern as TimerWrap / TimerWrapHandle
167+
// (see timer_wrap.h).
168+
// TODO(@jasnell): Consider moving it out to a separate file like timer_wrap.h.
169+
classCheckWrapfinal : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicitCheckWrap(Environment* env, Args&&... args)
175+
: env_(env), fn_(std::forward<Args>(args)...) {
176+
uv_check_init(env->event_loop(), &check_);
177+
check_.data = this;
178+
}
179+
180+
DISALLOW_COPY_AND_MOVE(CheckWrap)
181+
182+
inline Environment* env() const { return env_; }
183+
184+
voidStart();
185+
voidStop();
186+
voidClose();
187+
voidRef();
188+
voidUnref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
staticvoidOnCheck(uv_check_t* check);
196+
staticvoidCheckClosedCb(uv_handle_t* handle);
197+
~CheckWrap() = default;
198+
199+
Environment* env_;
200+
CheckCb fn_;
201+
uv_check_t check_;
202+
203+
friend std::unique_ptr<CheckWrap>::deleter_type;
204+
};
205+
206+
classCheckWrapHandle : publicMemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicitCheckWrapHandle(Environment* env, Args&&... args)
210+
: check_(new CheckWrap(env, std::forward<Args>(args)...)) {
211+
env->AddCleanupHook(CleanupHook, this);
212+
}
213+
214+
DISALLOW_COPY_AND_MOVE(CheckWrapHandle)
215+
216+
~CheckWrapHandle() { Close(); }
217+
218+
inlineoperatorbool() const { return check_ != nullptr; }
219+
220+
voidStart();
221+
voidStop();
222+
voidClose();
223+
voidRef();
224+
voidUnref();
225+
226+
voidMemoryInfo(node::MemoryTracker* tracker) constoverride;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
staticvoidCleanupHook(void* data);
233+
CheckWrap* check_;
234+
};
235+
160236
// =============================================================================
161237
// The BindingState object holds state for the internalBinding('quic') binding
162238
// instance. It is mostly used to hold the persistent constructors, strings, and
@@ -271,16 +347,15 @@ class BindingData final
271347
ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
272348
ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
273349

274-
// Deferred send flush state. The uv_check_t fires immediately after
350+
// Deferred send flush state. The CheckWrapHandle fires immediately after
275351
// the I/O poll phase in the same event loop tick, allowing batched
276352
// receive processing: all packets are read during poll, then
277353
// SendPendingData is called once per dirty session in the check callback.
278-
uv_check_t flush_check_;
354+
CheckWrapHandle flush_check_;
279355
std::vector<BaseObjectPtr<Session>> pending_flush_sessions_;
280356
bool flush_check_started_ = false;
281-
bool flush_check_initialized_ = false;
282357

283-
staticvoidOnFlushCheck(uv_check_t* handle);
358+
voidOnFlushCheck();
284359
};
285360

286361
JS_METHOD_IMPL(IllegalConstructor);

‎src/quic/streams.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,8 +1563,11 @@ void Stream::Destroy(QuicError error) {
15631563
auto session = session_;
15641564
session_.reset();
15651565
// EmitClose above triggers MakeCallback which can destroy the session
1566-
// via JS re-entrancy. The weak pointer may now be null.
1567-
if (session) session->RemoveStream(id());
1566+
// via JS re-entrancy. The weak pointer may still be non-null (the
1567+
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
1568+
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
1569+
// check is_destroyed() to avoid dereferencing the null impl_.
1570+
if (session && !session->is_destroyed()) session->RemoveStream(id());
15681571

15691572
// Critically, make sure that the RemoveStream call is the last thing
15701573
// trying to use this stream object. Once that call is made, the stream

0 commit comments

Comments
 (0)