Skip to content

Commit

Permalink
n-api: ensure in-module exceptions are propagated
Browse files Browse the repository at this point in the history 
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: nodejs#19437
PR-URL: nodejs#19537
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
Gabriel Schulhof committed Apr 12, 2018
1 parent c33daa4 commit 65b06de
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 43 deletions.
78 changes: 41 additions & 37 deletions src/node_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,23 @@ 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 {
namespace v8impl {
Expand Down Expand Up @@ -352,10 +369,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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
}

Expand Down
30 changes: 29 additions & 1 deletion test/addons-napi/test_exception/test.js
View file
Original file line number Diff line number Diff line change
@@ -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; };

Expand Down Expand Up @@ -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');
42 changes: 37 additions & 5 deletions test/addons-napi/test_exception/test_exception.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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;
}

Expand Down

0 comments on commit 65b06de

Please sign in to comment.