node-api: External type tag check fails if the original tag struct changes or disappears (regression in Node 20.12 and Node 21.6)

[Koromix]

Version

v20.12.1

Platform

Linux looper 6.5.0-26-generic #26~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Mar 12 10:22:43 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

node-api

What steps will reproduce the bug?

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 using temporary tags (made on the stack). 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 by value in a private property (in a BigInt). So the pointer does not get stale.

I've made a small reproduction available here: https://git.sr.ht/~koromix/napi_tag_bug

To reproduce, execute these commands:

nvm install 20.11
nvm install 20.12

git clone https://git.sr.ht/~koromix/napi_tag_bug
cd napi_tag_bug
npm install # uses cmake-js

nvm use 20.12
npm test

nvm use 20.11
npm test

What is the expected behavior? Why is that the expected behavior?

Expected result with Node v20.11.1:

Value should match, does it: true
Success!

This works because the type tag is embedded by value instead of just the pointer to the tag passed to napi_type_tag_object().

What do you see instead?

Wrong result with Node v20.12.1:

Value should match, does it: false
Failure (regression)!

Here it fails because the reproduction code explicitly clears out the tag value (which lives on the stack by the way) and the External value refers to it (instead of copying it).

Proposed fix and test

The following patch fixes the bug: store_tag_value.patch

I propose the following change to the test suite to prevent future regressions: prevent_tag_pointers.patch

I can make a pull request if you prefer.

[mhdawson]

@VoltrexKeyva a PR would be great, it would be the best way to review your proposed changes and get them landed if appropriate.

[VoltrexKeyva]

@VoltrexKeyva a PR would be great, it would be the best way to review your proposed changes and get them landed if appropriate.

I don't follow, am I pinged by mistake or what are you referring to?

[Koromix]

I have created the PR here: #52426

[mhdawson]

Closing since the related PR landed, please let us know if that was not the right thing to do.

[mhdawson]

@Koromix

[Koromix]

Yes it's all right, the PR solves the problem :)