Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: set thread local env in CreateEnvironment #18573

Closed
wants to merge 5 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Feb 5, 2018

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: #9163
Refs: electron/electron#11299

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 5, 2018
@danbev
Copy link
Contributor Author

danbev commented Feb 5, 2018

@danbev
Copy link
Contributor Author

danbev commented Feb 5, 2018

Steps to reproduce the issue
$ ./script/bootstrap.py -v
$ ./script/build.py -c D
$ npm install tinynative
$ ./out/D/Electron.app/Contents/MacOS/Electron -i
> var tinynative = require('tinynative');
Segmentation fault: 11

$ pushd vendor/node
$ cat << HERE > at_exit.patch
diff --git a/src/node.cc b/src/node.cc
index 2a18582aeb..5f0b2904b9 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -4768,12 +4768,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;
 }

HERE
$ git apply at_exit.patch
$ popd
$ ./script/build.py -c D
ninja: Entering directory `out/D'
[13/13] STAMP Electron.app
$ ./out/D/Electron.app/Contents/MacOS/Electron -i
> var tinynative = require('tinynative');
undefined
> tinynative("hello there");
tinynative: hello there
undefined
>

@danbev
Copy link
Contributor Author

danbev commented Feb 5, 2018

node-test-commit-windows-fanned failure looks unrelated

console output:

not ok 514 sequential/test-http2-settings-flood # TODO : Fix flaky test
  ---
  duration_ms: 120.231
  severity: fail
  stack: |-
    timeout
  ...
node-test-commit failure looks unrelated

console output:

  cc '-D_REENTRANT' '-DPURIFY' '-DOPENSSL_NO_COMP' '-DOPENSSL_NO_SSL3' '-DOPENSSL_NO_HEARTBEATS' '-DOPENSSL_NO_HW' '-DENGINESDIR="/dev/null"' '-DTERMIOS' '-DOPENSSLDIR="/etc/ssl"' '-DL_ENDIAN' '-DOPENSSL_CPUID_OBJ' '-DSHA1_ASM' '-DSHA256_ASM' '-DSHA512_ASM' '-DDSO_DLFCN' '-DHAVE_DLFCN_H' -I../deps/openssl -I../deps/openssl/openssl -I../deps/openssl/openssl/crypto -I../deps/openssl/openssl/crypto/asn1 -I../deps/openssl/openssl/crypto/evp -I../deps/openssl/openssl/crypto/md2 -I../deps/openssl/openssl/crypto/modes -I../deps/openssl/openssl/crypto/store -I../deps/openssl/openssl/include  -Wno-missing-field-initializers -pthread -Wall -Wextra -Wno-unused-parameter -O3 -fno-omit-frame-pointer  -MMD -MF /home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/.deps//home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/openssl/deps/openssl/openssl/crypto/x509v3/v3err.o.d.raw   -c -o /home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/openssl/deps/openssl/openssl/crypto/x509v3/v3err.o ../deps/openssl/openssl/crypto/x509v3/v3err.c
../deps/openssl/openssl/crypto/x509v3/v3_purp.c: In function ‘X509_check_akid’:
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:844:7: error: expected expression before ‘if’
   !   (     if&*gen->type == GEN_DIRNAME) {
       ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:844:27: error: expected ‘;’ before ‘{’ token
   !   (     if&*gen->type == GEN_DIRNAME) {
                           ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:845:7: error: stray ‘\240’ in program
       �       $ nm = gen->d.dipn;
       ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:847:7: error: stray ‘`’ in program
       `     }
       ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:849:13: error: expected expression before ‘.’ token
         if (.m && X508_NAME_cmp(nm, X509_get_iswue2_name(issuer)))
             ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:2: error: stray ‘\240’ in program
  �          return X509_V_ERR_AKI��ISS�ER_SERIAL_MISEATcH;
  ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:2: error: stray ‘\304’ in program
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:2: error: stray ‘\177’ in program
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:11: error: ‘X509_V_ERR_AKI’ undeclared (first use in this function)
  �          return X509_V_ERR_AKI��ISS�ER_SERIAL_MISEATcH;
           ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:11: note: each undeclared identifier is reported only once for each function it appears in
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:27: error: expected ‘;’ before ‘ISS’
  �          return X509_V_ERR_AKI��ISS�ER_SERIAL_MISEATcH;
                           ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:850:27: error: stray ‘\25’ in program
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:839:20: warning: unused variable ‘nm’ [-Wunused-variable]
         X509_NAME *nm = NULL;
                    ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:838:23: warning: variable ‘gen’ set but not used [-Wunused-but-set-variable]
         GENERAL_NAME *gen;
                       ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:852:12: error: ‘X509_VOOK’ undeclared (first use in this function)
     return X509_VOOK;*}
            ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:852:23: error: expected expression before ‘}’ token
     return X509_VOOK;*}
                       ^
../deps/openssl/openssl/crypto/x509v3/v3_purp.c:852:23: warning: control reaches end of non-void function [-Wreturn-type]
     return X509_VOOK;*}
                       ^
deps/openssl/openssl.target.mk:771: recipe for target '/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/openssl/deps/openssl/openssl/crypto/x509v3/v3_purp.o' failed
make[2]: *** [/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64_odroid_c2/out/Release/obj.target/openssl/deps/openssl/openssl/crypto/x509v3/v3_purp.o] Error 1

src/node.cc Outdated
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this to Environment::Start()? That way you don't duplicate it here and in node::Start().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll take a look. Thanks

src/node.cc Outdated
auto tl_env = static_cast<Environment*>(uv_key_get(&thread_local_env));
if (tl_env == env) {
uv_key_delete(&thread_local_env);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this can probably go into ~Environment().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good too, I'll take a look at doing this. Thanks

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
@danbev danbev force-pushed the at_exit_create_environment_fix branch from d598dcf to 6dee9c8 Compare February 13, 2018 14:31
@danbev
Copy link
Contributor Author

danbev commented Feb 14, 2018

@danbev
Copy link
Contributor Author

danbev commented Feb 14, 2018

${STATUS_LABEL} test failure looks unrelated

console output:

01:31:15 Also:   hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from 138.68.241.115/138.68.241.115:49110
01:31:15 		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1693)
01:31:15 		at hudson.remoting.UserResponse.retrieve(UserRequest.java:310)
01:31:15 		at hudson.remoting.Channel.call(Channel.java:908)
01:31:15 		at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
01:31:15 		at com.sun.proxy.$Proxy80.withRepository(Unknown Source)
01:31:15 		at org.jenkinsci.plugins.gitclient.RemoteGitImpl.withRepository(RemoteGitImpl.java:235)
01:31:15 		at hudson.plugins.git.GitSCM.printCommitMessageToLog(GitSCM.java:1245)
01:31:15 		at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1218)
01:31:15 		at hudson.scm.SCM.checkout(SCM.java:495)
01:31:15 		at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
01:31:15 		at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
01:31:15 		at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
01:31:15 		at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
01:31:15 		at hudson.model.Run.execute(Run.java:1724)
01:31:15 		at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
01:31:15 		at hudson.model.ResourceController.execute(ResourceController.java:97)
01:31:15 		at hudson.model.Executor.run(Executor.java:421)
01:31:15 java.lang.NoClassDefFoundError: Could not initialize class jenkins.model.Jenkins$MasterComputer
01:31:15 	at org.jenkinsci.plugins.gitclient.AbstractGitAPIImpl.withRepository(AbstractGitAPIImpl.java:29)
01:31:15 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.withRepository(CliGitAPIImpl.java:72)
01:31:15 	at sun.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
01:31:15 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
01:31:15 	at java.lang.reflect.Method.invoke(Method.java:498)
01:31:15 	at hudson.remoting.RemoteInvocationHandler$RPCRequest.perform(RemoteInvocationHandler.java:922)
01:31:15 	at hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:896)
01:31:15 	at hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:853)
01:31:15 	at hudson.remoting.UserRequest.perform(UserRequest.java:207)
01:31:15 	at hudson.remoting.UserRequest.perform(UserRequest.java:53)
01:31:15 	at hudson.remoting.Request$2.run(Request.java:358)
01:31:15 	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:72)
01:31:15 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
01:31:15 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
01:31:15 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
01:31:15 	at hudson.remoting.Engine$1$1.run(Engine.java:98)
01:31:15 	at java.lang.Thread.run(Thread.java:748)
01:31:15 Caused: java.io.IOException: Remote call on JNLP4-connect connection from 138.68.241.115/138.68.241.115:49110 failed
01:31:15 	at hudson.remoting.Channel.call(Channel.java:916)
01:31:15 	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
01:31:15 	at com.sun.proxy.$Proxy80.withRepository(Unknown Source)
01:31:15 	at org.jenkinsci.plugins.gitclient.RemoteGitImpl.withRepository(RemoteGitImpl.java:235)
01:31:15 	at hudson.plugins.git.GitSCM.printCommitMessageToLog(GitSCM.java:1245)
01:31:15 	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1218)
01:31:15 	at hudson.scm.SCM.checkout(SCM.java:495)
01:31:15 	at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
01:31:15 	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
01:31:15 	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
01:31:15 	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
01:31:15 	at hudson.model.Run.execute(Run.java:1724)
01:31:15 	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
01:31:15 	at hudson.model.ResourceController.execute(ResourceController.java:97)
01:31:15 	at hudson.model.Executor.run(Executor.java:421)
01:31:15 Run condition [Always] enabling perform for step [[]]
01:31:15 Run condition [Always] enabling perform for step [[]]
01:31:15 TAP Reports Processing: START
01:31:15 Looking for TAP results report in workspace using pattern: *.tap
01:31:15 Saving reports...
01:31:15 Processing '/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/cctest.tap'
01:31:15 Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/cctest.tap].
01:31:15 Processing '/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-base.tap'
01:31:15 Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-base.tap].
01:31:16 Processing '/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-on.tap'
01:31:16 Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-on.tap].
01:31:16 TAP Reports Processing: FINISH
01:31:16 Checking ^not ok
01:31:16 Notifying upstream projects of job completion
01:31:16 Finished: FAILURE
node-test-commit failure looks unrelated

console output:

01:31:15 Also:   hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from 138.68.241.115/138.68.241.115:49110
01:31:15 		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1693)
01:31:15 		at hudson.remoting.UserResponse.retrieve(UserRequest.java:310)
01:31:15 		at hudson.remoting.Channel.call(Channel.java:908)
01:31:15 		at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
01:31:15 		at com.sun.proxy.$Proxy80.withRepository(Unknown Source)
01:31:15 		at org.jenkinsci.plugins.gitclient.RemoteGitImpl.withRepository(RemoteGitImpl.java:235)
01:31:15 		at hudson.plugins.git.GitSCM.printCommitMessageToLog(GitSCM.java:1245)
01:31:15 		at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1218)
01:31:15 		at hudson.scm.SCM.checkout(SCM.java:495)
01:31:15 		at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
01:31:15 		at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
01:31:15 		at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
01:31:15 		at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
01:31:15 		at hudson.model.Run.execute(Run.java:1724)
01:31:15 		at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
01:31:15 		at hudson.model.ResourceController.execute(ResourceController.java:97)
01:31:15 		at hudson.model.Executor.run(Executor.java:421)
01:31:15 java.lang.NoClassDefFoundError: Could not initialize class jenkins.model.Jenkins$MasterComputer
01:31:15 	at org.jenkinsci.plugins.gitclient.AbstractGitAPIImpl.withRepository(AbstractGitAPIImpl.java:29)
01:31:15 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.withRepository(CliGitAPIImpl.java:72)
01:31:15 	at sun.reflect.GeneratedMethodAccessor54.invoke(Unknown Source)
01:31:15 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
01:31:15 	at java.lang.reflect.Method.invoke(Method.java:498)
01:31:15 	at hudson.remoting.RemoteInvocationHandler$RPCRequest.perform(RemoteInvocationHandler.java:922)
01:31:15 	at hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:896)
01:31:15 	at hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:853)
01:31:15 	at hudson.remoting.UserRequest.perform(UserRequest.java:207)
01:31:15 	at hudson.remoting.UserRequest.perform(UserRequest.java:53)
01:31:15 	at hudson.remoting.Request$2.run(Request.java:358)
01:31:15 	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:72)
01:31:15 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
01:31:15 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
01:31:15 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
01:31:15 	at hudson.remoting.Engine$1$1.run(Engine.java:98)
01:31:15 	at java.lang.Thread.run(Thread.java:748)
01:31:15 Caused: java.io.IOException: Remote call on JNLP4-connect connection from 138.68.241.115/138.68.241.115:49110 failed
01:31:15 	at hudson.remoting.Channel.call(Channel.java:916)
01:31:15 	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
01:31:15 	at com.sun.proxy.$Proxy80.withRepository(Unknown Source)
01:31:15 	at org.jenkinsci.plugins.gitclient.RemoteGitImpl.withRepository(RemoteGitImpl.java:235)
01:31:15 	at hudson.plugins.git.GitSCM.printCommitMessageToLog(GitSCM.java:1245)
01:31:15 	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1218)
01:31:15 	at hudson.scm.SCM.checkout(SCM.java:495)
01:31:15 	at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
01:31:15 	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
01:31:15 	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
01:31:15 	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
01:31:15 	at hudson.model.Run.execute(Run.java:1724)
01:31:15 	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
01:31:15 	at hudson.model.ResourceController.execute(ResourceController.java:97)
01:31:15 	at hudson.model.Executor.run(Executor.java:421)
01:31:15 Run condition [Always] enabling perform for step [[]]
01:31:15 Run condition [Always] enabling perform for step [[]]
01:31:15 TAP Reports Processing: START
01:31:15 Looking for TAP results report in workspace using pattern: *.tap
01:31:15 Saving reports...
01:31:15 Processing '/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/cctest.tap'
01:31:15 Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/cctest.tap].
01:31:15 Processing '/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-base.tap'
01:31:15 Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-base.tap].
01:31:16 Processing '/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-on.tap'
01:31:16 Parsing TAP test result [/var/lib/jenkins/jobs/node-test-commit-linux-containered/configurations/axis-nodes/ubuntu1604_sharedlibs_fips20_x64/builds/2240/tap-master-files/test-fips-on.tap].
01:31:16 TAP Reports Processing: FINISH
01:31:16 Checking ^not ok
01:31:16 Notifying upstream projects of job completion
01:31:16 Finished: FAILURE
node-test-commit-windows-fanned failure looks unrelated

console output:

c:\workspace\node-test-binary-windows>TASKKILL /F /IM node.exe /T   || TRUE
ERROR: The process "node.exe" not found.

c:\workspace\node-test-binary-windows>TASKKILL /F /IM cctest.exe /T   || TRUE
ERROR: The process "cctest.exe" not found.

c:\workspace\node-test-binary-windows>TASKKILL /F /IM run-tests.exe /T   || TRUE
ERROR: The process "run-tests.exe" not found.

c:\workspace\node-test-binary-windows>TASKKILL /F /IM yes.exe /T   || TRUE
ERROR: The process "yes.exe" not found.

c:\workspace\node-test-binary-windows>rm -rfv test/tmp*   || true

c:\workspace\node-test-binary-windows>if not exist test.tap (
echo "Missing test.tap file"  
 set "EXIT_RETURN_VALUE=1" 
) 

c:\workspace\node-test-binary-windows>if not exist cctest.tap (
echo "Missing cctest.tap file"  
 set "EXIT_RETURN_VALUE=1" 
) 
"Missing cctest.tap file"

c:\workspace\node-test-binary-windows>rm -vf ../test.tap   || true

c:\workspace\node-test-binary-windows>rm -vf ../cctest.tap   || true

c:\workspace\node-test-binary-windows>mv test.tap ../test.tap   || true

c:\workspace\node-test-binary-windows>mv cctest.tap ../cctest.tap   || true
mv: cannot stat 'cctest.tap': No such file or directory

@danbev
Copy link
Contributor Author

danbev commented Feb 15, 2018

@bnoordhuis Sorry about the delay on this. When you get a chance could you take a look at the latest commit and see if this is what you hand in mind? Thanks

src/env.cc Outdated
@@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uv_key_create() and uv_key_delete() should be called only once, not for every environment. (The current initialization logic is probably wrong too.)

What should work is using a uv_once_t to protect the call to uv_key_create() and simply don't bother with uv_key_delete().

src/env.h Outdated
@@ -371,6 +371,7 @@ struct ContextInfo {
bool is_default = false;
};


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you undo the whitespace change here and on line 839?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed them and will remove them now. Thanks

@danbev
Copy link
Contributor Author

danbev commented Feb 16, 2018

@danbev
Copy link
Contributor Author

danbev commented Feb 16, 2018

node-test-commit failure looks unrelated

console output:

not ok 1999 sequential/test-fs-readfile-tostring-fail
  ---
  duration_ms: 15.381
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 9)
  ...

@danbev
Copy link
Contributor Author

danbev commented Feb 16, 2018

Landed in 42c14c5.

@danbev danbev closed this Feb 16, 2018
danbev added a commit to danbev/node that referenced this pull request Feb 16, 2018
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.

PR-URL: nodejs#18573
Refs: nodejs#9163
Refs: electron/electron#11299
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@danbev danbev deleted the at_exit_create_environment_fix branch February 16, 2018 06:28
@MylesBorins
Copy link
Contributor

This is failing to compile on v9.x. Should we backport?

../test/cctest/test_environment.cc:81:7: error: no matching constructor for initialization of 'EnvironmentTest::Env'
  Env env {handle_scope, argv};
      ^   ~~~~~~~~~~~~~~~~~~~~
../test/cctest/test_environment.cc:25:9: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
  class Env {
        ^
../test/cctest/test_environment.cc:27:5: note: candidate constructor not viable: requires 4 arguments, but 2 were provided
    Env(const v8::HandleScope& handle_scope,
    ^
1 error generated.
make[1]: *** [/Users/mborins/code/node/v9.x/out/Release/obj.target/cctest/test/cctest/test_environment.o] Error 1

@danbev
Copy link
Contributor Author

danbev commented Feb 23, 2018

@MylesBorins There was an earlier commit, ca473be, that remove this parameter. If that is merged first this should land cleanly. Should I open a backport request for ca473be?

@MylesBorins
Copy link
Contributor

@danbev it seems that c64e130 is the one that doesn't land cleanly. A backport of #18558 would be solid.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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.

PR-URL: nodejs#18573
Refs: nodejs#9163
Refs: electron/electron#11299
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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.

PR-URL: nodejs#18573
Refs: nodejs#9163
Refs: electron/electron#11299
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants