Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

N-API: Implement stricter wrapping #13872

Closed
wants to merge 1 commit into from
Closed

N-API: Implement stricter wrapping #13872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions doc/api/n-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
64 changes: 50 additions & 14 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,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,
Copy link
Contributor Author

@gabrielschulhof gabrielschulhof Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we used a v8::FunctionTemplate instead of a simple string we could take advantage of v8::Object::FindInstanceInPrototypeChain(Local< FunctionTemplate> tmpl) instead of having to perform the descent ourselves.

The problem with that is that the FunctionTemplate it requires needs to be stored somewhere. Now, it can be stored on the napi_env but the problem with that is that the env is per-module. So, if one module does napi_wrap(), then the other module will not recognize that this has happened, because it will have its own copy of a FunctionTemplate serving the same purpose. Is this desirable or is this to be avoided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodejs/addon-api

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it we are better off depending on something that prevents things from working across modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to say not depending on something that prevents things from working across modules. I think that's why you avoided the function template in the PR so all should be good.

Copy link
Member

@bnoordhuis bnoordhuis Jul 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if one module does napi_wrap(), then the other module will not recognize that this has happened, because it will have its own copy of a FunctionTemplate serving the same purpose.

It doesn't have to be. FunctionTemplates can be per-isolate (don't need to be per-context) so if you manage to associate some state with the isolate, you should be good.

As to how to do that: you could use v8::Isolate::SetData() but that's a scarce resource (only 4 slots and one is already taken.)

Perhaps a static std::map<v8::Isolate*, State*> guarded by a mutex? You only need to look it up once per napi_env because you can cache it inside the env. Lock contention shouldn't be an issue.

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
Expand Down Expand Up @@ -2046,11 +2078,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.
Expand Down Expand Up @@ -2087,16 +2130,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);
Expand Down
8 changes: 8 additions & 0 deletions test/addons-napi/test_general/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
19 changes: 19 additions & 0 deletions test/addons-napi/test_general/test_general.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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(
Expand Down