BabylonJS / BabylonJS/JsRuntimeHost
napi_unwrap does not reject objects that were never wrapped (V8 port faults, QuickJS port confuses types)
- Dominant language
- C++
- Stars
- 22
- Forks
- 23
- Avg merge
- 3d 14h
- Merged PRs (30d)
- 6
Description
### Summary
`napi_unwrap` must fail when it is handed an object that was never wrapped. The V8 and QuickJS ports both violate that, so `Napi::ObjectWrap::Unwrap` cannot safely be called on an object whose type has not already been established. On V8 it is an outright memory-safety hole.
This affects every `Unwrap` call site in a consumer, not one polyfill. I hit it in BabylonNative's Canvas polyfill (BabylonJS/BabylonNative#1844), where `ctx.fill(Object.create(Path2D.prototype))` was an access violation.
### V8 port — dereferences internal field 0 unchecked
`Core/Node-API/Source/js_native_api_v8.cc`, `inline napi_status Unwrap(...)` (~line 345). A `[BABYLON-NATIVE-ADDITION]` marked *"Increase perf by using internal field instead of private property"* replaced the private-property lookup, **including its validity check**:
```cpp
// upstream
auto val = obj->GetPrivate(context, NAPI_PRIVATE_KEY(context, wrapper)).ToLocalChecked();
RETURN_STATUS_IF_FALSE(env, val->IsExternal(), napi_invalid_arg);
```
with a bare `obj->GetAlignedPointerFromInternalField(0)`, whose result is then dereferenced (`reference->Data()`).
For any object that is not a wrapped instance, internal field 0 is not a `Reference*`. The read returns garbage and the dereference faults. Reproduction:
```js
const impostor = Object.create(Path2D.prototype);
ctx.fill(impostor); // 0xC0000005
```
I confirmed this in a local V8 build: deterministic access violation, and it disappears when the unwrap is replaced with a checked lookup.
### QuickJS port — walks the prototype chain
`Core/Node-API/Source/js_native_api_quickjs.cc`, `napi_unwrap` (~line 2495). After the fast path on `js_wrap_class_id` there is a *"Fallback: search the prototype chain for a legacy wrapper object"*.
That returns **some other object's** native pointer — a type confusion rather than a crash. An object created with `Object.create(RealType.prototype)` unwraps to whatever instance is reachable on the chain. It does at least return `napi_invalid_arg` when nothing is found.
### JSI port
No C API, and `ObjectWrap::Unwrap` returns `nullptr` for a non-wrapped object (`napi-inl.h:2268`) rather than throwing. Safe, but inconsistent with the other two.
### Why this is hard to work around downstream
While fixing the Canvas case I found no portable way to do a type check:
- `napi_type_tag_object` / `napi_check_object_type_tag` exist only in `js_native_api_v8.cc`.
- `Napi::Object::DefineProperty` / `PropertyDescriptor` are absent from the JSI port, so a non-enumerable brand cannot be installed.
- `GetInstanceData` / `SetInstanceData` / `AddCleanupHook` are absent from the JSI port, so there is nowhere to keep per-`Env` C++ state.
- `Napi::ObjectWrap::Value()` **throws** on the QuickJS port, which rules out an identity check against a candidate instance.
I ended up branding each instance with a `Napi::External` and validating the pointer against a registry of live addresses. That works, but every consumer having to invent this is a strong argument for fixing the ports.
### Suggested fix
Restore the validity check in the V8 port (keep the internal-field fast path, but verify the field actually holds the wrapper before dereferencing), and drop the prototype-chain fallback in the QuickJS port so a non-wrapped object returns `napi_invalid_arg`.
Related: #225 (the `napi_throw` family reports failure on success), found while chasing the same PR.
Contributor guide
Research direction
Start by reading inline napi_status Unwrap in Core/Node-API/Source/js_native_api_v8.cc and napi_unwrap in Core/Node-API/Source/js_native_api_quickjs.cc. Verify the V8 path checks its internal field before dereferencing and that QuickJS rejects objects without a wrapper instead of walking the prototype chain; done means both ports return napi_invalid_arg for never-wrapped objects without unsafe access or type confusion.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, javascript
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100