Skip to content

Commit

Permalink
src: set thread local env in CreateEnvironment
Browse files Browse the repository at this point in the history
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: nodejs#9163
Refs: electron/electron#11299
  • Loading branch information
danbev committed Feb 13, 2018
1 parent b6000d8 commit c2ae326
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Environment*>(uv_key_get(&thread_local_env));
if (tl_env == env) {
uv_key_delete(&thread_local_env);
}
delete env;
}

Expand Down
10 changes: 10 additions & 0 deletions test/cctest/test_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit c2ae326

Please sign in to comment.