quic: guard against null impl_ in UpdateDataStats#62126
quic: guard against null impl_ in UpdateDataStats#62126zerone0x wants to merge 3 commits intonodejs:mainfrom
Conversation
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: nodejs#62057 Co-Authored-By: Claude <noreply@anthropic.com>
addaleax
left a comment
There was a problem hiding this comment.
Can you add a regression test?
Exercises the crash path in Session::SendDataStats() where on_exit fires UpdateDataStats() after Destroy() has already reset impl_ to nullptr. Refs: nodejs#62126
|
added a regression test in |
There was a problem hiding this comment.
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;
^
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 <noreply@anthropic.com>
|
good catch on both — fixed in the latest commit:
on the crash reproduction point: you are right that this does not trigger the actual crash path. the packet-allocation failure inside if a cctest that exercises |
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: #62057