node-api: store external type tags by value

[Koromix]

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:

A type tag is a 128-bit integer unique to the addon. Node-API provides the napi_type_tag structure for storing a type tag. [...] This creates a type-checking capability of a higher fidelity than napi_instanceof() can provide, because such type- tagging survives prototype manipulation and addon unloading/reloading.

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.

[nodejs-github-bot]

Review requested:

• @nodejs/node-api

[gabrielschulhof]

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?

[Koromix]

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 ¹² 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

1. TypeError: Unexpected External value as type specifier, expected string or type Koromix/koffi#142 ↩

2. Unexpected External value as type specifier, expected string or type Koromix/koffi#144 ↩

[gabrielschulhof]

@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]

I've updated the test commit with better comments (for memset), and force-pushed the new commits.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58317/

[mhdawson]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58325/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58333/

[nodejs-github-bot]

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)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

node-api: copy external type tags when they are set
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]
[detached HEAD b10b3ff59f] node-api: copy external type tags when they are set

Author: Niels Martignène [email protected]

Date: Mon Apr 8 18:41:32 2024 +0200

1 file changed, 7 insertions(+), 6 deletions(-)

Rebasing (3/4)

Rebasing (4/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

node-api: test that type tags are stored by value
PR-URL: #52426

Reviewed-By: Gabriel Schulhof [email protected]

Reviewed-By: Chengzhong Wu [email protected]

Reviewed-By: Michael Dawson [email protected]
[detached HEAD 5a0e034827] node-api: test that type tags are stored by value

Author: Niels Martignène [email protected]

Date: Mon Apr 8 18:44:10 2024 +0200

1 file changed, 26 insertions(+), 3 deletions(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/8682371396

[nodejs-github-bot]

Landed in c1bbc5d