Skip to content
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/runtime/node/quic/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2646,6 +2646,29 @@ impl QuicEndpoint {
Ok(JSValue::UNDEFINED)
}

/// `lastChanceToFinalize` sweeps the endpoint / session / stream wrappers
/// in unspecified order, so by the time `release_native` runs
/// `lsquic_engine_destroy` the raw `*mut QuicSession` registries and the
/// session/stream boxes the vtable callbacks reach may already be freed.
/// This drops the registries, nulls `vtable.owner` so every owner-keyed
/// callback short-circuits in `ctx_ref`, and points `on_stream_close` /
/// `on_conn_closed` at variants that never follow a session or endpoint
/// back-pointer; the session/stream `finalize` methods null their own
/// lsquic ctx, which is how already-freed boxes stay out of the shim.
Comment thread
robobun marked this conversation as resolved.
Resolved
fn detach_for_finalize(&self) {
self.sessions.with_mut(Vec::clear);
self.pending_new_sessions.with_mut(Vec::clear);
self.provisional.with_mut(Vec::clear);
self.pending_verneg.with_mut(Vec::clear);
self.vtable.with_mut(|vt| {
if let Some(vt) = vt.as_mut() {
vt.owner = null_mut();
vt.on_stream_close = stream::QuicStream::on_close_detached;
vt.on_conn_closed = QuicSession::on_closed_detached;
}
});
}

#[expect(
clippy::boxed_local,
reason = "codegen's host_fn_finalize calls this as `|b| QuicEndpoint::finalize(b)` and requires `self: Box<Self>`"
Expand All @@ -2656,6 +2679,7 @@ impl QuicEndpoint {
if self.event_loop_timer.get().state == EventLoopTimerState::ACTIVE {
timer_all().remove(self.event_loop_timer.as_ptr());
}
self.detach_for_finalize();
self.release_native();
}
}
23 changes: 23 additions & 0 deletions src/runtime/node/quic/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2404,6 +2404,29 @@ impl QuicSession {
};
Self::transport_params_to_js(global, &tp)
}

/// `on_conn_closed` for `QuicEndpoint::detach_for_finalize`.
pub(super) unsafe extern "C" fn on_closed_detached(ctx: *mut c_void) {
// SAFETY: `finalize` nulls the lsquic ctx, so a non-null one is live;
// only `conn` is touched because `endpoint`/`streams` may be freed.
if let Some(session) = unsafe { super::ffi::ctx_ref::<QuicSession>(ctx) } {
session.conn.set(null_mut());
}
}

#[expect(
clippy::boxed_local,
reason = "codegen's host_fn_finalize calls this as `|b| QuicSession::finalize(b)` and requires `self: Box<Self>`"
)]
pub(crate) fn finalize(self: Box<Self>) {
let conn = self.conn.get();
if !conn.is_null() {
// SAFETY: see `QuicEndpoint::detach_for_finalize`; a non-null
// `conn` is live (on_conn_closed / teardown null it), and null is
// always a valid ctx.
unsafe { lsquic::lsquic_conn_set_ctx(conn, null_mut()) };
}
}
}

lsquic_callback! {
Expand Down
29 changes: 28 additions & 1 deletion src/runtime/node/quic/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,12 @@ impl QuicStream {
session_handle: JSValue,
raw: *mut lsquic::lsquic_stream,
) -> JsResult<(*mut QuicStream, JSValue)> {
// `raw` stays null until `bind_raw` below: if an allocation fails
// first, `create` returns Err, lsquic keeps a null ctx, and the
// orphan wrapper's `finalize` must not touch the stream.
Comment thread
robobun marked this conversation as resolved.
Resolved
let stream = QuicStream {
vtable,
raw: Cell::new(raw),
raw: Cell::new(null_mut()),
session: Cell::new(session),
session_js: JsCell::new(Some(Strong::create(session_handle, global))),
this_value: JsCell::new(JsRef::empty()),
Expand Down Expand Up @@ -610,6 +613,30 @@ impl QuicStream {
self.this_value.with_mut(|r| r.downgrade());
}

/// `on_stream_close` for `QuicEndpoint::detach_for_finalize`.
pub(super) unsafe extern "C" fn on_close_detached(
ctx: *mut c_void,
_s: *mut lsquic::lsquic_stream,
) {
// SAFETY: `finalize` nulls the lsquic ctx, so a non-null one is live;
// only `raw` is touched because `session` may already be freed.
if let Some(qs) = unsafe { super::ffi::ctx_ref::<QuicStream>(ctx) } {
qs.raw.set(null_mut());
}
}

#[expect(
clippy::boxed_local,
reason = "codegen's host_fn_finalize calls this as `|b| QuicStream::finalize(b)` and requires `self: Box<Self>`"
)]
pub(crate) fn finalize(self: Box<Self>) {
if let Some(s) = self.ls() {
// SAFETY: see `QuicEndpoint::detach_for_finalize`; a non-null `raw`
// is live by the `ls()` invariant, and null is always a valid ctx.
unsafe { s.set_ctx(null_mut()) };
}
}
Comment thread
robobun marked this conversation as resolved.
Resolved

pub(crate) fn get_reader(&self, _g: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
self.with_state(|s| s.has_reader = 1);
Ok(frame.this())
Expand Down
27 changes: 27 additions & 0 deletions test/js/node/quic/quic-endpoint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,3 +310,30 @@ describe("endpoint.close() while a session is live", () => {
expect({ announced, resolved }).toEqual({ announced: 1, resolved: true });
});
});

// Worker VM teardown runs lastChanceToFinalize, which sweeps the QUIC wrappers
// in unspecified order. QuicEndpoint::finalize -> lsquic_engine_destroy drives
// on_close/on_conn_closed into per-stream/per-session ctx pointers; when those
// wrappers were swept first the ctx is a freed box. ASAN reports
// heap-use-after-free at nq_on_stream_close / nq_on_conn_closed, and a release
// build segfaults (exit 139).
describe("node:quic in a Worker", () => {
test("worker.terminate() with a live endpoint + session does not UAF at VM teardown", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
join(import.meta.dir, "quic-worker-terminate-fixture.ts"),
join(keysDir, "agent1-key.pem"),
join(keysDir, "agent1-cert.pem"),
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
// stderr carries the per-worker ExperimentalWarning (random blob URLs) on
// a clean run and the ASAN report on regression; keep it in the received
// object so the failure diff shows which callback UAF'd.
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "PASS\n", stderr: expect.any(String), exitCode: 0 });
}, 30000);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
});
68 changes: 68 additions & 0 deletions test/js/node/quic/quic-worker-terminate-fixture.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading