diff --git a/src/api/environment.cc b/src/api/environment.cc index ee9da42b606b..b8963b0fbcad 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -1048,6 +1048,7 @@ Maybe InitializePrimordials(Local 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(); diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 823d87e7f204..d314d15cc542 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -95,37 +95,27 @@ struct X509Less { }; using X509Set = std::set; -// Per-thread root cert store. See NewRootCertStore() on what it contains. -static thread_local DeleteFnPtr 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 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 store; + // Set by tls.setDefaultCACertificates(). Once set, NewRootCertStore() + // copies these certificates instead of loading the defaults. + std::unique_ptr 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 X509sToArrayOfStrings(Environment* env, void GetUserRootCertificates(const FunctionCallbackInfo& 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 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& args) { CHECK(args[0]->IsArray()); Local cert_array = args[0].As(); 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(); + root_certs->store.reset(); + root_certs->certs_from_users = std::make_unique(); return; } @@ -1263,7 +1256,6 @@ void ResetRootCertStore(const FunctionCallbackInfo& 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& 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& args) { diff --git a/src/env.cc b/src/env.cc index e96b6e6bb129..2da45b82f1a2 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1472,6 +1472,7 @@ void Environment::ClosePerEnvHandles() { close_and_finish(reinterpret_cast(&task_queues_async_)); } +// NOLINTNEXTLINE(runtime/thread_local) thread_local int handle_cleanup_depth = 0; void Environment::CleanupHandles() { diff --git a/src/env.h b/src/env.h index 75f3a66eda7b..4aadd63536e8 100644 --- a/src/env.h +++ b/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 provider_mac_cache; std::vector supported_mac_algorithms; bool supported_mac_algorithms_initialized = false; + DeleteFnPtr root_cert_store; #endif // HAVE_OPENSSL v8::Global temporary_required_module_facade_original; diff --git a/src/node_binding.cc b/src/node_binding.cc index 568325e8496a..8cb5578eb048 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -180,6 +180,7 @@ struct dl_wrap { static Mutex dlhandles_mutex; static std::unordered_set 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 diff --git a/src/node_debug.cc b/src/node_debug.cc index c995e791f252..8efb049a76fc 100644 --- a/src/node_debug.cc +++ b/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 generic_usage_counters; +// NOLINTNEXTLINE(runtime/thread_local) thread_local std::unordered_map v8_fast_api_call_counts; diff --git a/src/node_errors.cc b/src/node_errors.cc index cf000047d38d..cf71884c88db 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -190,9 +190,11 @@ static std::string GetErrorSource(Isolate* isolate, } static std::atomic is_in_oom{false}; +// NOLINTNEXTLINE(runtime/thread_local) static thread_local std::atomic 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 GetCurrentStackTrace(Isolate* isolate, int frame_count) { if (isolate == nullptr) { diff --git a/src/node_internals.h b/src/node_internals.h index 1c4da8e2d203..bb45d73e9090 100644 --- a/src/node_internals.h +++ b/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 { diff --git a/src/quic/README.md b/src/quic/README.md index 8c23ed2f4af9..acea22840a7e 100644 --- a/src/quic/README.md +++ b/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 diff --git a/src/quic/bindingdata.cc b/src/quic/bindingdata.cc index 391172c5979e..e49da50f3f03 100644 --- a/src/quic/bindingdata.cc +++ b/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(user_data); @@ -77,6 +74,10 @@ void* QuicRealloc(void* ptr, size_t size, void* user_data) { previous_size = *reinterpret_cast(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(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,7 +234,8 @@ 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(). @@ -239,27 +243,11 @@ BindingData::~BindingData() { } 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) : 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(); diff --git a/src/quic/bindingdata.h b/src/quic/bindingdata.h index 2ef9f7685314..7d424b9bf6f6 100644 --- a/src/quic/bindingdata.h +++ b/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 diff --git a/src/quic/data.cc b/src/quic/data.cc index fd4c3253432f..f15051da498f 100644 --- a/src/quic/data.cc +++ b/src/quic/data.cc @@ -32,6 +32,7 @@ using v8::Undefined; using v8::Value; namespace quic { +// NOLINTNEXTLINE(runtime/thread_local) thread_local int DebugIndentScope::indent_ = 0; Path::Path(const SocketAddress& local, const SocketAddress& remote) { diff --git a/src/quic/defs.h b/src/quic/defs.h index 45b4c77d1584..2f249b4ee469 100644 --- a/src/quic/defs.h +++ b/src/quic/defs.h @@ -395,6 +395,7 @@ class DebugIndentScope final { } private: + // NOLINTNEXTLINE(runtime/thread_local) static thread_local int indent_; }; diff --git a/test/cctest/test_environment_shared_isolate.cc b/test/cctest/test_environment_shared_isolate.cc index e8787d45c103..3e86063b064c 100644 --- a/test/cctest/test_environment_shared_isolate.cc +++ b/test/cctest/test_environment_shared_isolate.cc @@ -6,8 +6,17 @@ #include "cppgc/allocation.h" #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 #include @@ -446,6 +455,72 @@ TEST_P(SharedIsolateTest, FreeIsolateDataBeforeItsEnvironmentAsserts) { FreeInstance(std::move(instance)); } +#if HAVE_OPENSSL +TEST_P(SharedIsolateTest, RootCertStoreIsPerEnvironment) { + const HandleScope handle_scope(isolate_); + std::unique_ptr first = + CreateInstance(0, EnvironmentFlags::kNoCreateInspector); + std::unique_ptr second = + CreateInstance(1, EnvironmentFlags::kNoCreateInspector); + auto store_size = [](Instance* instance) { + return sk_X509_OBJECT_num(X509_STORE_get0_objects( + node::crypto::GetOrCreateRootCertStore(instance->env))); + }; + auto set_default_ca_count = [this](Instance* instance, int count) { + std::string source = + "const tls = process.getBuiltinModule('tls');" + "tls.setDefaultCACertificates(tls.rootCertificates.slice(0, " + + std::to_string(count) + "))"; + Evaluate(instance, source.c_str()); + }; + + set_default_ca_count(first.get(), 1); + set_default_ca_count(second.get(), 2); + EXPECT_EQ(store_size(first.get()), 1); + EXPECT_EQ(store_size(second.get()), 2); + + FreeInstance(std::move(first)); + EXPECT_EQ(store_size(second.get()), 2); + FreeInstance(std::move(second)); +} +#endif // HAVE_OPENSSL + +#ifndef OPENSSL_NO_QUIC +TEST_P(SharedIsolateTest, QuicAllocatorIsPerEnvironment) { + const HandleScope handle_scope(isolate_); + std::unique_ptr first = + CreateInstance(0, EnvironmentFlags::kNoCreateInspector); + std::unique_ptr second = + CreateInstance(1, EnvironmentFlags::kNoCreateInspector); + auto allocator = [this](Instance* instance) { + HandleScope inner(isolate_); + Local context = instance->context.Get(isolate_); + Context::Scope context_scope(context); + Local 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, diff --git a/tools/cpplint.py b/tools/cpplint.py index 464d95b8824f..1e26e3ded396 100755 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -350,6 +350,7 @@ "runtime/printf_format", "runtime/references", "runtime/string", + "runtime/thread_local", "runtime/threadsafe_fn", "runtime/vlog", "runtime/v8_persistent", @@ -7446,6 +7447,25 @@ def CheckStringValueUsage(filename, lines, error): 'Use node::TwoByteValue instead.') +def CheckThreadLocalUsage(filename, lines, error): + """Logs an error if thread_local is used in src/. + Args: + filename: The name of the current file. + lines: An array of strings, each representing a line of the file. + error: The function to call with any errors found. + """ + if not (filename.startswith('src/') or filename.startswith('src\\')): + return + + for linenum, line in enumerate(lines): + if re.search(r'\bthread_local\b', line.split('//', 1)[0]): + error(filename, linenum, 'runtime/thread_local', 5, + 'Several Environments can share a thread, so keep state that ' + 'belongs to one on the Environment or its BindingData. Mark ' + 'intentionally per-thread state with ' + 'NOLINTNEXTLINE(runtime/thread_local).') + + def ProcessLine( filename, file_extension, @@ -7609,6 +7629,8 @@ def ProcessFileData(filename, file_extension, lines, error, extra_check_function CheckStringValueUsage(filename, lines, error) + CheckThreadLocalUsage(filename, lines, error) + def ProcessConfigOverrides(filename): """Loads the configuration files and processes the config overrides.