Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: avoid crash in napi_escape_scope() #13651

Closed
wants to merge 6 commits into from
Closed

n-api: avoid crash in napi_escape_scope() #13651

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da0edd4
n-api: avoid crash in napi_escape_scope()
mhdawson Jun 13, 2017
039811f
squash: address comments
mhdawson Jun 15, 2017
d534bd3
squash: address next set of comments
mhdawson Jun 16, 2017
120eff1
squash: address comments
mhdawson Jun 20, 2017
08c3151
squash: address comments
mhdawson Jun 21, 2017
f849ae6
squash: address one more comment
mhdawson Jun 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,20 @@ class HandleScopeWrapper {
// across different versions.
class EscapableHandleScopeWrapper {
public:
explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : scope(isolate) {}
explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) :
scope(isolate), _escapeCalled(false) {}
bool escapeAlreadyCalled(void) {
return _escapeCalled;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style issues: method should be bool escape_called() const {, the data member should be escape_called_.

template <typename T>
v8::Local<T> Escape(v8::Local<T> handle) {
_escapeCalled = true;
return scope.Escape(handle);
}

private:
v8::EscapableHandleScope scope;
bool _escapeCalled;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just an extremely minor nit... in various places in core we use _ as a suffix on private fields, in others we seem to use it as a prefix. It would be great to have consistency there.

};

napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) {
Expand Down Expand Up @@ -718,7 +724,8 @@ const char* error_messages[] = {nullptr,
"An array was expected",
"Unknown failure",
"An exception is pending",
"The async work item was cancelled"};
"The async work item was cancelled",
"napi_escape_handle already called on scope"};

static napi_status napi_clear_last_error(napi_env env) {
CHECK_ENV(env);
Expand Down Expand Up @@ -2211,9 +2218,12 @@ napi_status napi_escape_handle(napi_env env,

v8impl::EscapableHandleScopeWrapper* s =
v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope);
*result = v8impl::JsValueFromV8LocalValue(
s->Escape(v8impl::V8LocalValueFromJsValue(escapee)));
return napi_clear_last_error(env);
if (!s->escapeAlreadyCalled()) {
*result = v8impl::JsValueFromV8LocalValue(
s->Escape(v8impl::V8LocalValueFromJsValue(escapee)));
return napi_clear_last_error(env);
}
return napi_set_last_error(env, napi_escape_called_twice);
}

napi_status napi_new_instance(napi_env env,
Expand Down
1 change: 1 addition & 0 deletions src/node_api_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ typedef enum {
napi_generic_failure,
napi_pending_exception,
napi_cancelled,
napi_escape_called_twice,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this enum is public, then inserting a field changes the ABI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is public. At this point since we are experimental it may be ok, but more generally we have to be able to add new error codes so assuming adding to an enum cannot preserve the abi we will have to define our errors in a different way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasongin can you think of any alternative to changing the enums to #defines ? We definitely need to be able to add new errors to the list as we expand or update the api.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does adding an enum value change the ABI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a value is okay, inserting it in the middle is not; it changes subsequent values. You're kind of painted in a corner here because of napi_status_last.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe napi_status_last should be removed, or changed to a #define ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A #define has the same issue: its value is fixed at compile time. If there is no real use case for napi_status_last, I'd remove it.

napi_status_last
} napi_status;

Expand Down
6 changes: 6 additions & 0 deletions test/addons-napi/test_handle_scope/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ testHandleScope.NewScope();

assert.ok(testHandleScope.NewScopeEscape() instanceof Object);

assert.throws(
() => {
testHandleScope.NewScopeEscapeTwice();
},
Error);

assert.throws(
() => {
testHandleScope.NewScopeWithException(() => { throw new RangeError(); });
Expand Down
14 changes: 14 additions & 0 deletions test/addons-napi/test_handle_scope/test_handle_scope.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@ napi_value NewScopeEscape(napi_env env, napi_callback_info info) {
return escapee;
}

napi_value NewScopeEscapeTwice(napi_env env, napi_callback_info info) {
napi_escapable_handle_scope scope;
napi_value output = NULL;
napi_value escapee = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullptr

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is C, not C++.


NAPI_CALL(env, napi_open_escapable_handle_scope(env, &scope));
NAPI_CALL(env, napi_create_object(env, &output));
NAPI_CALL(env, napi_escape_handle(env, scope, output, &escapee));
NAPI_CALL(env, napi_escape_handle(env, scope, output, &escapee));
NAPI_CALL(env, napi_close_escapable_handle_scope(env, scope));
return escapee;
}

napi_value NewScopeWithException(napi_env env, napi_callback_info info) {
napi_handle_scope scope;
size_t argc;
Expand Down Expand Up @@ -57,6 +70,7 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
napi_property_descriptor properties[] = {
DECLARE_NAPI_PROPERTY("NewScope", NewScope),
DECLARE_NAPI_PROPERTY("NewScopeEscape", NewScopeEscape),
DECLARE_NAPI_PROPERTY("NewScopeEscapeTwice", NewScopeEscapeTwice),
DECLARE_NAPI_PROPERTY("NewScopeWithException", NewScopeWithException),
};

Expand Down