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

[bkaradzic-microsoft]

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<T>::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:

// 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:

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<T>::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<T>::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<T> 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.