From da0edd405ff2de9622ebeede0940631d958ac733 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 13 Jun 2017 00:40:00 -0400 Subject: [PATCH 1/6] 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. --- src/node_api.cc | 20 ++++++++++++++----- src/node_api_types.h | 1 + test/addons-napi/test_handle_scope/test.js | 6 ++++++ .../test_handle_scope/test_handle_scope.c | 14 +++++++++++++ 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 0c8abd0998e734..a56141e95473f2 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -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 escapeAllreadyCalled(void) { + return escapeCalled; + } template v8::Local Escape(v8::Local handle) { + escapeCalled = true; return scope.Escape(handle); } private: v8::EscapableHandleScope scope; + bool escapeCalled; }; napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) { @@ -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); @@ -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->escapeAllreadyCalled()) { + *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, diff --git a/src/node_api_types.h b/src/node_api_types.h index 4bf1b8263139d8..57e6302b5a61d5 100644 --- a/src/node_api_types.h +++ b/src/node_api_types.h @@ -67,6 +67,7 @@ typedef enum { napi_generic_failure, napi_pending_exception, napi_cancelled, + napi_escape_called_twice, napi_status_last } napi_status; diff --git a/test/addons-napi/test_handle_scope/test.js b/test/addons-napi/test_handle_scope/test.js index 20bee78882f071..cb687d0bcbf413 100644 --- a/test/addons-napi/test_handle_scope/test.js +++ b/test/addons-napi/test_handle_scope/test.js @@ -10,6 +10,12 @@ testHandleScope.NewScope(); assert.ok(testHandleScope.NewScopeEscape() instanceof Object); +assert.throws( + () => { + testHandleScope.NewScopeEscapeTwice(); + }, + Error); + assert.throws( () => { testHandleScope.NewScopeWithException(() => { throw new RangeError(); }); diff --git a/test/addons-napi/test_handle_scope/test_handle_scope.c b/test/addons-napi/test_handle_scope/test_handle_scope.c index 6bcee16fda713e..6637da82316185 100644 --- a/test/addons-napi/test_handle_scope/test_handle_scope.c +++ b/test/addons-napi/test_handle_scope/test_handle_scope.c @@ -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; @@ -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), }; From 039811f9ab3b2df1047a973da93722c8e75ea9ba Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 15 Jun 2017 01:50:48 -0400 Subject: [PATCH 2/6] squash: address comments --- src/node_api.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index a56141e95473f2..5410aa499bb03d 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -157,19 +157,19 @@ class HandleScopeWrapper { class EscapableHandleScopeWrapper { public: explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : - scope(isolate), escapeCalled(false) {} - bool escapeAllreadyCalled(void) { - return escapeCalled; + scope(isolate), _escapeCalled(false) {} + bool escapeAlreadyCalled(void) { + return _escapeCalled; } template v8::Local Escape(v8::Local handle) { - escapeCalled = true; + _escapeCalled = true; return scope.Escape(handle); } private: v8::EscapableHandleScope scope; - bool escapeCalled; + bool _escapeCalled; }; napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) { @@ -2218,7 +2218,7 @@ napi_status napi_escape_handle(napi_env env, v8impl::EscapableHandleScopeWrapper* s = v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope); - if (!s->escapeAllreadyCalled()) { + if (!s->escapeAlreadyCalled()) { *result = v8impl::JsValueFromV8LocalValue( s->Escape(v8impl::V8LocalValueFromJsValue(escapee))); return napi_clear_last_error(env); From d534bd35b1f109961c2f0ff97109618d0a8fe706 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 15 Jun 2017 23:19:16 -0400 Subject: [PATCH 3/6] squash: address next set of comments --- src/node_api.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 5410aa499bb03d..c4dfc7f6ce32b6 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -157,19 +157,19 @@ class HandleScopeWrapper { class EscapableHandleScopeWrapper { public: explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : - scope(isolate), _escapeCalled(false) {} - bool escapeAlreadyCalled(void) { - return _escapeCalled; + scope(isolate), escape_called_(false) {} + bool escape_called() const { + return escape_called_; } template v8::Local Escape(v8::Local handle) { - _escapeCalled = true; + escape_called_ = true; return scope.Escape(handle); } private: v8::EscapableHandleScope scope; - bool _escapeCalled; + bool escape_called_; }; napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) { @@ -2218,7 +2218,7 @@ napi_status napi_escape_handle(napi_env env, v8impl::EscapableHandleScopeWrapper* s = v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope); - if (!s->escapeAlreadyCalled()) { + if (!s->escape_called()) { *result = v8impl::JsValueFromV8LocalValue( s->Escape(v8impl::V8LocalValueFromJsValue(escapee))); return napi_clear_last_error(env); From 120eff147d1d5781134c317dce2ff16ec1b39ad7 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 20 Jun 2017 17:45:49 -0400 Subject: [PATCH 4/6] squash: address comments --- src/node_api.cc | 9 +++++++-- src/node_api_types.h | 3 +-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index c4dfc7f6ce32b6..ddcc81286a3ce9 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -753,10 +753,15 @@ napi_status napi_get_last_error_info(napi_env env, CHECK_ENV(env); CHECK_ARG(env, result); + // you must udpate 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, + (sizeof (error_messages) / sizeof (*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 diff --git a/src/node_api_types.h b/src/node_api_types.h index 57e6302b5a61d5..43102c519c03f9 100644 --- a/src/node_api_types.h +++ b/src/node_api_types.h @@ -67,8 +67,7 @@ typedef enum { napi_generic_failure, napi_pending_exception, napi_cancelled, - napi_escape_called_twice, - napi_status_last + napi_escape_called_twice } napi_status; typedef napi_value (*napi_callback)(napi_env env, From 08c3151c1217d099e9e6b9ad398680fcc69944b3 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 21 Jun 2017 11:07:24 -0400 Subject: [PATCH 5/6] squash: address comments --- src/node_api.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index ddcc81286a3ce9..3a20d488f3027a 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -17,6 +17,7 @@ #include #include "uv.h" #include "node_api.h" +#include "node_internals.h" #define NAPI_VERSION 1 @@ -156,8 +157,8 @@ class HandleScopeWrapper { // across different versions. class EscapableHandleScopeWrapper { public: - explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) : - scope(isolate), escape_called_(false) {} + explicit EscapableHandleScopeWrapper(v8::Isolate* isolate) + : scope(isolate), escape_called_(false) {} bool escape_called() const { return escape_called_; } @@ -758,8 +759,7 @@ napi_status napi_get_last_error_info(napi_env env, // 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_escape_called_twice + 1, + 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_escape_called_twice); From f849ae68faf1cceb6a4263760e1ff139587f2af9 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 21 Jun 2017 15:16:47 -0400 Subject: [PATCH 6/6] squash: address one more comment --- src/node_api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_api.cc b/src/node_api.cc index 3a20d488f3027a..9c579cde726869 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -754,7 +754,7 @@ napi_status napi_get_last_error_info(napi_env env, CHECK_ENV(env); CHECK_ARG(env, result); - // you must udpate this assert to reference the last message + // 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.