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: #19265 PR-URL: #19537 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Author: Gabriel Schulhof

src/node_api.cc

@@ -173,6 +173,23 @@ struct napi_env__{
  (out) = v8::type::New((buffer), (byte_offset), (length)); \
  } while (0)
 
+#defineNAPI_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)
+
+#defineNAPI_CALL_INTO_MODULE_THROW(env, call) \
+NAPI_CALL_INTO_MODULE((env), call, (env)->isolate->ThrowException)
 
 namespace{
 namespacev8impl{
@@ -352,10 +369,11 @@ class Finalizer{
 staticvoidFinalizeBufferCallback(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);
@@ -467,10 +485,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){
@@ -555,32 +574,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;
@@ -888,8 +892,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
@@ -3384,19 +3390,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);
- }
  }
  }
 

test/addons-napi/test_exception/test.js

@@ -1,10 +1,26 @@
 'use strict';
+// Flags: --expose-gc
 
 constcommon=require('../../common');
-consttest_exception=require(`./build/${common.buildType}/test_exception`);
 constassert=require('assert');
 consttheError=newError('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
+consttest_exception=(function(){
+letresultingException;
+try{
+require(`./build/${common.buildType}/test_exception`);
+}catch(anException){
+resultingException=anException;
+}
+assert.strictEqual(resultingException.message,'Error during Init');
+returnresultingException.binding;
+})();
+
 {
 constthrowTheError=()=>{throwtheError;};
 
@@ -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.
+functiontestFinalize(binding){
+letx=test_exception[binding]();
+x=null;
+assert.throws(()=>{global.gc();},/ErrorduringFinalize/);
+
+// To assuage the linter's concerns.
+(function(){})(x);
+}
+testFinalize('createExternal');
+testFinalize('createExternalBuffer');

test/addons-napi/test_exception/test_exception.c

@@ -3,7 +3,7 @@
 
 staticboolexceptionWasPending= false;
 
-napi_valuereturnException(napi_envenv, napi_callback_infoinfo){
+staticnapi_valuereturnException(napi_envenv, napi_callback_infoinfo){
 size_targc=1;
 napi_valueargs[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){
 returnNULL;
 }
 
-napi_valueallowException(napi_envenv, napi_callback_infoinfo){
+staticnapi_valueallowException(napi_envenv, napi_callback_infoinfo){
 size_targc=1;
 napi_valueargs[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){
 returnNULL;
 }
 
-napi_valuewasPending(napi_envenv, napi_callback_infoinfo){
+staticnapi_valuewasPending(napi_envenv, napi_callback_infoinfo){
 napi_valueresult;
 NAPI_CALL(env, napi_get_boolean(env, exceptionWasPending, &result));
 
 returnresult;
 }
 
-napi_valueInit(napi_envenv, napi_valueexports){
+staticvoidfinalizer(napi_envenv, void*data, void*hint){
+NAPI_CALL_RETURN_VOID(env,
+napi_throw_error(env, NULL, "Error during Finalize"));
+}
+
+staticnapi_valuecreateExternal(napi_envenv, napi_callback_infoinfo){
+napi_valueexternal;
+
+NAPI_CALL(env,
+napi_create_external(env, NULL, finalizer, NULL, &external));
+
+returnexternal;
+}
+
+staticcharbuffer_data[12];
+
+staticnapi_valuecreateExternalBuffer(napi_envenv, napi_callback_infoinfo){
+napi_valuebuffer;
+NAPI_CALL(env, napi_create_external_buffer(env, sizeof(buffer_data),
+buffer_data, finalizer, NULL, &buffer));
+returnbuffer;
+}
+
+staticnapi_valueInit(napi_envenv, napi_valueexports){
 napi_property_descriptordescriptors[] ={
 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_valueerror, 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));
+
 returnexports;
 }