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 <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>

4 files changed, 43 insertions, 8 deletions

diff --git a/src/node_api.cc b/src/node_api.cc
index 8978997480..18ff7b64e4 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -17,6 +17,7 @@
 #include <vector>
 #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 <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) {
@@ -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 4bf1b82631..43102c519c 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 20bee78882..cb687d0bcb 100644
--- a/test/addons-napi/test_handle_scope/test.js
+++ b/test/addons-napi/test_handle_scope/test.js
@@ -12,6 +12,12 @@ assert.ok(testHandleScope.NewScopeEscape() instanceof Object);
 
 assert.throws(
   () => {
+    testHandleScope.NewScopeEscapeTwice();
+  },
+  Error);
+
+assert.throws(
+  () => {
     testHandleScope.NewScopeWithException(() => { throw new RangeError(); });
   },
   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 6bcee16fda..6637da8231 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),
   };