Implement napi type tags and reject unwrapping objects that were never wrapped - #229
Draft
bghgary wants to merge 1 commit into
Draft
Implement napi type tags and reject unwrapping objects that were never wrapped#229bghgary wants to merge 1 commit into
bghgary wants to merge 1 commit into
Conversation
…cessary napi_unwrap returns void* and does no type check, so it cannot answer "is this JS object one of my type". The Node-API answer is napi_type_tag_object / napi_check_object_type_tag, and until now no engine here exposed them: NAPI_VERSION is pinned to 5 and the whole type-tag surface sits behind NAPI_VERSION >= 8, so even the V8 body was dead code -- and would not have compiled, since the NAPI_PRIVATE_KEY it calls was commented out. Raising NAPI_VERSION is the wrong lever, because napi_get_version returns that macro and Chakra and JavaScriptCore implement none of v6/v7. So the type-tag declarations, the napi_type_tag struct and the Napi::TypeTaggable wrappers are ungated instead and NAPI_VERSION stays at 5, understating capability rather than overstating it. V8 keeps the tag under a v8::Private, which script cannot reach. QuickJS, Chakra and JavaScriptCore have no per-object native slot for an arbitrary object, so each stores the tag in a WeakMap held only on napi_env__. A hidden own property would not do: even under a symbol, Object.getOwnPropertySymbols hands script the key, and the tag could then be read off a real instance and replayed onto a spoofed object. Three unwrap holes are fixed alongside, because a type tag is only useful once unwrap itself is sound. The first two are BabylonJS#226; the third was found by the new test: - The V8 internal-field optimisation replaced a private-property lookup and dropped its IsExternal() validity check with it, so Unwrap read field 0 off any object and dereferenced it. napi_wrap had the mirror gap, writing field 0 of an object that has none. Both now require InternalFieldCount() >= 1, and Unwrap rejects the null that napi_remove_wrap leaves behind -- two integer compares, still cheaper than the private-property lookup upstream does. - QuickJS napi_unwrap searched the prototype chain, so Object.create(realInstance) resolved to the real instance's native pointer. The chain walk is gone from unwrap, remove_wrap and wrap, so only an object created by a napi class constructor can be wrapped -- the same constraint the V8 internal field already imposes. - JavaScriptCore had the identical defect one layer down: NativeInfo::Query used JSObjectHasPropertyForKey / JSObjectGetPropertyForKey, both of which search the prototype chain. It applies to references too, so two distinct objects reported the same object id. JSC's C API has no own-property accessor, so the env caches Object.prototype.hasOwnProperty, which is what the Chakra and QuickJS ports already do. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Please can you look at #189? after merging it we can it go onto proper NAPI v8 support instead of a non-standard offshoot that will keep BabylonNative users locked out of the modern NodeJS addon ecosystem. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
[Created by Copilot on behalf of @bghgary]
Context
napi_unwrapreturnsvoid*and does no type check, so it cannot answer "is this JS object one of my type". BabylonNative#1844 hand-rolls a ~100-line registry to get that answer, because the Node-API primitive for it —napi_type_tag_object/napi_check_object_type_tag— is unreachable here:NAPI_VERSIONis pinned to 5 and the whole type-tag surface sits behindNAPI_VERSION >= 8. Even the V8 body was dead code, and would not have compiled, since theNAPI_PRIVATE_KEYit calls is commented out.Worth a look
NAPI_VERSIONstays at 5; only the type-tag surface is ungated.napi_get_versionreturns that macro, so raising it to 8 would have all four ports claim v8 while Chakra and JavaScriptCore implement none of v6/v7 (BigInt,napi_get_all_property_names, instance data, ArrayBuffer detaching).env_hermes.ccalso depends on the 5.WeakMapheld only onnapi_env__. They have no per-object native slot for an arbitrary object, and a hidden own property is not private —Object.getOwnPropertySymbolshands script the key even under a symbol, and the tag can then be read off a real instance and replayed onto a spoofed one. V8 keeps usingv8::Private.napi_wrapnow rejects an object that no napi class constructor created. Wrapping one used to splice a wrapper into its prototype chain, which is what unwrap's chain walk existed to find. That is the same constraint V8's internal field already imposes.NativeInfo::Query, and it is not in napi_unwrap does not reject objects that were never wrapped (V8 port faults, QuickJS port confuses types) #226. It applies to references too: two distinct objects reported the same object id.Verification
Built and ran the full suite on all six engine configurations: V8, Chakra, QuickJS, JSI and Hermes on Windows x64, and JavaScriptCore on Ubuntu 24.04. The new
NodeApi.TypeTagstest was also run against each guard reverted in turn — V8 dies with an access violation, QuickJS and JavaScriptCore hand back the wrong object's pointer.Fixes #226.