FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

quic: fixup UAFs in bindingdata, streams, and app · nodejs/node@9a72949 · GitHub

/ node Public

Commit 9a72949

Browse files
jasnell
authored and
aduh95
committed
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+
void CheckWrap::Start() {
156+
if (check_.data == nullptr) return;
157+
uv_check_start(&check_, OnCheck);
158+
}
159+
160+
void CheckWrap::Stop() {
161+
if (check_.data == nullptr) return;
162+
uv_check_stop(&check_);
163+
}
164+
165+
void CheckWrap::Close() {
166+
check_.data = nullptr;
167+
env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&check_), CheckClosedCb);
168+
}
169+
170+
void CheckWrap::Ref() {
171+
if (check_.data == nullptr) return;
172+
uv_ref(reinterpret_cast<uv_handle_t*>(&check_));
173+
}
174+
175+
void CheckWrap::Unref() {
176+
if (check_.data == nullptr) return;
177+
uv_unref(reinterpret_cast<uv_handle_t*>(&check_));
178+
}
179+
180+
void CheckWrap::OnCheck(uv_check_t* check) {
181+
CheckWrap* wrap = ContainerOf(&CheckWrap::check_, check);
182+
wrap->fn_();
183+
}
184+
185+
void CheckWrap::CheckClosedCb(uv_handle_t* handle) {
186+
std::unique_ptr<CheckWrap> ptr(
187+
ContainerOf(&CheckWrap::check_, reinterpret_cast<uv_check_t*>(handle)));
188+
}
189+
190+
void CheckWrapHandle::Start() {
191+
if (check_ != nullptr) check_->Start();
192+
}
193+
194+
void CheckWrapHandle::Stop() {
195+
if (check_ != nullptr) check_->Stop();
196+
}
197+
198+
void CheckWrapHandle::Close() {
199+
if (check_ != nullptr) {
200+
check_->env()->RemoveCleanupHook(CleanupHook, this);
201+
check_->Close();
202+
}
203+
check_ = nullptr;
204+
}
205+
206+
void CheckWrapHandle::Ref() {
207+
if (check_ != nullptr) check_->Ref();
208+
}
209+
210+
void CheckWrapHandle::Unref() {
211+
if (check_ != nullptr) check_->Unref();
212+
}
213+
214+
void CheckWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
215+
if (check_ != nullptr) tracker->TrackField("check", *check_);
216+
}
217+
218+
void CheckWrapHandle::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
void BindingData::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-
void BindingData::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+
void BindingData::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+
class CheckWrap final : public MemoryRetainer {
170+
public:
171+
using CheckCb = std::function<void()>;
172+
173+
template <typename... Args>
174+
explicit CheckWrap(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+
void Start();
185+
void Stop();
186+
void Close();
187+
void Ref();
188+
void Unref();
189+
190+
SET_NO_MEMORY_INFO()
191+
SET_MEMORY_INFO_NAME(CheckWrap)
192+
SET_SELF_SIZE(CheckWrap)
193+
194+
private:
195+
static void OnCheck(uv_check_t* check);
196+
static void CheckClosedCb(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+
class CheckWrapHandle : public MemoryRetainer {
207+
public:
208+
template <typename... Args>
209+
explicit CheckWrapHandle(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+
inline operator bool() const { return check_ != nullptr; }
219+
220+
void Start();
221+
void Stop();
222+
void Close();
223+
void Ref();
224+
void Unref();
225+
226+
void MemoryInfo(node::MemoryTracker* tracker) const override;
227+
228+
SET_MEMORY_INFO_NAME(CheckWrapHandle)
229+
SET_SELF_SIZE(CheckWrapHandle)
230+
231+
private:
232+
static void CleanupHook(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-
static void OnFlushCheck(uv_check_t* handle);
358+
void OnFlushCheck();
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)
Back | FazBrowse Home | New Git URL