From e6ccdfbde3735b4af6049aff31c2ca4f3a4eb369 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 22 Mar 2018 17:01:37 -0400 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/19437 Backport-PR-URL: https://github.com/nodejs/node/pull/19447 PR-URL: https://github.com/nodejs/node/pull/19537 Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_api.cc | 79 ++++++++++--------- test/addons-napi/test_exception/test.js | 30 ++++++- .../test_exception/test_exception.c | 42 ++++++++-- 3 files changed, 108 insertions(+), 43 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index a52ecef27c4c27..e4638e4559af97 100644 --- 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::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(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( v8::Local::Cast( _cbdata->GetInternalField(kInternalFieldIndex))->Value()); - v8::Isolate* isolate = _cbinfo.GetIsolate(); napi_env env = static_cast( v8::Local::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::New(isolate, env->last_exception)); - env->last_exception.Reset(); - } } const Info& _cbinfo; @@ -885,8 +890,10 @@ void napi_module_register_cb(v8::Local 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 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 local_err = v8::Local::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 index 787b7d78b1c72b..b9311add6c92d7 100644 --- 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 index 2ab01b653becfb..c83fd5ebcade19 100644 --- 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; }