From d7616260a4cdf16f947235755e7973578808750e Mon Sep 17 00:00:00 2001 From: "Kamat, Trivikram" <16024985+trivikr@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:04:27 -0700 Subject: [PATCH] quic: release stream arenas before cleanup Release QUIC stream stats and state arena slots when the stream is destroyed instead of from the Stream destructor. Realm cleanup destroys QUIC binding data before draining remaining BaseObjects, so a stream that survives until process teardown must not need BindingData from its destructor. HTTP/3 header callbacks can synchronously destroy their stream. Check whether the stream was destroyed after those callbacks before accessing its arena-backed state, and make repeated native destruction safe after the slots have been released. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: codex:gpt-5.6-sol --- src/quic/http3.cc | 8 +++++ src/quic/streams.cc | 14 ++++---- src/quic/streams.h | 1 + .../test-quic-h3-uni-stream-teardown.mjs | 34 +++++++++++++++++++ 4 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-quic-h3-uni-stream-teardown.mjs diff --git a/src/quic/http3.cc b/src/quic/http3.cc index b6d876af60f6..24e01e9dfc13 100644 --- a/src/quic/http3.cc +++ b/src/quic/http3.cc @@ -849,6 +849,10 @@ class Http3ApplicationImpl final : public Session::Application { "HTTP/3 application received end of headers for stream %" PRIi64, id); stream->EmitHeaders(); + // EmitHeaders() calls into JavaScript, which can synchronously destroy the + // stream. Its arena-backed state is released by Destroy(), so do not touch + // the stream again if that happened. + if (stream->is_destroyed()) return; if (fin) { // The stream is done. There's no more data to receive! Debug(&session(), "Headers are final for stream %" PRIi64, id); @@ -891,6 +895,10 @@ class Http3ApplicationImpl final : public Session::Application { "HTTP/3 application received end of trailers for stream %" PRIi64, id); stream->EmitHeaders(); + // EmitHeaders() calls into JavaScript, which can synchronously destroy the + // stream. Its arena-backed state is released by Destroy(), so do not touch + // the stream again if that happened. + if (stream->is_destroyed()) return; if (fin) { Debug(&session(), "Trailers are final for stream %" PRIi64, id); Stream::ReceiveDataFlags flags{ diff --git a/src/quic/streams.cc b/src/quic/streams.cc index c2691362447a..4515921fb51e 100644 --- a/src/quic/streams.cc +++ b/src/quic/streams.cc @@ -1211,11 +1211,9 @@ Stream::Stream(BaseObjectWeakPtr session, STAT_SET(Stats, max_offset, params->initial_max_data); } -Stream::~Stream() { - // Make sure that Destroy() was called before Stream is actually destructed. - DCHECK_NE(stats()->destroyed_at, 0); +Stream::~Stream() = default; - // Release arena slots back to the freelist. +void Stream::ReleaseArenaSlots() { auto& binding = BindingData::Get(env()); if (stats_slot_) { GetStreamStatsArena(binding).ReleaseSlot(stats_slot_); @@ -1306,6 +1304,7 @@ bool Stream::is_pending() const { } bool Stream::is_destroyed() const { + if (!stats_slot_) return true; return stats()->destroyed_at != 0; } @@ -1621,7 +1620,7 @@ void Stream::EndReadable(std::optional maybe_final_size) { } void Stream::Destroy(QuicError error) { - if (stats()->destroyed_at != 0) return; + if (is_destroyed()) return; // Record the destroyed at timestamp before notifying the JavaScript side // that the stream is being destroyed. @@ -1666,6 +1665,9 @@ void Stream::Destroy(QuicError error) { // handle. EmitClose(error); + stream_id id_to_remove = id(); + ReleaseArenaSlots(); + auto session = session_; session_.reset(); // EmitClose above triggers MakeCallback which can destroy the session @@ -1673,7 +1675,7 @@ void Stream::Destroy(QuicError error) { // Session BaseObject can be kept alive by a BaseObjectPtr elsewhere, // e.g. OnTimeout's ref) even though impl_ has been reset. We must // check is_destroyed() to avoid dereferencing the null impl_. - if (session && !session->is_destroyed()) session->RemoveStream(id()); + if (session && !session->is_destroyed()) session->RemoveStream(id_to_remove); // Critically, make sure that the RemoveStream call is the last thing // trying to use this stream object. Once that call is made, the stream diff --git a/src/quic/streams.h b/src/quic/streams.h index f18702ae75ab..366694605b4b 100644 --- a/src/quic/streams.h +++ b/src/quic/streams.h @@ -408,6 +408,7 @@ class Stream final : public AsyncWrap, bool is_local_unidirectional() const; bool is_remote_unidirectional() const; + void ReleaseArenaSlots(); // JavaScript callouts diff --git a/test/parallel/test-quic-h3-uni-stream-teardown.mjs b/test/parallel/test-quic-h3-uni-stream-teardown.mjs new file mode 100644 index 000000000000..6f93ef73eb2d --- /dev/null +++ b/test/parallel/test-quic-h3-uni-stream-teardown.mjs @@ -0,0 +1,34 @@ +// Flags: --experimental-quic --no-warnings + +// Regression test for https://github.com/nodejs/node/issues/65408. +// A client-created unidirectional stream is not a valid HTTP/3 request stream, +// but nghttp3 handles it internally. Destroying the endpoint after receiving +// data on that stream must not crash during process teardown. + +import { hasQuic, skip, mustNotCall } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; + +if (!hasQuic) { + skip('QUIC is not enabled'); +} + +const { createPrivateKey } = await import('node:crypto'); +const { listen, connect } = await import('node:quic'); + +const key = createPrivateKey(fixtures.readKey('agent1-key.pem')); +const cert = fixtures.readKey('agent1-cert.pem'); + +const endpoint = await listen(mustNotCall(), { + sni: { '*': { keys: [key], certs: [cert] } }, +}); + +const session = await connect(endpoint.address, { + servername: 'localhost', + verifyPeer: 'manual', +}); + +const stream = await session.createUnidirectionalStream(); +stream.writer.writeSync('x'); + +endpoint.destroy(); +await endpoint.closed;