n-api: do not call into JS when that is not allowed

addaleax · rvagg · commit 1ce5e6398735 · 2019-02-28T23:35:32.000+11:00
Check whether calling into JS is allowed before doing so. PR-URL: #26127 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
@@ -11,6 +11,7 @@ struct napi_env__ {
         context_persistent(isolate, context) {
     CHECK_EQ(isolate, context->GetIsolate());
   }
+  virtual ~napi_env__() {}
   v8::Isolate* const isolate;  // Shortcut for context()->GetIsolate()
   v8impl::Persistent<v8::Context> context_persistent;
 
@@ -21,6 +22,8 @@ struct napi_env__ {
   inline void Ref() { refs++; }
   inline void Unref() { if ( --refs == 0) delete this; }
 
+  virtual bool can_call_into_js() const { return true; }
+
   v8impl::Persistent<v8::Value> last_exception;
   napi_extended_error_info last_error;
   int open_handle_scopes = 0;
@@ -68,11 +71,12 @@ napi_status napi_set_last_error(napi_env env, napi_status error_code,
   RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))
 
 // NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
-#define NAPI_PREAMBLE(env)                                       \
-  CHECK_ENV((env));                                              \
-  RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
-                         napi_pending_exception);                \
-  napi_clear_last_error((env));                                  \
+#define NAPI_PREAMBLE(env)                                          \
+  CHECK_ENV((env));                                                 \
+  RETURN_STATUS_IF_FALSE((env),                                     \
+      (env)->last_exception.IsEmpty() && (env)->can_call_into_js(), \
+      napi_pending_exception);                                      \
+  napi_clear_last_error((env));                                     \
   v8impl::TryCatch try_catch((env))
 
 #define CHECK_TO_TYPE(env, type, context, result, src, status)                \
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -12,9 +12,14 @@ struct node_napi_env__ : public napi_env__ {
       napi_env__(context) {
     CHECK_NOT_NULL(node_env());
   }
+
   inline node::Environment* node_env() const {
     return node::Environment::GetCurrent(context());
   }
+
+  bool can_call_into_js() const override {
+    return node_env()->can_call_into_js();
+  }
 };
 
 typedef node_napi_env__* node_napi_env;
diff --git a/test/node-api/test_worker_terminate/binding.gyp b/test/node-api/test_worker_terminate/binding.gyp
@@ -0,0 +1,8 @@
+{
+  "targets": [
+    {
+      "target_name": "test_worker_terminate",
+      "sources": [ "test_worker_terminate.c" ]
+    }
+  ]
+}
diff --git a/test/node-api/test_worker_terminate/test.js b/test/node-api/test_worker_terminate/test.js
@@ -0,0 +1,23 @@
+'use strict';
+const common = require('../../common');
+const assert = require('assert');
+const { Worker, isMainThread, workerData } = require('worker_threads');
+
+if (isMainThread) {
+  const counter = new Int32Array(new SharedArrayBuffer(4));
+  const worker = new Worker(__filename, { workerData: { counter } });
+  worker.on('exit', common.mustCall(() => {
+    assert.strictEqual(counter[0], 1);
+  }));
+  worker.on('error', common.mustNotCall());
+} else {
+  const { Test } = require(`./build/${common.buildType}/test_worker_terminate`);
+
+  const { counter } = workerData;
+  // Test() tries to call a function twice and asserts that the second call does
+  // not work because of a pending exception.
+  Test(() => {
+    Atomics.add(counter, 0, 1);
+    process.exit();
+  });
+}
diff --git a/test/node-api/test_worker_terminate/test_worker_terminate.c b/test/node-api/test_worker_terminate/test_worker_terminate.c
@@ -0,0 +1,39 @@
+#include <stdio.h>
+#include <node_api.h>
+#include <assert.h>
+#include "../../js-native-api/common.h"
+
+napi_value Test(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value recv;
+  napi_value argv[1];
+  napi_status status;
+
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, &recv, NULL));
+  NAPI_ASSERT(env, argc >= 1, "Not enough arguments, expected 1.");
+
+  napi_valuetype t;
+  NAPI_CALL(env, napi_typeof(env, argv[0], &t));
+  NAPI_ASSERT(env, t == napi_function,
+      "Wrong first argument, function expected.");
+
+  status = napi_call_function(env, recv, argv[0], 0, NULL, NULL);
+  assert(status == napi_ok);
+  status = napi_call_function(env, recv, argv[0], 0, NULL, NULL);
+  assert(status == napi_pending_exception);
+
+  return NULL;
+}
+
+napi_value Init(napi_env env, napi_value exports) {
+  napi_property_descriptor properties[] = {
+    DECLARE_NAPI_PROPERTY("Test", Test)
+  };
+
+  NAPI_CALL(env, napi_define_properties(
+      env, exports, sizeof(properties) / sizeof(*properties), properties));
+
+  return exports;
+}
+
+NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

Original file line number	Diff line number	Diff line change
`@@ -12,9 +12,14 @@ struct node_napi_env__ : public napi_env__ {`
`12`	`12`	`napi_env__(context) {`
`13`	`13`	`CHECK_NOT_NULL(node_env());`
`14`	`14`	`}`
	`15`	`+`
`15`	`16`	`inline node::Environment* node_env() const {`
`16`	`17`	`return node::Environment::GetCurrent(context());`
`17`	`18`	`}`
	`19`	`+`
	`20`	`+ bool can_call_into_js() const override {`
	`21`	`+ return node_env()->can_call_into_js();`
	`22`	`+ }`
`18`	`23`	`};`
`19`	`24`
`20`	`25`	`typedef node_napi_env__* node_napi_env;`