BabylonJS / BabylonJS/JsRuntimeHost

napi_unwrap does not reject objects that were never wrapped (V8 port faults, QuickJS port confuses types)

Open
#226 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.