Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions src/quic/session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
70 changes: 70 additions & 0 deletions test/parallel/test-quic-session-update-data-stats-after-close.mjs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't crash on main for me, it just gets killed for a non-settling promise. With NODE_DEBUG=quic:

QUIC 662671: endpoint created
QUIC 662671: endpoint listening as a server
QUIC 662671: endpoint created
QUIC 662671: endpoint connecting as a client
QUIC 662671: session created
QUIC 662671: new server session callback QuicEndpoint { ... } Session { ... }
QUIC 662671: session created
QUIC 662671: session handshake callback localhost h3 TLS_AES_128_GCM_SHA256 TLSv1.3 unable to get local issuer certificate UNABLE_TO_GET_ISSUER_CERT_LOCALLY
QUIC 662671: gracefully closing the session
Warning: Detected unsettled top-level await at file:///tmp/node/test/parallel/test-quic-session-update-data-stats-after-close.mjs:52
await clientSession.closed;
^

Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Flags: --experimental-quic --no-warnings
// Regression test for https://github.com/nodejs/node/pull/62126
//
// 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(); });
// 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';

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');

// 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 () => {
// 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 },
);

// 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.
clientSession.sendDatagram(Buffer.from('world')).catch(() => {});
clientSession.close();

await clientSession.closed;
await serverClosed.promise;
serverEndpoint.close();
await serverEndpoint.closed;