From cabca4b5e70381f3c670b15b7b1ed7bed11dce8b Mon Sep 17 00:00:00 2001 From: zerone0x Date: Fri, 6 Mar 2026 08:06:05 +0100 Subject: [PATCH 1/3] quic: guard against null impl_ in UpdateDataStats When a QUIC session's handshake fails or the connection is terminated early, UpdateDataStats() can be called before impl_ has been initialized, leading to a null pointer dereference and SIGSEGV. Add an early return when impl_ is nullptr to prevent the crash. Fixes: https://github.com/nodejs/node/issues/62057 Co-Authored-By: Claude --- src/quic/session.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/quic/session.cc b/src/quic/session.cc index 39ffad3e09faa8..fa523ebfc8a5bc 100644 --- a/src/quic/session.cc +++ b/src/quic/session.cc @@ -2232,6 +2232,7 @@ void Session::ExtendOffset(size_t amount) { } void Session::UpdateDataStats() { + if (!impl_) return; Debug(this, "Updating data stats"); auto& stats_ = impl_->stats_; ngtcp2_conn_info info; From da731418eaceae2c554c25e62be4796978cbef0e Mon Sep 17 00:00:00 2001 From: zerone0x Date: Fri, 6 Mar 2026 13:08:23 +0100 Subject: [PATCH 2/3] test: add regression test for quic UpdateDataStats null-deref Exercises the crash path in Session::SendDataStats() where on_exit fires UpdateDataStats() after Destroy() has already reset impl_ to nullptr. Refs: https://github.com/nodejs/node/pull/62126 --- ...-session-update-data-stats-after-close.mjs | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 test/parallel/test-quic-session-update-data-stats-after-close.mjs diff --git a/test/parallel/test-quic-session-update-data-stats-after-close.mjs b/test/parallel/test-quic-session-update-data-stats-after-close.mjs new file mode 100644 index 00000000000000..bba1bb47e2f723 --- /dev/null +++ b/test/parallel/test-quic-session-update-data-stats-after-close.mjs @@ -0,0 +1,54 @@ +// Flags: --experimental-quic --no-warnings +// Regression test for https://github.com/nodejs/node/pull/62126 +// UpdateDataStats() must not crash when called after the session's impl_ has +// been reset to null (i.e. after the session is destroyed). +// +// The crash path is in Session::SendDatagram(): +// auto on_exit = OnScopeLeave([&] { ...; UpdateDataStats(); }); +// If the send encounters an error it calls Close(SILENT) → Destroy() → +// impl_.reset(). The on_exit lambda then fires with impl_ == nullptr. + +import { hasQuic, skip, mustCall } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; + +if (!hasQuic) { + skip('QUIC is not enabled'); +} + +const { listen, connect } = await import('node:quic'); +const { createPrivateKey } = await import('node:crypto'); + +const keys = createPrivateKey(fixtures.readKey('agent1-key.pem')); +const certs = fixtures.readKey('agent1-cert.pem'); + +// Datagrams must be enabled on both sides for sendDatagram() to work. +const kDatagramOptions = { + transportParams: { + maxDatagramFrameSize: 1200n, + }, +}; + +const serverEndpoint = await listen( + mustCall((serverSession) => { + serverSession.opened.then(mustCall(async () => { + // Send a datagram then immediately close. This exercises the + // UpdateDataStats() call that fires via on_exit after SendDatagram — + // the close can race with or precede the stats update, leaving + // impl_ == nullptr. Before the fix this would crash. + serverSession.sendDatagram(Buffer.from('hello')).catch(() => {}); + serverSession.close(); + })); + }), + { keys, certs, ...kDatagramOptions }, +); + +const clientSession = await connect(serverEndpoint.address, kDatagramOptions); +await clientSession.opened; + +// Mirror the race on the client side. +clientSession.sendDatagram(Buffer.from('world')).catch(() => {}); +clientSession.close(); + +await clientSession.closed; +serverEndpoint.close(); +await serverEndpoint.closed; From e55dab1205aef3ef202eb8022e83bafb24d10ed7 Mon Sep 17 00:00:00 2001 From: zerone0x <39543393+zerone0x@users.noreply.github.com> Date: Sun, 8 Mar 2026 12:06:14 +0100 Subject: [PATCH 3/3] =?UTF-8?q?test:=20fix=20quic=20regression=20test=20?= =?UTF-8?q?=E2=80=94=20pass=20ca=20cert=20and=20track=20server=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes: 1. Pass ca: certs to connect() so TLS validation succeeds against the self-signed agent1-cert.pem. Without it the client rejects the server cert with UNABLE_TO_GET_ISSUER_CERT_LOCALLY, the session never fully opens, and 'await clientSession.closed' never settles. 2. Track serverSession.closed via a separate Promise.withResolvers() so the test waits for both sides to cleanly tear down before exiting. Also clarifies in the comment that the exact C++ crash path (packet-allocation failure inside SendDatagram) cannot be triggered from JS; the test exercises the closest reachable scenario and validates the null-impl_ guard introduced in session.cc. Co-Authored-By: Claude --- ...-session-update-data-stats-after-close.mjs | 38 +++++++++++++------ 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/test/parallel/test-quic-session-update-data-stats-after-close.mjs b/test/parallel/test-quic-session-update-data-stats-after-close.mjs index bba1bb47e2f723..d9a80ca4b2ae31 100644 --- a/test/parallel/test-quic-session-update-data-stats-after-close.mjs +++ b/test/parallel/test-quic-session-update-data-stats-after-close.mjs @@ -1,12 +1,19 @@ // Flags: --experimental-quic --no-warnings // Regression test for https://github.com/nodejs/node/pull/62126 -// UpdateDataStats() must not crash when called after the session's impl_ has -// been reset to null (i.e. after the session is destroyed). // -// The crash path is in Session::SendDatagram(): +// Validates that UpdateDataStats() does not crash when called on a session +// whose impl_ has already been reset to null (i.e. after Destroy()). +// +// The crash path in Session::SendDatagram(): // auto on_exit = OnScopeLeave([&] { ...; UpdateDataStats(); }); -// If the send encounters an error it calls Close(SILENT) → Destroy() → -// impl_.reset(). The on_exit lambda then fires with impl_ == nullptr. +// When an internal send error calls Close(SILENT) -> Destroy() -> impl_.reset(), +// the on_exit lambda fires with impl_ == nullptr. The `if (!impl_) return;` +// guard added by this PR prevents the crash. +// +// Note: the precise crash trigger (a packet-allocation failure inside +// SendDatagram) cannot be induced from JS. This test exercises the closest +// reachable scenario: datagrams sent immediately before session close, racing +// the on_exit UpdateDataStats() call against teardown. import { hasQuic, skip, mustCall } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; @@ -21,28 +28,36 @@ const { createPrivateKey } = await import('node:crypto'); const keys = createPrivateKey(fixtures.readKey('agent1-key.pem')); const certs = fixtures.readKey('agent1-cert.pem'); -// Datagrams must be enabled on both sides for sendDatagram() to work. +// Both sides must negotiate datagram support (maxDatagramFrameSize > 0). const kDatagramOptions = { transportParams: { maxDatagramFrameSize: 1200n, }, }; +const serverClosed = Promise.withResolvers(); + const serverEndpoint = await listen( mustCall((serverSession) => { serverSession.opened.then(mustCall(async () => { - // Send a datagram then immediately close. This exercises the - // UpdateDataStats() call that fires via on_exit after SendDatagram — - // the close can race with or precede the stats update, leaving - // impl_ == nullptr. Before the fix this would crash. + // Fire a datagram send then close immediately. This races the + // UpdateDataStats() invoked by on_exit in SendDatagram() against the + // Destroy() triggered by close(), exercising the null-impl_ guard. serverSession.sendDatagram(Buffer.from('hello')).catch(() => {}); serverSession.close(); })); + serverSession.closed.then(mustCall(() => serverClosed.resolve())); }), { keys, certs, ...kDatagramOptions }, ); -const clientSession = await connect(serverEndpoint.address, kDatagramOptions); +// Pass the server certificate as the CA so TLS validation succeeds on the +// client side (agent1-cert.pem is self-signed). +const clientSession = await connect(serverEndpoint.address, { + ca: certs, + ...kDatagramOptions, +}); + await clientSession.opened; // Mirror the race on the client side. @@ -50,5 +65,6 @@ clientSession.sendDatagram(Buffer.from('world')).catch(() => {}); clientSession.close(); await clientSession.closed; +await serverClosed.promise; serverEndpoint.close(); await serverEndpoint.closed;