quic: give each BindingData its own allocator state

The ngtcp2 and nghttp3 allocators shared one thread_local state whose
BindingData pointer was set by the last BindingData that handed out an
allocator. With several Environments on a thread, memory allocated for
a session in one Environment was accounted against another's
BindingData and failed a CHECK when freed.

Give each BindingData its own heap-allocated state. nghttp3 buffers
backing external strings can be freed after the BindingData is gone,
so the state counts live allocations and is deleted once the
BindingData has been destroyed and the last of them is freed.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: #66411
Refs: #66239
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Author: codebytere

src/quic/README.md

@@ -150,28 +150,30 @@ The Application is selected as soon as the ALPN protocol is known:
 immediately for clients, and for servers from the `OnClientHello` TLS
 callback (see [Server handshake ordering](#server-handshake-ordering)).
 
-### Thread-Local Allocator
+### Allocator
 
 Both ngtcp2 and nghttp3 require custom allocators (`ngtcp2_mem`,
 `nghttp3_mem`). These allocator structs must outlive every object they
 create. Some nghttp3 objects (notably `rcbuf`s backing V8 external strings)
 can survive past `BindingData` destruction during isolate teardown.
 
-The solution uses `thread_local` storage:
+Each `BindingData` owns a heap-allocated `QuicAllocState` that holds both
+allocator structs and counts live allocations:
 
 ```cpp
 struct QuicAllocState {
-    BindingData* binding = nullptr;  // Nulled in ~BindingData
+    BindingData* binding;  // Nulled in ~BindingData
+    size_t live_allocations = 0;
     ngtcp2_mem ngtcp2;
     nghttp3_mem nghttp3;
 };
-thread_local QuicAllocState quic_alloc_state;
 ```
 
 Each allocation prepends its size before the returned pointer. This allows
 `free` and `realloc` to report correct sizes for memory tracking. When
 `binding` is null (after `BindingData` destruction), allocations still
-succeed but memory tracking is silently skipped.
+succeed but memory tracking is silently skipped. The state is deleted once
+`binding` is null and the last allocation has been freed.
 
 ## Session Lifecycle
 

src/quic/bindingdata.cc

@@ -36,33 +36,30 @@ using v8::Value;
 namespace quic {
 
 // ============================================================================
-// Thread-local QUIC allocator.
+// QUIC allocator.
 //
-// Both ngtcp2 and nghttp3 take an allocator struct (ngtcp2_mem /
-// nghttp3_mem) whose pointer is stored inside every object they
-// allocate. Some of those objects — notably nghttp3 rcbufs backing
-// V8 external strings — can outlive the BindingData that created them
-// (freed during V8 isolate teardown, after Environment cleanup).
-//
-// To handle this safely, both allocators live in a thread-local static
-// struct that is never destroyed. Memory tracking goes through the
-// BindingData pointer when it is alive and is silently skipped during
-// teardown (after ~BindingData nulls the pointer).
+// ngtcp2 and nghttp3 keep a pointer to their allocator struct in every object
+// they allocate, and nghttp3 rcbufs backing V8 external strings can be freed
+// after the BindingData is gone. A QuicAllocState is therefore deleted only
+// once its BindingData has been destroyed and its last allocation freed.
 //
 // The allocation functions use the same prepended-size-header scheme as
 // NgLibMemoryManager (node_mem-inl.h) so that frees always know the
 // allocation size regardless of whether BindingData is still around.
 
-namespace {
 struct QuicAllocState {
-  BindingData* binding = nullptr;
+  BindingData* binding;
+  size_t live_allocations = 0;
   ngtcp2_mem ngtcp2 = {};
   nghttp3_mem nghttp3 = {};
+
+  void OnFreed() {
+    CHECK_GT(live_allocations, 0);
+    if (--live_allocations == 0 && binding == nullptr) delete this;
+  }
 };
-thread_local QuicAllocState quic_alloc_state;
 
-// Core allocation functions shared by both ngtcp2 and nghttp3.
-// user_data always points to the thread-local QuicAllocState.
+namespace {
 
 void* QuicRealloc(void* ptr, size_t size, void* user_data) {
   auto* state = static_cast<QuicAllocState*>(user_data);
@@ -77,6 +74,10 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) {
     previous_size = *reinterpret_cast<size_t*>(original_ptr);
     if (previous_size == 0) {
       char* ret = UncheckedRealloc(original_ptr, size);
+      if (size == 0) {
+        state->OnFreed();
+        return nullptr;
+      }
       if (ret != nullptr) ret += kReserveSizeAndAlign;
       return ret;
     }
@@ -95,6 +96,7 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) {
       state->binding->env()->external_memory_accounter()->Update(
           state->binding->env()->isolate(), new_size);
     }
+    if (ptr == nullptr) state->live_allocations++;
     *reinterpret_cast<size_t*>(mem) = size;
     mem += kReserveSizeAndAlign;
   } else if (size == 0) {
@@ -103,6 +105,7 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) {
       state->binding->env()->external_memory_accounter()->Decrease(
           state->binding->env()->isolate(), previous_size);
     }
+    if (ptr != nullptr) state->OnFreed();
   }
   return mem;
 }
@@ -231,35 +234,20 @@ BindingData& BindingData::Get(Environment* env) {
 }
 
 BindingData::~BindingData() {
-  quic_alloc_state.binding = nullptr;
+  alloc_state_->binding = nullptr;
+  if (alloc_state_->live_allocations == 0) delete alloc_state_;
   // flush_check_ is cleaned up by ~CheckWrapHandle() after the destructor
   // body completes. The inner CheckWrap (and its uv_check_t) will be freed
   // later by the uv_close callback, after CleanupHandles() runs uv_run().
   pending_flush_sessions_.clear();
 }
 
 ngtcp2_mem* BindingData::ngtcp2_allocator() {
-  quic_alloc_state.binding = this;
-  quic_alloc_state.ngtcp2 = {
-      &quic_alloc_state,
-      Ngtcp2Malloc,
-      Ngtcp2Free,
-      Ngtcp2Calloc,
-      Ngtcp2Realloc,
-  };
-  return &quic_alloc_state.ngtcp2;
+  return &alloc_state_->ngtcp2;
 }
 
 nghttp3_mem* BindingData::nghttp3_allocator() {
-  quic_alloc_state.binding = this;
-  quic_alloc_state.nghttp3 = {
-      &quic_alloc_state,
-      Nghttp3Malloc,
-      Nghttp3Free,
-      Nghttp3Calloc,
-      Nghttp3Realloc,
-  };
-  return &quic_alloc_state.nghttp3;
+  return &alloc_state_->nghttp3;
 }
 
 void BindingData::CheckAllocatedSize(size_t previous_size) const {
@@ -348,7 +336,12 @@ JS_METHOD_IMPL(BindingData::SetHeadersInterest) {
 
 BindingData::BindingData(Realm* realm, Local<Object> object)
     : BaseObject(realm, object),
+      alloc_state_(new QuicAllocState{this}),
       flush_check_(env(), [this]() { OnFlushCheck(); }) {
+  alloc_state_->ngtcp2 = {
+      alloc_state_, Ngtcp2Malloc, Ngtcp2Free, Ngtcp2Calloc, Ngtcp2Realloc};
+  alloc_state_->nghttp3 = {
+      alloc_state_, Nghttp3Malloc, Nghttp3Free, Nghttp3Calloc, Nghttp3Realloc};
   MakeWeak();
   // Unref so the check handle doesn't keep the event loop alive on its own.
   flush_check_.Unref();

src/quic/bindingdata.h

@@ -24,6 +24,7 @@ class Endpoint;
 class Packet;
 class Session;
 class SessionManager;
+struct QuicAllocState;
 
 // ============================================================================
 
@@ -281,15 +282,12 @@ class BindingData final
 
   // NgLibMemoryManager — the base class provides CheckAllocatedSize,
   // IncreaseAllocatedSize, DecreaseAllocatedSize, and StopTrackingMemory.
-  // Actual allocations go through the thread-local allocators below.
+  // Actual allocations go through the allocators below.
   void CheckAllocatedSize(size_t previous_size) const;
   void IncreaseAllocatedSize(size_t size);
   void DecreaseAllocatedSize(size_t size);
 
-  // Thread-local allocators that outlive BindingData destruction.
-  // Both ngtcp2 and nghttp3 store the allocator pointer inside every
-  // object they allocate; some of those objects (e.g., nghttp3 rcbufs
-  // backing V8 external strings) can be freed after BindingData is gone.
+  // The allocators can outlive the BindingData; see QuicAllocState.
   ngtcp2_mem* ngtcp2_allocator();
   nghttp3_mem* nghttp3_allocator();
 
@@ -384,6 +382,8 @@ class BindingData final
   ArenaPtr endpoint_state_arena_{nullptr, +[](void*) {}};
   ArenaPtr endpoint_stats_arena_{nullptr, +[](void*) {}};
 
+  QuicAllocState* alloc_state_;
+
   // Deferred send flush state. The CheckWrapHandle fires immediately after
   // the I/O poll phase in the same event loop tick, allowing batched
   // receive processing: all packets are read during poll, then

test/cctest/test_environment_shared_isolate.cc

@@ -8,10 +8,15 @@
 #include "cppgc/garbage-collected.h"
 #include "env-inl.h"
 #include "node_test_fixture.h"
+#include "quic/guard.h"
 #include "v8-cppgc.h"
 #if HAVE_OPENSSL
 #include "crypto/crypto_context.h"
 #endif
+#ifndef OPENSSL_NO_QUIC
+#include "node_realm-inl.h"
+#include "quic/bindingdata.h"
+#endif
 
 #include <string>
 #include <vector>
@@ -480,6 +485,42 @@ TEST_P(SharedIsolateTest, RootCertStoreIsPerEnvironment) {
 }
 #endif  // HAVE_OPENSSL
 
+#ifndef OPENSSL_NO_QUIC
+TEST_P(SharedIsolateTest, QuicAllocatorIsPerEnvironment) {
+  const HandleScope handle_scope(isolate_);
+  std::unique_ptr<Instance> first =
+      CreateInstance(0, EnvironmentFlags::kNoCreateInspector);
+  std::unique_ptr<Instance> second =
+      CreateInstance(1, EnvironmentFlags::kNoCreateInspector);
+  auto allocator = [this](Instance* instance) {
+    HandleScope inner(isolate_);
+    Local<Context> context = instance->context.Get(isolate_);
+    Context::Scope context_scope(context);
+    Local<Value> name = v8::String::NewFromUtf8Literal(isolate_, "quic");
+    instance->env->principal_realm()
+        ->internal_binding_loader()
+        ->Call(context, v8::Undefined(isolate_), 1, &name)
+        .ToLocalChecked();
+    return node::quic::BindingData::Get(instance->env).ngtcp2_allocator();
+  };
+
+  ngtcp2_mem* first_mem = allocator(first.get());
+  void* first_ptr = first_mem->malloc(64, first_mem->user_data);
+  ngtcp2_mem* second_mem = allocator(second.get());
+  void* tracked = second_mem->malloc(16, second_mem->user_data);
+  void* untracked = second_mem->malloc(16, second_mem->user_data);
+  node::quic::BindingData::Get(second->env).StopTrackingMemory(untracked);
+  EXPECT_NE(first_mem, second_mem);
+  first_mem->free(first_ptr, first_mem->user_data);
+
+  FreeInstance(std::move(second));
+  second_mem->free(untracked, second_mem->user_data);
+  tracked = second_mem->realloc(tracked, 32, second_mem->user_data);
+  second_mem->free(tracked, second_mem->user_data);
+  FreeInstance(std::move(first));
+}
+#endif  // OPENSSL_NO_QUIC
+
 INSTANTIATE_TEST_SUITE_P(
     EnvironmentTest,
     SharedIsolateTest,