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. Backport-PR-URL: #19447 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]>
nodejs · Apr 16, 2018 · c3eb187 · c3eb187
1 parent cc3a4af
commit c3eb187
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 8 deletions.
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -10,13 +10,15 @@
 
 #include <node_buffer.h>
 #include <node_object_wrap.h>
+
 #include <string.h>
 #include <algorithm>
 #include <cassert>
 #include <cmath>
 #include <vector>
 #include "uv.h"
 #include "node_api.h"
+#include "node_internals.h"
 #include "env-inl.h"
 #include "node_api_backport.h"
 
@@ -160,14 +162,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) {
@@ -720,7 +728,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);
@@ -748,10 +757,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
@@ -2213,9 +2226,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
@@ -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
@@ -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
@@ -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),
   };