Skip to content

Commit

Permalink
n-api: avoid crash in napi_escape_scope()
Browse files Browse the repository at this point in the history
V8 will crash if escape is called twice on the same
scope.

Add checks to avoid crashing if napi_escape_scope() is
called to try and do this.

Add test that tries to call napi_create_scope() twice.

PR-URL: #13651
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
mhdawson authored and addaleax committed Jun 21, 2017
1 parent 5840138 commit f49dd21
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 8 deletions.
29 changes: 22 additions & 7 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <vector>
#include "uv.h"
#include "node_api.h"
#include "node_internals.h"

#define NAPI_VERSION 1

Expand Down Expand Up @@ -156,14 +157,20 @@ class HandleScopeWrapper {
// across different versions.
class EscapableHandleScopeWrapper {
public:
explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : scope(isolate) {}
explicit EscapableHandleScopeWrapper(v8::Isolate* isolate)
: scope(isolate), escape_called_(false) {}
bool escape_called() const {
return escape_called_;
}
template <typename T>
v8::Local<T> Escape(v8::Local<T> handle) {
escape_called_ = true;
return scope.Escape(handle);
}

private:
v8::EscapableHandleScope scope;
bool escape_called_;
};

napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) {
Expand Down Expand Up @@ -718,7 +725,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 @@ -746,10 +754,14 @@ napi_status napi_get_last_error_info(napi_env env,
CHECK_ENV(env);
CHECK_ARG(env, result);

// you must update this assert to reference the last message
// in the napi_status enum each time a new error message is added.
// We don't have a napi_status_last as this would result in an ABI
// change each time a message was added.
static_assert(
(sizeof (error_messages) / sizeof (*error_messages)) == napi_status_last,
node::arraysize(error_messages) == napi_escape_called_twice + 1,
"Count of error messages must match count of error values");
assert(env->last_error.error_code < napi_status_last);
assert(env->last_error.error_code <= napi_escape_called_twice);

// Wait until someone requests the last error information to fetch the error
// message string
Expand Down Expand Up @@ -2211,9 +2223,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->escape_called()) {
*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
2 changes: 1 addition & 1 deletion src/node_api_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ typedef enum {
napi_generic_failure,
napi_pending_exception,
napi_cancelled,
napi_status_last
napi_escape_called_twice
} napi_status;

typedef napi_value (*napi_callback)(napi_env env,
Expand Down
6 changes: 6 additions & 0 deletions test/addons-napi/test_handle_scope/test.js
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
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;

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

0 comments on commit f49dd21

Please sign in to comment.