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.

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

node-api: type tag external values without v8::Private #51149

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

legendecas
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Dec 13, 2023
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.
@vmoroz
Copy link
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.
It would be nice to be able to keep cheap external values for the most common scenarios.
What are the targeted scenarios? Can we address them with new APIs?

@legendecas
Copy link
Member Author

nodejs/node-v8#273 is blocking node-v8 updates. By V8 API contract, v8::External doesn't support adding properties so there is no additional slot to distinguish if a v8::External has a node-api type tag. That's why we need a wrapper for every napi_external.

I'd be willing to update the PR if there is any other approach that we can unblock the node-v8 update.

Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit a81788c into nodejs:main Dec 22, 2023
54 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a81788c

@legendecas legendecas deleted the node-api/external branch December 22, 2023 14:44
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>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
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>
@richardlau richardlau mentioned this pull request Mar 25, 2024
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.
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
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test/js-native-api/test_object/test.js fails with debug build
6 participants