Skip to content

Commit f49dd21

Browse files
mhdawsonaddaleax
authored andcommitted
n-api: avoid crash in napi_escape_scope()
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
1 parent 5840138 commit f49dd21

File tree

4 files changed

+43
-8
lines changed

4 files changed

+43
-8
lines changed

src/node_api.cc

+22-7
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <vector>
1818
#include "uv.h"
1919
#include "node_api.h"
20+
#include "node_internals.h"
2021

2122
#define NAPI_VERSION 1
2223

@@ -156,14 +157,20 @@ class HandleScopeWrapper {
156157
// across different versions.
157158
class EscapableHandleScopeWrapper {
158159
public:
159-
explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : scope(isolate) {}
160+
explicit EscapableHandleScopeWrapper(v8::Isolate* isolate)
161+
: scope(isolate), escape_called_(false) {}
162+
bool escape_called() const {
163+
return escape_called_;
164+
}
160165
template <typename T>
161166
v8::Local<T> Escape(v8::Local<T> handle) {
167+
escape_called_ = true;
162168
return scope.Escape(handle);
163169
}
164170

165171
private:
166172
v8::EscapableHandleScope scope;
173+
bool escape_called_;
167174
};
168175

169176
napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) {
@@ -718,7 +725,8 @@ const char* error_messages[] = {nullptr,
718725
"An array was expected",
719726
"Unknown failure",
720727
"An exception is pending",
721-
"The async work item was cancelled"};
728+
"The async work item was cancelled",
729+
"napi_escape_handle already called on scope"};
722730

723731
static napi_status napi_clear_last_error(napi_env env) {
724732
CHECK_ENV(env);
@@ -746,10 +754,14 @@ napi_status napi_get_last_error_info(napi_env env,
746754
CHECK_ENV(env);
747755
CHECK_ARG(env, result);
748756

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

754766
// Wait until someone requests the last error information to fetch the error
755767
// message string
@@ -2211,9 +2223,12 @@ napi_status napi_escape_handle(napi_env env,
22112223

22122224
v8impl::EscapableHandleScopeWrapper* s =
22132225
v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope);
2214-
*result = v8impl::JsValueFromV8LocalValue(
2215-
s->Escape(v8impl::V8LocalValueFromJsValue(escapee)));
2216-
return napi_clear_last_error(env);
2226+
if (!s->escape_called()) {
2227+
*result = v8impl::JsValueFromV8LocalValue(
2228+
s->Escape(v8impl::V8LocalValueFromJsValue(escapee)));
2229+
return napi_clear_last_error(env);
2230+
}
2231+
return napi_set_last_error(env, napi_escape_called_twice);
22172232
}
22182233

22192234
napi_status napi_new_instance(napi_env env,

src/node_api_types.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ typedef enum {
6767
napi_generic_failure,
6868
napi_pending_exception,
6969
napi_cancelled,
70-
napi_status_last
70+
napi_escape_called_twice
7171
} napi_status;
7272

7373
typedef napi_value (*napi_callback)(napi_env env,

test/addons-napi/test_handle_scope/test.js

+6
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,12 @@ testHandleScope.NewScope();
1010

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

13+
assert.throws(
14+
() => {
15+
testHandleScope.NewScopeEscapeTwice();
16+
},
17+
Error);
18+
1319
assert.throws(
1420
() => {
1521
testHandleScope.NewScopeWithException(() => { throw new RangeError(); });

test/addons-napi/test_handle_scope/test_handle_scope.c

+14
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,19 @@ napi_value NewScopeEscape(napi_env env, napi_callback_info info) {
2929
return escapee;
3030
}
3131

32+
napi_value NewScopeEscapeTwice(napi_env env, napi_callback_info info) {
33+
napi_escapable_handle_scope scope;
34+
napi_value output = NULL;
35+
napi_value escapee = NULL;
36+
37+
NAPI_CALL(env, napi_open_escapable_handle_scope(env, &scope));
38+
NAPI_CALL(env, napi_create_object(env, &output));
39+
NAPI_CALL(env, napi_escape_handle(env, scope, output, &escapee));
40+
NAPI_CALL(env, napi_escape_handle(env, scope, output, &escapee));
41+
NAPI_CALL(env, napi_close_escapable_handle_scope(env, scope));
42+
return escapee;
43+
}
44+
3245
napi_value NewScopeWithException(napi_env env, napi_callback_info info) {
3346
napi_handle_scope scope;
3447
size_t argc;
@@ -57,6 +70,7 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
5770
napi_property_descriptor properties[] = {
5871
DECLARE_NAPI_PROPERTY("NewScope", NewScope),
5972
DECLARE_NAPI_PROPERTY("NewScopeEscape", NewScopeEscape),
73+
DECLARE_NAPI_PROPERTY("NewScopeEscapeTwice", NewScopeEscapeTwice),
6074
DECLARE_NAPI_PROPERTY("NewScopeWithException", NewScopeWithException),
6175
};
6276

0 commit comments

Comments
 (0)