FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

n-api: ensure in-module exceptions are propagated · nodejs/node@92f699e · GitHub

/ node Public

Commit 92f699e

Browse files
Gabriel Schulhof
authored and
MylesBorins
committed
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 <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1 parent 367113f commit 92f699e

3 files changed

Lines changed: 107 additions & 43 deletions

File tree

‎src/node_api.cc‎

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,23 @@ struct napi_env__ {
173173
(out) = v8::type::New((buffer), (byte_offset), (length)); \
174174
} while (0)
175175

176+
#define NAPI_CALL_INTO_MODULE(env, call, handle_exception) \
177+
do { \
178+
int open_handle_scopes = (env)->open_handle_scopes; \
179+
int open_callback_scopes = (env)->open_callback_scopes; \
180+
napi_clear_last_error((env)); \
181+
call; \
182+
CHECK_EQ((env)->open_handle_scopes, open_handle_scopes); \
183+
CHECK_EQ((env)->open_callback_scopes, open_callback_scopes); \
184+
if (!(env)->last_exception.IsEmpty()) { \
185+
handle_exception( \
186+
v8::Local<v8::Value>::New((env)->isolate, (env)->last_exception)); \
187+
(env)->last_exception.Reset(); \
188+
} \
189+
} while (0)
190+
191+
#define NAPI_CALL_INTO_MODULE_THROW(env, call) \
192+
NAPI_CALL_INTO_MODULE((env), call, (env)->isolate->ThrowException)
176193

177194
namespace {
178195
namespace v8impl {
@@ -352,10 +369,11 @@ class Finalizer {
352369
static void FinalizeBufferCallback(char* data, void* hint) {
353370
Finalizer* finalizer = static_cast<Finalizer*>(hint);
354371
if (finalizer->_finalize_callback != nullptr) {
355-
finalizer->_finalize_callback(
356-
finalizer->_env,
357-
data,
358-
finalizer->_finalize_hint);
372+
NAPI_CALL_INTO_MODULE_THROW(finalizer->_env,
373+
finalizer->_finalize_callback(
374+
finalizer->_env,
375+
data,
376+
finalizer->_finalize_hint));
359377
}
360378

361379
Delete(finalizer);
@@ -467,10 +485,11 @@ class Reference : private Finalizer {
467485
bool delete_self = reference->_delete_self;
468486

469487
if (reference->_finalize_callback != nullptr) {
470-
reference->_finalize_callback(
471-
reference->_env,
472-
reference->_finalize_data,
473-
reference->_finalize_hint);
488+
NAPI_CALL_INTO_MODULE_THROW(reference->_env,
489+
reference->_finalize_callback(
490+
reference->_env,
491+
reference->_finalize_data,
492+
reference->_finalize_hint));
474493
}
475494

476495
if (delete_self) {
@@ -555,32 +574,17 @@ class CallbackWrapperBase : public CallbackWrapper {
555574
napi_callback cb = reinterpret_cast<napi_callback>(
556575
v8::Local<v8::External>::Cast(
557576
_cbdata->GetInternalField(kInternalFieldIndex))->Value());
558-
v8::Isolate* isolate = _cbinfo.GetIsolate();
559577

560578
napi_env env = static_cast<napi_env>(
561579
v8::Local<v8::External>::Cast(
562580
_cbdata->GetInternalField(kEnvIndex))->Value());
563581

564-
// Make sure any errors encountered last time we were in N-API are gone.
565-
napi_clear_last_error(env);
566-
567-
int open_handle_scopes = env->open_handle_scopes;
568-
int open_callback_scopes = env->open_callback_scopes;
569-
570-
napi_value result = cb(env, cbinfo_wrapper);
582+
napi_value result;
583+
NAPI_CALL_INTO_MODULE_THROW(env, result = cb(env, cbinfo_wrapper));
571584

572585
if (result != nullptr) {
573586
this->SetReturnValue(result);
574587
}
575-
576-
CHECK_EQ(env->open_handle_scopes, open_handle_scopes);
577-
CHECK_EQ(env->open_callback_scopes, open_callback_scopes);
578-
579-
if (!env->last_exception.IsEmpty()) {
580-
isolate->ThrowException(
581-
v8::Local<v8::Value>::New(isolate, env->last_exception));
582-
env->last_exception.Reset();
583-
}
584588
}
585589

586590
const Info& _cbinfo;
@@ -888,8 +892,10 @@ void napi_module_register_cb(v8::Local<v8::Object> exports,
888892
// one is found.
889893
napi_env env = v8impl::GetEnv(context);
890894

891-
napi_value _exports =
892-
mod->nm_register_func(env, v8impl::JsValueFromV8LocalValue(exports));
895+
napi_value _exports;
896+
NAPI_CALL_INTO_MODULE_THROW(env,
897+
_exports = mod->nm_register_func(env,
898+
v8impl::JsValueFromV8LocalValue(exports)));
893899

894900
// If register function returned a non-null exports object different from
895901
// the exports object we passed it, set that as the "exports" property of
@@ -3384,19 +3390,17 @@ class Work : public node::AsyncResource {
33843390
v8::HandleScope scope(env->isolate);
33853391
CallbackScope callback_scope(work);
33863392

3387-
work->_complete(env, ConvertUVErrorCode(status), work->_data);
3393+
NAPI_CALL_INTO_MODULE(env,
3394+
work->_complete(env, ConvertUVErrorCode(status), work->_data),
3395+
[env] (v8::Local<v8::Value> local_err) {
3396+
// If there was an unhandled exception in the complete callback,
3397+
// report it as a fatal exception. (There is no JavaScript on the
3398+
// callstack that can possibly handle it.)
3399+
v8impl::trigger_fatal_exception(env, local_err);
3400+
});
33883401

33893402
// Note: Don't access `work` after this point because it was
33903403
// likely deleted by the complete callback.
3391-
3392-
// If there was an unhandled exception in the complete callback,
3393-
// report it as a fatal exception. (There is no JavaScript on the
3394-
// callstack that can possibly handle it.)
3395-
if (!env->last_exception.IsEmpty()) {
3396-
v8::Local<v8::Value> local_err = v8::Local<v8::Value>::New(
3397-
env->isolate, env->last_exception);
3398-
v8impl::trigger_fatal_exception(env, local_err);
3399-
}
34003404
}
34013405
}
34023406

‎test/addons-napi/test_exception/test.js‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,26 @@
11
'use strict';
2+
// Flags: --expose-gc
23

34
const common = require('../../common');
4-
const test_exception = require(`./build/${common.buildType}/test_exception`);
55
const assert = require('assert');
66
const theError = new Error('Some error');
77

8+
// The test module throws an error during Init, but in order for its exports to
9+
// not be lost, it attaches them to the error's "bindings" property. This way,
10+
// we can make sure that exceptions thrown during the module initialization
11+
// phase are propagated through require() into JavaScript.
12+
// https://github.com/nodejs/node/issues/19437
13+
const test_exception = (function() {
14+
let resultingException;
15+
try {
16+
require(`./build/${common.buildType}/test_exception`);
17+
} catch (anException) {
18+
resultingException = anException;
19+
}
20+
assert.strictEqual(resultingException.message, 'Error during Init');
21+
return resultingException.binding;
22+
})();
23+
824
{
925
const throwTheError = () => { throw theError; };
1026

@@ -50,3 +66,15 @@ const theError = new Error('Some error');
5066
'Exception state did not remain clear as expected,' +
5167
` .wasPending() returned ${exception_pending}`);
5268
}
69+
70+
// Make sure that exceptions that occur during finalization are propagated.
71+
function testFinalize(binding) {
72+
let x = test_exception[binding]();
73+
x = null;
74+
assert.throws(() => { global.gc(); }, /Error during Finalize/);
75+
76+
// To assuage the linter's concerns.
77+
(function() {})(x);
78+
}
79+
testFinalize('createExternal');
80+
testFinalize('createExternalBuffer');

‎test/addons-napi/test_exception/test_exception.c‎

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
static bool exceptionWasPending = false;
55

6-
napi_value returnException(napi_env env, napi_callback_info info) {
6+
static napi_value returnException(napi_env env, napi_callback_info info) {
77
size_t argc = 1;
88
napi_value args[1];
99
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) {
2222
return NULL;
2323
}
2424

25-
napi_value allowException(napi_env env, napi_callback_info info) {
25+
static napi_value allowException(napi_env env, napi_callback_info info) {
2626
size_t argc = 1;
2727
napi_value args[1];
2828
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) {
3838
return NULL;
3939
}
4040

41-
napi_value wasPending(napi_env env, napi_callback_info info) {
41+
static napi_value wasPending(napi_env env, napi_callback_info info) {
4242
napi_value result;
4343
NAPI_CALL(env, napi_get_boolean(env, exceptionWasPending, &result));
4444

4545
return result;
4646
}
4747

48-
napi_value Init(napi_env env, napi_value exports) {
48+
static void finalizer(napi_env env, void *data, void *hint) {
49+
NAPI_CALL_RETURN_VOID(env,
50+
napi_throw_error(env, NULL, "Error during Finalize"));
51+
}
52+
53+
static napi_value createExternal(napi_env env, napi_callback_info info) {
54+
napi_value external;
55+
56+
NAPI_CALL(env,
57+
napi_create_external(env, NULL, finalizer, NULL, &external));
58+
59+
return external;
60+
}
61+
62+
static char buffer_data[12];
63+
64+
static napi_value createExternalBuffer(napi_env env, napi_callback_info info) {
65+
napi_value buffer;
66+
NAPI_CALL(env, napi_create_external_buffer(env, sizeof(buffer_data),
67+
buffer_data, finalizer, NULL, &buffer));
68+
return buffer;
69+
}
70+
71+
static napi_value Init(napi_env env, napi_value exports) {
4972
napi_property_descriptor descriptors[] = {
5073
DECLARE_NAPI_PROPERTY("returnException", returnException),
5174
DECLARE_NAPI_PROPERTY("allowException", allowException),
5275
DECLARE_NAPI_PROPERTY("wasPending", wasPending),
76+
DECLARE_NAPI_PROPERTY("createExternal", createExternal),
77+
DECLARE_NAPI_PROPERTY("createExternalBuffer", createExternalBuffer),
5378
};
54-
5579
NAPI_CALL(env, napi_define_properties(
5680
env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));
5781

82+
napi_value error, code, message;
83+
NAPI_CALL(env, napi_create_string_utf8(env, "Error during Init",
84+
NAPI_AUTO_LENGTH, &message));
85+
NAPI_CALL(env, napi_create_string_utf8(env, "", NAPI_AUTO_LENGTH, &code));
86+
NAPI_CALL(env, napi_create_error(env, code, message, &error));
87+
NAPI_CALL(env, napi_set_named_property(env, error, "binding", exports));
88+
NAPI_CALL(env, napi_throw(env, error));
89+
5890
return exports;
5991
}
6092

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL