From 3e18c49657bcb0f2504def3d7d6d900f0c25e357 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 13 Jun 2017 00:40:00 -0400 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/13651 Reviewed-By: Jason Ginchereau Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis --- src/node_api.cc | 29 ++++++++++++++----- src/node_api_types.h | 2 +- test/addons-napi/test_handle_scope/test.js | 6 ++++ .../test_handle_scope/test_handle_scope.c | 14 +++++++++ 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 8978997480178d..18ff7b64e4e5a3 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,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 v8::Local Escape(v8::Local handle) { + escape_called_ = true; return scope.Escape(handle); } private: v8::EscapableHandleScope scope; + bool escape_called_; }; napi_handle_scope JsHandleScopeFromV8HandleScope(HandleScopeWrapper* s) { @@ -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); @@ -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 @@ -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, diff --git a/src/node_api_types.h b/src/node_api_types.h index 4bf1b8263139d8..43102c519c03f9 100644 --- a/src/node_api_types.h +++ b/src/node_api_types.h @@ -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, 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), };