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;