N-API: Implement stricter wrapping by gabrielschulhof · Pull Request #13872 · nodejs/node · GitHub
Skip to content

N-API: Implement stricter wrapping #13872

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

Closed
gabrielschulhof 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
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,

@gabrielschulhof gabrielschulhof Jun 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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?

@gabrielschulhof gabrielschulhof Jun 22, 2017

Copy link
Copy Markdown
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

@mhdawson mhdawson Jun 22, 2017

Copy link
Copy Markdown
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.

@mhdawson mhdawson Jun 28, 2017

Copy link
Copy Markdown
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.

@bnoordhuis bnoordhuis Jul 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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