node-api: type tag external values without v8::Private#51149
Merged
Uh oh!
There was an error while loading. Please reload this page.
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
nodejs-github-bot commented Dec 13, 2023
Review requested:
|
nodejs-github-bot added c++ 
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.
legendecasforce-pushed the node-api/external branch from 1d0ed04 to 2012d0fCompare
Member
vmoroz commented Dec 14, 2023
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. |
MemberAuthor
legendecas commented Dec 15, 2023
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. |
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Dec 18, 2023
Collaborator
nodejs-github-bot commented Dec 20, 2023
gabrielschulhof approved these changes Dec 22, 2023
targos added the commit-queue nodejs-github-bot removed the commit-queue Collaborator
nodejs-github-bot commented Dec 22, 2023
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: #51149Fixes: nodejs/node-v8#273 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
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: #51149Fixes: nodejs/node-v8#273 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
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 <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Sign up for freeto 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