From c2ae32600d6e32af4d7aa4f15326426c69bb4f0b Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Mon, 5 Feb 2018 11:20:07 +0100 Subject: [PATCH 1/5] src: set thread local env in CreateEnvironment This commit set the Environment as a thread local when CreateEnvironment is called which is currently not being done. This would lead to a segment fault if later node::AtExit is called without specifying the environment parameter. This specific issue was reported by Electron. If I recall correctly, back when this was implemented the motivation was that if embedders have multiple environments per isolate they should be using the AtExit functions that take an environment. This is not the case with Electron which only create a single environment (as far as I know), and if a native module calls AtExit this would lead to the segment fault. I was able to reproduce Electron issue and the provided test simulates it. I was also able to use this patch and verify that it works for the Electron issue as well. Refs: https://github.com/nodejs/node/pull/9163 Refs: https://github.com/electron/electron/issues/11299 --- src/node.cc | 6 ++++++ test/cctest/test_environment.cc | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/node.cc b/src/node.cc index 4e3638b886fcb8..a8ae810f050a1a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4296,12 +4296,18 @@ Environment* CreateEnvironment(IsolateData* isolate_data, HandleScope handle_scope(isolate); Context::Scope context_scope(context); auto env = new Environment(isolate_data, context); + CHECK_EQ(0, uv_key_create(&thread_local_env)); + uv_key_set(&thread_local_env, env); env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); return env; } void FreeEnvironment(Environment* env) { + auto tl_env = static_cast(uv_key_get(&thread_local_env)); + if (tl_env == env) { + uv_key_delete(&thread_local_env); + } delete env; } diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index c559a21fda1530..c5ed4499351a2a 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -33,6 +33,16 @@ TEST_F(EnvironmentTest, AtExitWithEnvironment) { EXPECT_TRUE(called_cb_1); } +TEST_F(EnvironmentTest, AtExitWithoutEnvironment) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv, this}; + + AtExit(at_exit_callback1); // No Environment is passed to AtExit. + RunAtExit(*env); + EXPECT_TRUE(called_cb_1); +} + TEST_F(EnvironmentTest, AtExitWithArgument) { const v8::HandleScope handle_scope(isolate_); const Argv argv; From e047cb051c18e3bde623a9ec920700584f5e2b0e Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 6 Feb 2018 07:23:34 +0100 Subject: [PATCH 2/5] squash: remove this for Env contstructor call --- test/cctest/test_environment.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index c5ed4499351a2a..07170ac267adea 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -36,7 +36,7 @@ TEST_F(EnvironmentTest, AtExitWithEnvironment) { TEST_F(EnvironmentTest, AtExitWithoutEnvironment) { const v8::HandleScope handle_scope(isolate_); const Argv argv; - Env env {handle_scope, argv, this}; + Env env {handle_scope, argv}; AtExit(at_exit_callback1); // No Environment is passed to AtExit. RunAtExit(*env); From 6dee9c8fc27f77ecb913c3a1f0b9546e36ce0d7b Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 13 Feb 2018 15:29:40 +0100 Subject: [PATCH 3/5] squash: move code into Environment to avoid dupl --- src/env-inl.h | 9 +++++++++ src/env.cc | 5 +++++ src/env.h | 5 +++++ src/node.cc | 14 +------------- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 5643fffb6f8b6a..6de7944a969bbd 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -311,6 +311,10 @@ inline Environment* Environment::GetCurrent( info.Data().template As()->Value()); } +inline Environment* Environment::GetThreadLocalEnv() { + return static_cast(uv_key_get(&thread_local_env)); +} + inline Environment::Environment(IsolateData* isolate_data, v8::Local context) : isolate_(context->GetIsolate()), @@ -369,6 +373,11 @@ inline Environment::~Environment() { ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V + auto tl_env = static_cast(uv_key_get(&thread_local_env)); + if (tl_env == this) { + uv_key_delete(&Environment::thread_local_env); + } + delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; delete[] http_parser_buffer_; diff --git a/src/env.cc b/src/env.cc index 07e37498c898bb..0575442b7adb11 100644 --- a/src/env.cc +++ b/src/env.cc @@ -147,6 +147,9 @@ void Environment::Start(int argc, SetupProcessObject(this, argc, argv, exec_argc, exec_argv); LoadAsyncWrapperInfo(this); + + CHECK_EQ(0, uv_key_create(&thread_local_env)); + uv_key_set(&thread_local_env, this); } void Environment::CleanupHandles() { @@ -471,4 +474,6 @@ void Environment::AsyncHooks::grow_async_ids_stack() { async_ids_stack_.GetJSArray()).FromJust(); } +uv_key_t Environment::thread_local_env = {}; + } // namespace node diff --git a/src/env.h b/src/env.h index 95548c0900ea13..18a10c5a28e14f 100644 --- a/src/env.h +++ b/src/env.h @@ -371,6 +371,7 @@ struct ContextInfo { bool is_default = false; }; + class Environment { public: class AsyncHooks { @@ -544,6 +545,8 @@ class Environment { static inline Environment* GetCurrent( const v8::PropertyCallbackInfo& info); + static inline Environment* GetThreadLocalEnv(); + inline Environment(IsolateData* isolate_data, v8::Local context); inline ~Environment(); @@ -832,6 +835,8 @@ class Environment { v8::Local promise, v8::Local parent); + static uv_key_t thread_local_env; + #define V(PropertyName, TypeName) \ v8::Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) diff --git a/src/node.cc b/src/node.cc index a8ae810f050a1a..7b9285b8193cf7 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4207,11 +4207,8 @@ uv_loop_t* GetCurrentEventLoop(v8::Isolate* isolate) { } -static uv_key_t thread_local_env; - - void AtExit(void (*cb)(void* arg), void* arg) { - auto env = static_cast(uv_key_get(&thread_local_env)); + auto env = Environment::GetThreadLocalEnv(); AtExit(env, cb, arg); } @@ -4296,18 +4293,12 @@ Environment* CreateEnvironment(IsolateData* isolate_data, HandleScope handle_scope(isolate); Context::Scope context_scope(context); auto env = new Environment(isolate_data, context); - CHECK_EQ(0, uv_key_create(&thread_local_env)); - uv_key_set(&thread_local_env, env); env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); return env; } void FreeEnvironment(Environment* env) { - auto tl_env = static_cast(uv_key_get(&thread_local_env)); - if (tl_env == env) { - uv_key_delete(&thread_local_env); - } delete env; } @@ -4348,8 +4339,6 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, Local context = NewContext(isolate); Context::Scope context_scope(context); Environment env(isolate_data, context); - CHECK_EQ(0, uv_key_create(&thread_local_env)); - uv_key_set(&thread_local_env, &env); env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); const char* path = argc > 1 ? argv[1] : nullptr; @@ -4399,7 +4388,6 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, const int exit_code = EmitExit(&env); RunAtExit(&env); - uv_key_delete(&thread_local_env); v8_platform.DrainVMTasks(isolate); v8_platform.CancelVMTasks(isolate); From 3bceaeeb4d7cccc1f6914872e9663a1c151d04da Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 15 Feb 2018 11:56:20 +0100 Subject: [PATCH 4/5] squash: use uv_once_t to create the thread local --- src/env-inl.h | 5 ----- src/env.cc | 8 +++++++- src/env.h | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 6de7944a969bbd..1e64a0fda5f6e9 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -373,11 +373,6 @@ inline Environment::~Environment() { ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V - auto tl_env = static_cast(uv_key_get(&thread_local_env)); - if (tl_env == this) { - uv_key_delete(&Environment::thread_local_env); - } - delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; delete[] http_parser_buffer_; diff --git a/src/env.cc b/src/env.cc index 0575442b7adb11..cb1f4a231b5481 100644 --- a/src/env.cc +++ b/src/env.cc @@ -80,6 +80,11 @@ v8::CpuProfiler* IsolateData::GetCpuProfiler() { return cpu_profiler_; } + +void InitThreadLocalOnce() { + CHECK_EQ(0, uv_key_create(&Environment::thread_local_env)); +} + void Environment::Start(int argc, const char* const* argv, int exec_argc, @@ -148,7 +153,8 @@ void Environment::Start(int argc, SetupProcessObject(this, argc, argv, exec_argc, exec_argv); LoadAsyncWrapperInfo(this); - CHECK_EQ(0, uv_key_create(&thread_local_env)); + static uv_once_t init_once = UV_ONCE_INIT; + uv_once(&init_once, InitThreadLocalOnce); uv_key_set(&thread_local_env, this); } diff --git a/src/env.h b/src/env.h index 18a10c5a28e14f..6000c635605c95 100644 --- a/src/env.h +++ b/src/env.h @@ -545,6 +545,7 @@ class Environment { static inline Environment* GetCurrent( const v8::PropertyCallbackInfo& info); + static uv_key_t thread_local_env; static inline Environment* GetThreadLocalEnv(); inline Environment(IsolateData* isolate_data, v8::Local context); @@ -835,7 +836,6 @@ class Environment { v8::Local promise, v8::Local parent); - static uv_key_t thread_local_env; #define V(PropertyName, TypeName) \ v8::Persistent PropertyName ## _; From b5761a7ba3064add5220df7ed3775f605369bc02 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Fri, 16 Feb 2018 05:55:44 +0100 Subject: [PATCH 5/5] squash: remove white spaces added --- src/env.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/env.h b/src/env.h index 6000c635605c95..90313e6ef680c9 100644 --- a/src/env.h +++ b/src/env.h @@ -371,7 +371,6 @@ struct ContextInfo { bool is_default = false; }; - class Environment { public: class AsyncHooks { @@ -836,7 +835,6 @@ class Environment { v8::Local promise, v8::Local parent); - #define V(PropertyName, TypeName) \ v8::Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)