node-api: type tag external values without v8::Private #51149
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Review requested:
|
v8::External can not have any properties and private properties. Type tag v8::External with a wrapper struct without setting a private property on the v8::External.
1d0ed04 to
2012d0f
Compare
Member
|
My concern with this change is that it adds a lot of overhead to each external value even if it does not use a type tag. |
Member
Author
|
nodejs/node-v8#273 is blocking node-v8 updates. By V8 API contract, I'd be willing to update the PR if there is any other approach that we can unblock the node-v8 update. |
Collaborator
Collaborator
gabrielschulhof
approved these changes
Dec 22, 2023
Collaborator
|
Landed in a81788c |
RafaelGSS
pushed a commit
that referenced
this pull request
Jan 2, 2024
v8::External can not have any properties and private properties. Type tag v8::External with a wrapper struct without setting a private property on the v8::External. PR-URL: #51149 Fixes: nodejs/node-v8#273 Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Merged
richardlau
pushed a commit
that referenced
this pull request
Mar 25, 2024
v8::External can not have any properties and private properties. Type tag v8::External with a wrapper struct without setting a private property on the v8::External. PR-URL: #51149 Fixes: nodejs/node-v8#273 Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Merged
Koromix
added a commit
to Koromix/rygel
that referenced
this pull request
Apr 4, 2024
At least it seems to be a bug according to the N-API documentation, or a documentation mistake, and probably comes from here: nodejs/node#51149 This Node.js code change stores the napi_type_tag pointer and not its value, but Koffi was using temporaries.
Koromix
added a commit
to Koromix/rygel
that referenced
this pull request
Apr 5, 2024
At least it seems to be a bug according to the N-API documentation, or a documentation mistake, and probably comes from here: nodejs/node#51149 This Node.js code change stores the napi_type_tag pointer and not its value, but Koffi was using temporaries.
This was referenced Apr 6, 2024
nodejs-github-bot
pushed a commit
that referenced
this pull request
Apr 15, 2024
In order to adapt to V8 changes regarding storing private properties on Externals, ExternalWrapper objects were introduced in #51149. However, this new code stores the type tag pointer and not the 128-bit value inside. This breaks some pre-existing code that were making temporary tags. It also means that unloading the module will cause existing External objects to have a tag pointer that points nowhere (use-after-free bug). Change ExternalWrapper to store tags by value to fix this regression. PR-URL: #52426 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
thisalihassan
pushed a commit
to thisalihassan/node
that referenced
this pull request
Apr 15, 2024
In order to adapt to V8 changes regarding storing private properties on Externals, ExternalWrapper objects were introduced in nodejs#51149. However, this new code stores the type tag pointer and not the 128-bit value inside. This breaks some pre-existing code that were making temporary tags. It also means that unloading the module will cause existing External objects to have a tag pointer that points nowhere (use-after-free bug). Change ExternalWrapper to store tags by value to fix this regression. PR-URL: nodejs#52426 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
aduh95
pushed a commit
that referenced
this pull request
Apr 29, 2024
In order to adapt to V8 changes regarding storing private properties on Externals, ExternalWrapper objects were introduced in #51149. However, this new code stores the type tag pointer and not the 128-bit value inside. This breaks some pre-existing code that were making temporary tags. It also means that unloading the module will cause existing External objects to have a tag pointer that points nowhere (use-after-free bug). Change ExternalWrapper to store tags by value to fix this regression. PR-URL: #52426 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito
pushed a commit
that referenced
this pull request
May 2, 2024
In order to adapt to V8 changes regarding storing private properties on Externals, ExternalWrapper objects were introduced in #51149. However, this new code stores the type tag pointer and not the 128-bit value inside. This breaks some pre-existing code that were making temporary tags. It also means that unloading the module will cause existing External objects to have a tag pointer that points nowhere (use-after-free bug). Change ExternalWrapper to store tags by value to fix this regression. PR-URL: #52426 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
marco-ippolito
pushed a commit
that referenced
this pull request
May 3, 2024
In order to adapt to V8 changes regarding storing private properties on Externals, ExternalWrapper objects were introduced in #51149. However, this new code stores the type tag pointer and not the 128-bit value inside. This breaks some pre-existing code that were making temporary tags. It also means that unloading the module will cause existing External objects to have a tag pointer that points nowhere (use-after-free bug). Change ExternalWrapper to store tags by value to fix this regression. PR-URL: #52426 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
v8::External can not have any properties and private properties. Type
tag v8::External with a wrapper struct without setting a private
property on the v8::External.
Fixes: nodejs/node-v8#273