node-api: store external type tags by value #52426
Conversation
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.
|
Review requested:
|
nodejs-github-bot
added
c++
|
The original intent behind type tags was to be able to use a global static constant structure as the source. That way, tags work across module instances, and we save some memory per-object. I think we should have a new API for copying the tag into the object. That way only those objects will use extra space that need to. OTOH we should change the signature for the existing API to accept a constant pointer to a constant structure. @nodejs/node-api WDYT? |
|
While I agree with this design/intent, my main problem is that it was not clear from the documentation and the existing API. And at least for me, my library Koffi 12 stopped working on Node 20.12 and 21.6 because of this change. I understand that I was misusing tags, and I have changed my code since then. But I might not have been the only one, as it was not clear from the documentation and it was working before. Footnotes |
|
@Koromix hmmm... it looks like our existing implementation actually copies the type tag, since it stores it in a BigInt. Thus, we should probably replicate this behaviour across the board for this API, and make the non-copying one the new API. |
Koromix
force-pushed
the
store_external_type_tags_by_value
branch
from
ebcbb47
to
f608fee
Compare
|
I've updated the test commit with better comments (for memset), and force-pushed the new commits. |
github-actions
bot
removed
the
request-ci
gabrielschulhof
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/52426 ✔ Done loading data for nodejs/node/pull/52426 ----------------------------------- PR info ------------------------------------ Title node-api: store external type tags by value (#52426) Author Niels Martignène (@Koromix, first-time contributor) Branch Koromix:store_external_type_tags_by_value -> nodejs:main Labels c++, node-api Commits 2 - node-api: copy external type tags when they are set - node-api: test that type tags are stored by value Committers 1 - Niels Martignène PR-URL: https://github.com/nodejs/node/pull/52426 Reviewed-By: Gabriel Schulhof Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52426 Reviewed-By: Gabriel Schulhof Reviewed-By: Chengzhong Wu Reviewed-By: Michael Dawson -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 08 Apr 2024 17:02:45 GMT ✔ Approvals: 3 ✔ - Gabriel Schulhof (@gabrielschulhof): https://github.com/nodejs/node/pull/52426#pullrequestreview-1995030568 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/52426#pullrequestreview-1996933393 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/52426#pullrequestreview-1997448487 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-04-12T21:09:49Z: https://ci.nodejs.org/job/node-test-pull-request/58333/ - Querying data for job/node-test-pull-request/58333/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52426 From https://github.com/nodejs/node * branch refs/pull/52426/merge -> FETCH_HEAD ✔ Fetched commits as f8e325ea8a24..f608fee85cae -------------------------------------------------------------------------------- [main 91618fb545] node-api: copy external type tags when they are set Author: Niels Martignène Date: Mon Apr 8 18:41:32 2024 +0200 1 file changed, 7 insertions(+), 6 deletions(-) [main d7dbcbe003] node-api: test that type tags are stored by value Author: Niels Martignène Date: Mon Apr 8 18:44:10 2024 +0200 1 file changed, 26 insertions(+), 3 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)https://github.com/nodejs/node/actions/runs/8682371396 |
gabrielschulhof
added
commit-queue-squash
gabrielschulhof
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in c1bbc5d |
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>
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>
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>
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>
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>
Overview
A change introduced in Node 20.12 and Node 21.6 changes the semantics of tagging External values, and introduces a possible memory bug.
This pull request is responsible for the change: #51149
The code in this commit 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).
This violates what is stated in the N-API documentation:
For objects, nothing has changed since type tags are still stored in a private property (in a BigInt). So the pointer does not get stale.
This potential bug was reported here: #52387
Patch series
The first patch fixes the issue, the second patch changes existing type tagging tests to make sure this issue does not reoccur in the future.