quic: release stream arenas before cleanup

Project: nodejs/node | Language: C++/JavaScript | Concepts: memory-management, error-handling, testing-strategy | Difficulty: senior
Link: https://github.com/nodejs/node/pull/65410

The problem

QUIC streams in Node.js were releasing their arena-allocated state (used for stats and stream state) too late, specifically in the C++ Stream destructor. During process or realm teardown, the QUIC binding data could be destroyed before all streams are destructed, leading to potential use-after-free bugs or crashes if a stream's destructor tried to access already-freed arena memory. Additionally, certain HTTP/3 header callbacks could synchronously destroy their stream, making it unsafe to access arena-backed state after such callbacks.

Why it's non-trivial

A naive fix might just move the arena release to an earlier point, but:

• Streams can be destroyed synchronously from JavaScript callbacks, so the C++ code must not touch arena-backed state after such a callback.
• The code must be robust against repeated destruction attempts and must not double-free arena slots.
• The fix must not break existing tests or introduce regressions in other QUIC/HTTP3 behaviors.
• The solution must be tested, including edge cases like process teardown and invalid HTTP/3 streams.

The approach

1. Avoid accessing arena state after possible synchronous destruction

After emitting headers (which can call into JS and synchronously destroy the stream), the code now checks if the stream is destroyed before continuing:

+    // 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;

Annotation:
This prevents use-after-free by ensuring the stream's memory is not accessed if it was destroyed during a JS callback.

2. Move arena slot release to explicit Destroy() method

Previously, arena slots were released in the Stream destructor. Now, they're released earlier, in the Destroy() method, and the destructor is defaulted:

-Stream::~Stream() {
-  // Make sure that Destroy() was called before Stream is actually destructed.
-  DCHECK_NE(stats()->destroyed_at, 0);
-
-  // Release arena slots back to the freelist.
-  auto& binding = BindingData::Get(env());
-  if (stats_slot_) {
-    GetStreamStatsArena(binding).ReleaseSlot(stats_slot_);
-    stats_slot_ = 0;
-  }
-  if (state_slot_) {
-    GetStreamStateArena(binding).ReleaseSlot(state_slot_);
-    state_slot_ = 0;
-  }
-}
+Stream::~Stream() = default;

Annotation:
The destructor no longer does cleanup; it's now the responsibility of Destroy() to free arena slots.

The new ReleaseArenaSlots() method encapsulates the arena slot release logic:

+void Stream::ReleaseArenaSlots() {
+  auto& binding = BindingData::Get(env());
+  if (stats_slot_) {
+    GetStreamStatsArena(binding).ReleaseSlot(stats_slot_);
+    stats_slot_ = 0;
+  }
+  if (state_slot_) {
+    GetStreamStateArena(binding).ReleaseSlot(state_slot_);
+    state_slot_ = 0;
+  }
+}

Annotation:
This ensures that arena slots are released exactly once, and only when the stream is destroyed.

3. Make repeated destruction safe and robust

The Destroy() method now checks is_destroyed() (which also checks if the slots are already released):

-void Stream::Destroy(QuicError error) {
-  if (stats()->destroyed_at != 0) return;
+void Stream::Destroy(QuicError error) {
+  if (is_destroyed()) return;

Annotation:
This prevents double-destruction and double-free bugs.

4. Test for regression and teardown safety

A new regression test was added to ensure no crash occurs when destroying endpoints after handling an invalid unidirectional stream:

+const stream = await session.createUnidirectionalStream();
+stream.writer.writeSync('x');
+
+endpoint.destroy();
+await endpoint.closed;

Annotation:
This test simulates the teardown scenario that previously could crash Node.js.

From the review thread

No notable review discussion on this PR.

Takeaways

• Release resources as early as possible, not in destructors, when dependencies may already be gone.
  Relying on destructors for cleanup is dangerous if other teardown logic may have already released shared resources.

• Always check for object destruction after callbacks that can synchronously destroy the object.
  Especially when calling into user code or JS from C++, assume the object may have been deleted.

• Make cleanup functions idempotent and safe for repeated calls.
  Defensive checks (like is_destroyed()) prevent double-free and other memory errors.