n-api: Implement stricter wrapping

Gabriel Schulhof · MylesBorins · commit 7973bd3e630e · 2018-04-16T17:46:01.000-04:00
Use a stronger criterion to identify objects in the prototype chain that store pointers to native data that were added by previous calls to `napi_wrap()`. Whereas the old criterion for identifying `napi_wrap()`-injected prototype chain objects was to consider an object with an internal field count of 1 to be such an object, the new criterion is to consider an object with an internal field count of 2 such that the second field holds a `v8::External` which itself contains a pointer to a global static string unique to N-API to be a `napi_wrap()`-injected prototype chain object. This greatly reduces the possibility of returning a pointer that was not previously added with `napi_wrap()`, and it allows us to recognize that an object has already undergone `napi_wrap()` and we can thus prevent a chain of wrappers only the first of which is accessible from appearing in the prototype chain, as would be the result of multiple calls to `napi_wrap()` using the same object. Backport-PR-URL: #19447 PR-URL: #13872 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
diff --git a/doc/api/n-api.md b/doc/api/n-api.md
@@ -2876,8 +2876,8 @@ napi_status napi_wrap(napi_env env,
 
 Returns `napi_ok` if the API succeeded.
 
-Wraps a native instance in JavaScript object of the corresponding type.
-The native instance can be retrieved later using `napi_unwrap()`.
+Wraps a native instance in a JavaScript object. The native instance can be
+retrieved later using `napi_unwrap()`.
 
 When JavaScript code invokes a constructor for a class that was defined using
 `napi_define_class()`, the `napi_callback` for the constructor is invoked.
@@ -2905,6 +2905,10 @@ required in order to enable correct proper of the reference.
 Afterward, additional manipulation of the wrapper's prototype chain may cause
 `napi_unwrap()` to fail.
 
+*Note*: Calling `napi_wrap()` a second time on an object that already has a
+native instance associated with it by virtue of a previous call to
+`napi_wrap()` will cause an error to be returned.
+
 ### *napi_unwrap*
 <!-- YAML
 added: v8.0.0
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -678,6 +678,38 @@ v8::Local<v8::Object> CreateAccessorCallbackData(napi_env env,
   return cbdata;
 }
 
+// Pointer used to identify items wrapped by N-API. Used by FindWrapper and
+// napi_wrap().
+const char napi_wrap_name[] = "N-API Wrapper";
+
+// Search the object's prototype chain for the wrapper object. Usually the
+// wrapper would be the first in the chain, but it is OK for other objects to
+// be inserted in the prototype chain.
+bool FindWrapper(v8::Local<v8::Object> obj,
+                 v8::Local<v8::Object>* result = nullptr) {
+  v8::Local<v8::Object> wrapper = obj;
+
+  do {
+    v8::Local<v8::Value> proto = wrapper->GetPrototype();
+    if (proto.IsEmpty() || !proto->IsObject()) {
+      return false;
+    }
+    wrapper = proto.As<v8::Object>();
+    if (wrapper->InternalFieldCount() == 2) {
+      v8::Local<v8::Value> external = wrapper->GetInternalField(1);
+      if (external->IsExternal() &&
+          external.As<v8::External>()->Value() == v8impl::napi_wrap_name) {
+        break;
+      }
+    }
+  } while (true);
+
+  if (result != nullptr) {
+    *result = wrapper;
+  }
+  return true;
+}
+
 }  // end of namespace v8impl
 
 // Intercepts the Node-V8 module registration callback. Converts parameters
@@ -2049,11 +2081,22 @@ napi_status napi_wrap(napi_env env,
   RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg);
   v8::Local<v8::Object> obj = value.As<v8::Object>();
 
-  // Create a wrapper object with an internal field to hold the wrapped pointer.
+  // If we've already wrapped this object, we error out.
+  RETURN_STATUS_IF_FALSE(env, !v8impl::FindWrapper(obj), napi_invalid_arg);
+
+  // Create a wrapper object with an internal field to hold the wrapped pointer
+  // and a second internal field to identify the owner as N-API.
   v8::Local<v8::ObjectTemplate> wrapper_template;
-  ENV_OBJECT_TEMPLATE(env, wrap, wrapper_template, 1);
-  v8::Local<v8::Object> wrapper =
-      wrapper_template->NewInstance(context).ToLocalChecked();
+  ENV_OBJECT_TEMPLATE(env, wrap, wrapper_template, 2);
+
+  auto maybe_object = wrapper_template->NewInstance(context);
+  CHECK_MAYBE_EMPTY(env, maybe_object, napi_generic_failure);
+
+  v8::Local<v8::Object> wrapper = maybe_object.ToLocalChecked();
+  wrapper->SetInternalField(1, v8::External::New(isolate,
+    reinterpret_cast<void*>(const_cast<char*>(v8impl::napi_wrap_name))));
+
+  // Store the pointer as an external in the wrapper.
   wrapper->SetInternalField(0, v8::External::New(isolate, native_object));
 
   // Insert the wrapper into the object's prototype chain.
@@ -2090,16 +2133,9 @@ napi_status napi_unwrap(napi_env env, napi_value js_object, void** result) {
   RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg);
   v8::Local<v8::Object> obj = value.As<v8::Object>();
 
-  // Search the object's prototype chain for the wrapper with an internal field.
-  // Usually the wrapper would be the first in the chain, but it is OK for
-  // other objects to be inserted in the prototype chain.
-  v8::Local<v8::Object> wrapper = obj;
-  do {
-    v8::Local<v8::Value> proto = wrapper->GetPrototype();
-    RETURN_STATUS_IF_FALSE(
-      env, !proto.IsEmpty() && proto->IsObject(), napi_invalid_arg);
-    wrapper = proto.As<v8::Object>();
-  } while (wrapper->InternalFieldCount() != 1);
+  v8::Local<v8::Object> wrapper;
+  RETURN_STATUS_IF_FALSE(
+    env, v8impl::FindWrapper(obj, &wrapper), napi_invalid_arg);
 
   v8::Local<v8::Value> unwrappedValue = wrapper->GetInternalField(0);
   RETURN_STATUS_IF_FALSE(env, unwrappedValue->IsExternal(), napi_invalid_arg);
diff --git a/test/addons-napi/test_general/test.js b/test/addons-napi/test_general/test.js
@@ -50,3 +50,11 @@ assert.strictEqual(test_general.testGetVersion(), 1);
 // since typeof in js return object need to validate specific case
 // for null
 assert.strictEqual(test_general.testNapiTypeof(null), 'null');
+
+const x = {};
+
+// Assert that wrapping twice fails.
+test_general.wrap(x, 25);
+assert.throws(function() {
+  test_general.wrap(x, 'Blah');
+}, Error);
diff --git a/test/addons-napi/test_general/test_general.c b/test/addons-napi/test_general/test_general.c
@@ -119,6 +119,24 @@ napi_value testNapiTypeof(napi_env env, napi_callback_info info) {
   return result;
 }
 
+static void deref_item(napi_env env, void* data, void* hint) {
+  (void) hint;
+
+  NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, (napi_ref)data));
+}
+
+napi_value wrap(napi_env env, napi_callback_info info) {
+  size_t argc = 2;
+  napi_value argv[2];
+  napi_ref payload;
+
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
+  NAPI_CALL(env, napi_create_reference(env, argv[1], 1, &payload));
+  NAPI_CALL(env, napi_wrap(env, argv[0], payload, deref_item, NULL, NULL));
+
+  return NULL;
+}
+
 void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
   napi_property_descriptor descriptors[] = {
     DECLARE_NAPI_PROPERTY("testStrictEquals", testStrictEquals),
@@ -130,6 +148,7 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
     DECLARE_NAPI_PROPERTY("createNapiError", createNapiError),
     DECLARE_NAPI_PROPERTY("testNapiErrorCleanup", testNapiErrorCleanup),
     DECLARE_NAPI_PROPERTY("testNapiTypeof", testNapiTypeof),
+    DECLARE_NAPI_PROPERTY("wrap", wrap),
   };
 
   NAPI_CALL_RETURN_VOID(env, napi_define_properties(
