N-API: Implement stricter wrapping #13872
Conversation
src/node_api.cc
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@nodejs/addon-api
There was a problem hiding this comment.
I'd say it we are better off depending on something that prevents things from working across modules.
There was a problem hiding this comment.
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.
9d6d6f1 to
5fd44ce
Compare
|
will wait to do complete review until updated to return an error on second wrap as discussed in the N-API meeting today. |
5fd44ce to
ad01722
Compare
|
@mhdawson done now. |
doc/api/n-api.md
Outdated
There was a problem hiding this comment.
I think this needs to be updated to reflect an error will be thrown.
src/node_api.cc
Outdated
There was a problem hiding this comment.
This should be updated as well. Since we should error out above.
There was a problem hiding this comment.
I think we should add this test to test_general instead of using a new addon. Each addon makes the compile just a little bit longer.
ad01722 to
19fb553
Compare
|
@mhdawson I have addressed your comments. |
src/node_api.cc
Outdated
There was a problem hiding this comment.
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.
|
@gabrielschulhof sorry this dropped off my radar, needs a rebase and a new CI run. |
19fb553 to
023c1bf
Compare
|
I had an unrelated test failure when I ran |
|
@mhdawson I've rebased, but I suspect a test run now will likely run into this unrelated error. |
|
Needs one more rebase since I landed the other PR we talked about first. |
023c1bf to
ee07177
Compare
|
@mhdawson It's rebased now, and when I ran |
src/node_api.cc
Outdated
There was a problem hiding this comment.
I left out the const because converting from const char* to void* requires a pile of casting:
reinterpret_cast<void*>(const_cast<char*>(v8impl::napi_wrap_name))But anyway, I'll add it.
ee07177 to
c8f4446
Compare
|
Looks like @gabrielschulhof pushed a commit to address @TimothyGu's comments. CI run - https://ci.nodejs.org/job/node-test-pull-request/9030/ |
|
Is this OK to land? |
doc/api/n-api.md
Outdated
src/node_api.cc
Outdated
There was a problem hiding this comment.
Why are you overwriting the External created and stored just a few lines up?
There was a problem hiding this comment.
I'm not. A few lines up I'm setting the other index (1) whereas here I'm setting (0).
There was a problem hiding this comment.
Oh, dang! Merge error. You're right!
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.
c8f4446 to
10e30e5
Compare
|
@TimothyGu should be good now, but it needs another CI run. |
|
Running CI at https://ci.nodejs.org/job/node-test-pull-request/9078/. Might take a while though because of the backlog from the CI lockdown. |
|
Can you lowercase the subsystem in the commit? |
|
@evanlucas will do |
|
Landed as d5b397c |
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. 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>
|
@bnoordhuis I just noticed your comment about not recognizing the wrap between 2 modules after landing. I had discussed this with Gabriel and the way the check works that is not a problem since we don't check against the FunctionTemplate itself. Having said that @gabrielschulhof does make sense for us to look at the suggestion to see if that makes thing better. |
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. 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>
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. 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>
Store the `napi_env` on the global object at a private key. This gives us one `napi_env` per context. Re: nodejs#13872 (comment)
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. PR-URL: nodejs#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>
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>
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 prototypechain 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::Externalwhich itself contains a pointer to a global static string uniqueto 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 anobject has already undergone
napi_wrap()and we can thus replace the oldv8::Externalwith the new one such that, when the old one will be garbage-collected, any finalize callback for its native pointer will also be called.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)