n-api: ensure in-module exceptions are propagated

Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: #19437 Backport-PR-URL: #19447 PR-URL: #19537 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · Apr 16, 2018 · e6ccdfb · e6ccdfb
1 parent c6d0a66
commit e6ccdfb
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 43 deletions.
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -168,6 +168,24 @@ struct napi_env__ {
     (out) = v8::type::New((buffer), (byte_offset), (length));                  \
   } while (0)
 
+#define NAPI_CALL_INTO_MODULE(env, call, handle_exception)                   \
+  do {                                                                       \
+    int open_handle_scopes = (env)->open_handle_scopes;                      \
+    int open_callback_scopes = (env)->open_callback_scopes;                  \
+    napi_clear_last_error((env));                                            \
+    call;                                                                    \
+    CHECK_EQ((env)->open_handle_scopes, open_handle_scopes);                 \
+    CHECK_EQ((env)->open_callback_scopes, open_callback_scopes);             \
+    if (!(env)->last_exception.IsEmpty()) {                                  \
+      handle_exception(                                                      \
+          v8::Local<v8::Value>::New((env)->isolate, (env)->last_exception)); \
+      (env)->last_exception.Reset();                                         \
+    }                                                                        \
+  } while (0)
+
+#define NAPI_CALL_INTO_MODULE_THROW(env, call) \
+  NAPI_CALL_INTO_MODULE((env), call, (env)->isolate->ThrowException)
+
 namespace v8impl {
 
 // convert from n-api property attributes to v8::PropertyAttribute
@@ -349,10 +367,11 @@ class Finalizer {
   static void FinalizeBufferCallback(char* data, void* hint) {
     Finalizer* finalizer = static_cast<Finalizer*>(hint);
     if (finalizer->_finalize_callback != nullptr) {
-      finalizer->_finalize_callback(
-        finalizer->_env,
-        data,
-        finalizer->_finalize_hint);
+      NAPI_CALL_INTO_MODULE_THROW(finalizer->_env,
+        finalizer->_finalize_callback(
+          finalizer->_env,
+          data,
+          finalizer->_finalize_hint));
     }
 
     Delete(finalizer);
@@ -464,10 +483,11 @@ class Reference : private Finalizer {
     bool delete_self = reference->_delete_self;
 
     if (reference->_finalize_callback != nullptr) {
-      reference->_finalize_callback(
-          reference->_env,
-          reference->_finalize_data,
-          reference->_finalize_hint);
+      NAPI_CALL_INTO_MODULE_THROW(reference->_env,
+        reference->_finalize_callback(
+            reference->_env,
+            reference->_finalize_data,
+            reference->_finalize_hint));
     }
 
     if (delete_self) {
@@ -552,32 +572,17 @@ class CallbackWrapperBase : public CallbackWrapper {
     napi_callback cb = reinterpret_cast<napi_callback>(
         v8::Local<v8::External>::Cast(
             _cbdata->GetInternalField(kInternalFieldIndex))->Value());
-    v8::Isolate* isolate = _cbinfo.GetIsolate();
 
     napi_env env = static_cast<napi_env>(
         v8::Local<v8::External>::Cast(
             _cbdata->GetInternalField(kEnvIndex))->Value());
 
-    // Make sure any errors encountered last time we were in N-API are gone.
-    napi_clear_last_error(env);
-
-    int open_handle_scopes = env->open_handle_scopes;
-    int open_callback_scopes = env->open_callback_scopes;
-
-    napi_value result = cb(env, cbinfo_wrapper);
+    napi_value result;
+    NAPI_CALL_INTO_MODULE_THROW(env, result = cb(env, cbinfo_wrapper));
 
     if (result != nullptr) {
       this->SetReturnValue(result);
     }
-
-    CHECK_EQ(env->open_handle_scopes, open_handle_scopes);
-    CHECK_EQ(env->open_callback_scopes, open_callback_scopes);
-
-    if (!env->last_exception.IsEmpty()) {
-      isolate->ThrowException(
-          v8::Local<v8::Value>::New(isolate, env->last_exception));
-      env->last_exception.Reset();
-    }
   }
 
   const Info& _cbinfo;
@@ -885,8 +890,10 @@ void napi_module_register_cb(v8::Local<v8::Object> exports,
   // one is found.
   napi_env env = v8impl::GetEnv(context);
 
-  napi_value _exports =
-      mod->nm_register_func(env, v8impl::JsValueFromV8LocalValue(exports));
+  napi_value _exports;
+  NAPI_CALL_INTO_MODULE_THROW(env,
+      _exports = mod->nm_register_func(env,
+          v8impl::JsValueFromV8LocalValue(exports)));
 
   // If register function returned a non-null exports object different from
   // the exports object we passed it, set that as the "exports" property of
@@ -3465,19 +3472,17 @@ class Work : public node::AsyncResource {
       v8::HandleScope scope(env->isolate);
       CallbackScope callback_scope(work);
 
-      work->_complete(env, ConvertUVErrorCode(status), work->_data);
+      NAPI_CALL_INTO_MODULE(env,
+          work->_complete(env, ConvertUVErrorCode(status), work->_data),
+          [env] (v8::Local<v8::Value> local_err) {
+            // If there was an unhandled exception in the complete callback,
+            // report it as a fatal exception. (There is no JavaScript on the
+            // callstack that can possibly handle it.)
+            v8impl::trigger_fatal_exception(env, local_err);
+          });
 
       // Note: Don't access `work` after this point because it was
       // likely deleted by the complete callback.
-
-      // If there was an unhandled exception in the complete callback,
-      // report it as a fatal exception. (There is no JavaScript on the
-      // callstack that can possibly handle it.)
-      if (!env->last_exception.IsEmpty()) {
-        v8::Local<v8::Value> local_err = v8::Local<v8::Value>::New(
-          env->isolate, env->last_exception);
-        v8impl::trigger_fatal_exception(env, local_err);
-      }
     }
   }
 

diff --git a/test/addons-napi/test_exception/test.js b/test/addons-napi/test_exception/test.js
@@ -1,10 +1,26 @@
 'use strict';
+// Flags: --expose-gc
 
 const common = require('../../common');
-const test_exception = require(`./build/${common.buildType}/test_exception`);
 const assert = require('assert');
 const theError = new Error('Some error');
 
+// The test module throws an error during Init, but in order for its exports to
+// not be lost, it attaches them to the error's "bindings" property. This way,
+// we can make sure that exceptions thrown during the module initialization
+// phase are propagated through require() into JavaScript.
+// https://github.com/nodejs/node/issues/19437
+const test_exception = (function() {
+  let resultingException;
+  try {
+    require(`./build/${common.buildType}/test_exception`);
+  } catch (anException) {
+    resultingException = anException;
+  }
+  assert.strictEqual(resultingException.message, 'Error during Init');
+  return resultingException.binding;
+})();
+
 {
   const throwTheError = () => { throw theError; };
 
@@ -50,3 +66,15 @@ const theError = new Error('Some error');
                      'Exception state did not remain clear as expected,' +
                      ` .wasPending() returned ${exception_pending}`);
 }
+
+// Make sure that exceptions that occur during finalization are propagated.
+function testFinalize(binding) {
+  let x = test_exception[binding]();
+  x = null;
+  assert.throws(() => { global.gc(); }, /Error during Finalize/);
+
+  // To assuage the linter's concerns.
+  (function() {})(x);
+}
+testFinalize('createExternal');
+testFinalize('createExternalBuffer');
diff --git a/test/addons-napi/test_exception/test_exception.c b/test/addons-napi/test_exception/test_exception.c
@@ -3,7 +3,7 @@
 
 static bool exceptionWasPending = false;
 
-napi_value returnException(napi_env env, napi_callback_info info) {
+static napi_value returnException(napi_env env, napi_callback_info info) {
   size_t argc = 1;
   napi_value args[1];
   NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
@@ -22,7 +22,7 @@ napi_value returnException(napi_env env, napi_callback_info info) {
   return NULL;
 }
 
-napi_value allowException(napi_env env, napi_callback_info info) {
+static napi_value allowException(napi_env env, napi_callback_info info) {
   size_t argc = 1;
   napi_value args[1];
   NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
@@ -38,23 +38,55 @@ napi_value allowException(napi_env env, napi_callback_info info) {
   return NULL;
 }
 
-napi_value wasPending(napi_env env, napi_callback_info info) {
+static napi_value wasPending(napi_env env, napi_callback_info info) {
   napi_value result;
   NAPI_CALL(env, napi_get_boolean(env, exceptionWasPending, &result));
 
   return result;
 }
 
-napi_value Init(napi_env env, napi_value exports) {
+static void finalizer(napi_env env, void *data, void *hint) {
+  NAPI_CALL_RETURN_VOID(env,
+      napi_throw_error(env, NULL, "Error during Finalize"));
+}
+
+static napi_value createExternal(napi_env env, napi_callback_info info) {
+  napi_value external;
+
+  NAPI_CALL(env,
+      napi_create_external(env, NULL, finalizer, NULL, &external));
+
+  return external;
+}
+
+static char buffer_data[12];
+
+static napi_value createExternalBuffer(napi_env env, napi_callback_info info) {
+  napi_value buffer;
+  NAPI_CALL(env, napi_create_external_buffer(env, sizeof(buffer_data),
+      buffer_data, finalizer, NULL, &buffer));
+  return buffer;
+}
+
+static napi_value Init(napi_env env, napi_value exports) {
   napi_property_descriptor descriptors[] = {
     DECLARE_NAPI_PROPERTY("returnException", returnException),
     DECLARE_NAPI_PROPERTY("allowException", allowException),
     DECLARE_NAPI_PROPERTY("wasPending", wasPending),
+    DECLARE_NAPI_PROPERTY("createExternal", createExternal),
+    DECLARE_NAPI_PROPERTY("createExternalBuffer", createExternalBuffer),
   };
-
   NAPI_CALL(env, napi_define_properties(
       env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));
 
+  napi_value error, code, message;
+  NAPI_CALL(env, napi_create_string_utf8(env, "Error during Init",
+      NAPI_AUTO_LENGTH, &message));
+  NAPI_CALL(env, napi_create_string_utf8(env, "", NAPI_AUTO_LENGTH, &code));
+  NAPI_CALL(env, napi_create_error(env, code, message, &error));
+  NAPI_CALL(env, napi_set_named_property(env, error, "binding", exports));
+  NAPI_CALL(env, napi_throw(env, error));
+
   return exports;
 }