src: keep per-Environment state out of thread_locals

src/api/environment.cc

@@ -1048,6 +1048,7 @@ Maybe<void> InitializePrimordials(Local<Context> context,
   // in the first place. However, creating BuiltinLoader instances is
   // relatively cheap and all the scripts that we may want to run at
   // startup are always present in it.
+  // NOLINTNEXTLINE(runtime/thread_local)
   thread_local builtins::BuiltinLoader builtin_loader;
   // Primordials can always be just eagerly compiled.
   builtin_loader.SetEagerCompile();

src/crypto/crypto_context.cc

@@ -95,37 +95,27 @@ struct X509Less {
 };
 using X509Set = std::set<ncrypto::X509Pointer, X509Less>;
 
-// Per-thread root cert store. See NewRootCertStore() on what it contains.
-static thread_local DeleteFnPtr<X509_STORE, X509_STORE_free> root_cert_store;
-// If the user calls tls.setDefaultCACertificates() this will be used
-// to hold the user-provided certificates, the root_cert_store and any new
-// copy generated by NewRootCertStore() will then contain the certificates
-// from this set.
-static thread_local std::unique_ptr<X509Set> root_certs_from_users;
-static thread_local bool has_cleanup_hook = false;
-
-static void CleanupRootCertStore(void*) {
-  root_cert_store.reset();
-  root_certs_from_users.reset();
-  has_cleanup_hook = false;
-}
-
-static void EnsureRootCertStoreCleanupHook(Environment* env) {
-  if (env == nullptr || has_cleanup_hook) {
-    return;
-  }
+struct RootCertStore {
+  // See NewRootCertStore() on what it contains.
+  DeleteFnPtr<X509_STORE, X509_STORE_free> store;
+  // Set by tls.setDefaultCACertificates(). Once set, NewRootCertStore()
+  // copies these certificates instead of loading the defaults.
+  std::unique_ptr<X509Set> certs_from_users;
+};
 
-  env->AddCleanupHook(CleanupRootCertStore, nullptr);
-  has_cleanup_hook = true;
+void FreeRootCertStore(RootCertStore* root_certs) {
+  delete root_certs;
+}
+
+static RootCertStore* GetRootCertStore(Environment* env) {
+  if (!env->root_cert_store) env->root_cert_store.reset(new RootCertStore());
+  return env->root_cert_store.get();
 }
 
 X509_STORE* GetOrCreateRootCertStore(Environment* env) {
-  EnsureRootCertStoreCleanupHook(env);
-  if (root_cert_store != nullptr) {
-    return root_cert_store.get();
-  }
-  root_cert_store.reset(NewRootCertStore(env));
-  return root_cert_store.get();
+  RootCertStore* root_certs = GetRootCertStore(env);
+  if (!root_certs->store) root_certs->store.reset(NewRootCertStore(env));
+  return root_certs->store.get();
 }
 
 // Takes a string or buffer and loads it into a BIO.
@@ -1062,8 +1052,10 @@ X509_STORE* NewRootCertStore(Environment* env) {
   // If the root cert store is already reset by users through
   // tls.setDefaultCACertificates(), just create a copy from the
   // user-provided certificates.
-  if (root_certs_from_users != nullptr) {
-    for (const auto& cert : *root_certs_from_users) {
+  const X509Set* certs_from_users =
+      env != nullptr ? GetRootCertStore(env)->certs_from_users.get() : nullptr;
+  if (certs_from_users != nullptr) {
+    for (const auto& cert : *certs_from_users) {
       CHECK_EQ(1, X509_STORE_add_cert(store, cert.get()));
     }
     return store;
@@ -1230,12 +1222,13 @@ MaybeLocal<Array> X509sToArrayOfStrings(Environment* env,
 
 void GetUserRootCertificates(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK_NOT_NULL(root_certs_from_users);
+  const auto& certs_from_users = GetRootCertStore(env)->certs_from_users;
+  CHECK(certs_from_users);
   Local<Array> results;
   if (X509sToArrayOfStrings(env,
-                            root_certs_from_users->begin(),
-                            root_certs_from_users->end(),
-                            root_certs_from_users->size())
+                            certs_from_users->begin(),
+                            certs_from_users->end(),
+                            certs_from_users->size())
           .ToLocal(&results)) {
     args.GetReturnValue().Set(results);
   }
@@ -1246,12 +1239,12 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsArray());
   Local<Array> cert_array = args[0].As<Array>();
   Environment* env = Environment::GetCurrent(context);
-  EnsureRootCertStoreCleanupHook(env);
+  RootCertStore* root_certs = GetRootCertStore(env);
 
   if (cert_array->Length() == 0) {
     // If the array is empty, just clear the user certs and reset the store.
-    root_cert_store.reset();
-    root_certs_from_users = std::make_unique<X509Set>();
+    root_certs->store.reset();
+    root_certs->certs_from_users = std::make_unique<X509Set>();
     return;
   }
 
@@ -1263,7 +1256,6 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
   }
 
   if (certs->empty()) {
-    Environment* env = Environment::GetCurrent(context);
     return THROW_ERR_CRYPTO_OPERATION_FAILED(
         env, "No valid certificates found in the provided array");
   }
@@ -1275,11 +1267,11 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
     // is not consumed by insert (element already exists).
   }
 
-  root_certs_from_users = std::move(new_set);
+  root_certs->certs_from_users = std::move(new_set);
 
-  // Reset the global root cert store so it will be recreated with the
-  // new certificates.
-  root_cert_store.reset();
+  // Reset the root cert store so it will be recreated with the new
+  // certificates.
+  root_certs->store.reset();
 }
 
 void GetSystemCACertificates(const FunctionCallbackInfo<Value>& args) {

src/env.cc

@@ -1472,6 +1472,7 @@ void Environment::ClosePerEnvHandles() {
   close_and_finish(reinterpret_cast<uv_handle_t*>(&task_queues_async_));
 }
 
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local int handle_cleanup_depth = 0;
 
 void Environment::CleanupHandles() {

src/env.h

@@ -76,6 +76,13 @@ class MacCache;
 
 namespace node {
 
+#if HAVE_OPENSSL
+namespace crypto {
+struct RootCertStore;
+void FreeRootCertStore(RootCertStore* root_certs);
+}  // namespace crypto
+#endif  // HAVE_OPENSSL
+
 namespace shadow_realm {
 class ShadowRealm;
 }
@@ -1220,6 +1227,7 @@ class Environment final : public MemoryRetainer {
   std::unique_ptr<ncrypto::MacCache> provider_mac_cache;
   std::vector<std::string> supported_mac_algorithms;
   bool supported_mac_algorithms_initialized = false;
+  DeleteFnPtr<crypto::RootCertStore, crypto::FreeRootCertStore> root_cert_store;
 #endif  // HAVE_OPENSSL
 
   v8::Global<v8::Module> temporary_required_module_facade_original;

src/node_binding.cc

@@ -180,6 +180,7 @@ struct dl_wrap {
 static Mutex dlhandles_mutex;
 static std::unordered_set<dl_wrap*, dl_wrap::hash, dl_wrap::equal>
     dlhandles;
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local std::string dlerror_storage;
 
 char* wrapped_dlerror() {
@@ -286,6 +287,7 @@ using v8::Value;
 // Globals per process
 static node_module* modlist_internal;
 static node_module* modlist_linked;
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local node_module* thread_local_modpending;
 
 // This is set by node::Init() which is used by embedders

src/node_debug.cc

@@ -24,8 +24,10 @@ using v8::Number;
 using v8::Object;
 using v8::Value;
 
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local std::unordered_map<FastStringKey, int, FastStringKey::Hash>
     generic_usage_counters;
+// NOLINTNEXTLINE(runtime/thread_local)
 thread_local std::unordered_map<FastStringKey, int, FastStringKey::Hash>
     v8_fast_api_call_counts;
 

src/node_errors.cc

@@ -190,9 +190,11 @@ static std::string GetErrorSource(Isolate* isolate,
 }
 
 static std::atomic<bool> is_in_oom{false};
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local std::atomic<bool> is_retrieving_js_stacktrace{false};
 // This is thread-local because it only guards re-entrancy within the current
 // thread's uncaught-exception path; no cross-thread synchronization is needed.
+// NOLINTNEXTLINE(runtime/thread_local)
 static thread_local bool is_in_uncaught_exception = false;
 MaybeLocal<StackTrace> GetCurrentStackTrace(Isolate* isolate, int frame_count) {
   if (isolate == nullptr) {

src/node_internals.h

@@ -286,6 +286,7 @@ class InternalCallbackScope {
 // Non-zero while an Environment on this thread is closing its handles with JS
 // disallowed isolate-wide; InternalCallbackScope re-allows it for the other
 // Environments whose callbacks run in those loop turns.
+// NOLINTNEXTLINE(runtime/thread_local)
 extern thread_local int handle_cleanup_depth;
 
 class DebugSealHandleScope {

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();