Canvas: reject foreign objects instead of unwrapping them as a Path2D or gradient - #1844
Canvas: reject foreign objects instead of unwrapping them as a Path2D or gradient#1844bkaradzic-microsoft wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Canvas2D polyfill’s Path2D interop by preventing unsafe ObjectWrap::Unwrap calls on non-Path2D values, aligning fill, stroke, and addPath argument handling with browser behavior and adding regression coverage.
Changes:
- Added
NativeCanvasPath2D::IsInstance(mirroringCanvasGradient::IsInstance) and used it to gate allNativeCanvasPath2D::Unwrapcall sites. - Updated
Context2D.fill,Context2D.stroke, andPath2D.addPathto throwTypeErrorfor non-Path2Darguments (while preserving the intendedPath2Dconstructor behavior for non-Path2Dinputs by stringifying them as path data). - Added unit tests covering the previously-unsafe argument forms and the still-valid overload forms.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Polyfills/Canvas/Source/Path2D.h | Declares NativeCanvasPath2D::IsInstance for safe instance checking prior to Unwrap. |
| Polyfills/Canvas/Source/Path2D.cpp | Implements IsInstance, fixes Path2D constructor routing, and adds addPath argument validation before unwrapping. |
| Polyfills/Canvas/Source/Context.cpp | Adds Path2D instance checks (and TypeErrors) to fill/stroke prior to unwrapping. |
| Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts | Adds regression tests for invalid/valid fill/stroke/addPath argument forms and Path2D ctor behavior. |
| Apps/UnitTests/JavaScript/dist/tests.javaScript.all.js | Updates built test bundle corresponding to the new/updated TS tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // the global is writable, so script can replace it, and this has to answer whether the | ||
| // object is safe to Unwrap, not whether it matches whatever Path2D currently names. | ||
| const auto constructor = JsRuntime::NativeObject::GetFromJavaScript(env).Get(JS_PATH2D_CONSTRUCTOR_NAME); | ||
| return constructor.IsFunction() && value.As<Napi::Object>().InstanceOf(constructor.As<Napi::Function>()); |
There was a problem hiding this comment.
You are right, and it is worse than a type-confusion risk -- both directions are a hard crash. Fixed in 4b37ddf.
I reproduced it before changing anything. Object.setPrototypeOf(gradient, Path2D.prototype) makes gradient instanceof Path2D true, and ctx.fill(gradient) then exits with 0xC0000005 on Win32 D3D11. Object.create(Path2D.prototype) passes the same check while having no native wrap behind it at all.
Checking the neighbouring type on your hint that a gradient was the vehicle: CanvasGradient::IsInstance had the identical gate, and it is reachable the other way round -- ctx.fillStyle = spoofedPath2D stores the impostor, and the next fill does CanvasGradient::Unwrap(...)->Paint(). Also 0xC0000005. Fixed both.
On the mechanism, I went through the options you implied:
napi_type_tag_objectis the idiomatic answer, but only the V8 port implements it in this tree; the Chakra, JavaScriptCore and QuickJS ports do not, so it is not usable here.- A brand property is forgeable even under a symbol, since
Object.getOwnPropertySymbolshands the symbol to script, which can then define it on any object.
So the authority has to live somewhere script cannot reach, which means C++. NativeInstanceRegistry<T> records the address of every live instance; a candidate is accepted only when its unwrapped pointer is one of them. The pointer is compared and never dereferenced before it is accepted, so a foreign wrapped object is rejected rather than misread, and a reused address cannot alias because entries are removed in the destructor. It also calls napi_unwrap directly instead of ObjectWrap::Unwrap, which throws for an object that was never wrapped -- that is what closes the Object.create case.
Both crashes now have regression tests, and they are the prototype-spoofed-native-object tests you asked for. Full run is 21/21 gtest suites and 49 JS assertions, up from 47. I also ran the 305-test visual sweep because the gradient draw path changed: 305/305, unchanged.
There was a problem hiding this comment.
Follow-up: the first version of this fix reached the instance pointer through ObjectWrap::Unwrap, and that turned out not to be safe to call on an object that might never have been wrapped. Neither JsRuntimeHost Node-API port honours that part of the contract:
- the V8 port dereferences internal field 0 unconditionally (
[BABYLON-NATIVE-ADDITION]injs_native_api_v8.cc), so a plain object access-violates; - the QuickJS port falls back to walking the prototype chain, so it returns some other object's native pointer.
Object.create(Path2D.prototype) hit exactly this and crashed the V8 and QuickJS unit test runs.
So the check no longer calls Unwrap at all: each instance brands its own JS object with an External holding its address, and a candidate is accepted only if that address is still registered natively. Externals are opaque to script, and the address is compared, never dereferenced, before it is accepted.
An identity check against the instance's own object would also close brand-copying, but ObjectWrap::Value() throws on the QuickJS port, so it is not portable. It is not a memory-safety gap: copying a brand off a real instance yields that live instance, which the script already held, and a brand left over from a collected instance is rejected because the destructor unregisters the address.
Verified on Chakra, V8 and QuickJS: 21/21 gtest, 49 assertions each. Reverting only the C++ change reproduces the access violation. Visual sweep 305/305.
I'll file the napi_unwrap contract violations against JsRuntimeHost separately, since every ObjectWrap::Unwrap call site in BabylonNative has the same exposure.
There was a problem hiding this comment.
Filed the promised follow-up: BabylonJS/JsRuntimeHost#226 covers the napi_unwrap contract violations (V8 port dereferences internal field 0 unchecked; QuickJS port falls back to walking the prototype chain).
Chasing the remaining CI failure on this PR turned up a second, separate port bug in the same area — BabylonJS/JsRuntimeHost#225, where the napi_throw family returns napi_pending_exception after a successful throw. That one makes every native throw on QuickJS escape WrapCallback and get rebuilt from e.what() after its handle scope has closed, which is the segfault in Ubuntu_Clang_QuickJS.
|
The remaining What the core dump says. The job uploaded a core this time. The failing callback is Root cause. The QuickJS port's This affects every native throw on QuickJS, not just Canvas. I instrumented that catch block locally: all ~50 native throws in the unit-test run escape It also explains the Fix: BabylonJS/JsRuntimeHost#225. Verified there with an A/B on Linux QuickJS: without the change So this PR is blocked on JsRuntimeHost#225 landing plus a pin bump, the same way #1835 waited on JsRuntimeHost#223. Everything else here is green (31/32), and the Canvas work itself is verified on Chakra, V8 and QuickJS locally (21/21 gtest, 49 assertions each) with a 305/305 visual sweep. |
… them as Path2D/gradient # Conflicts: # Polyfills/Canvas/Source/Context.cpp
bac7ca0 to
68300c1
Compare
|
Confirming the loop is closed: BabylonJS/JsRuntimeHost#225 is merged, the pin here now points at No change was needed to the Canvas code for that failure; it was the QuickJS Node-API port reporting failure from a successful |
bghgary
left a comment
There was a problem hiding this comment.
[Reviewed by Copilot on behalf of @bghgary]
The type check belongs in JsRuntimeHost rather than here — comment inline. The call-site gating, the arity check and the tests all stand.
Not a complete review; holding the smaller points until the mechanism settles.
| // field 0 unconditionally, which access-violates, and the QuickJS port falls back to | ||
| // walking the prototype chain, which returns some other object's pointer. | ||
| // | ||
| // So each instance brands its own JS object with an External holding its address, and a |
There was a problem hiding this comment.
napi_type_tag_object is the Node-API answer to this, and it is unusable here only because three of the four ports do not implement it. V8 does (js_native_api_v8.cc:2556), storing the tag under a private key — invisible to script, absent from the prototype chain, not copyable onto another object. Implementing it in the QuickJS, Chakra and JavaScriptCore ports is the fix, and this file then deletes: Napi::Object::CheckTypeTag before Unwrap rejects both spoofs, with no brand property on the object and no process-wide set or mutex.
JsRuntimeHost#226 alone is not enough. It closes Object.create(Path2D.prototype), which was never wrapped, but a correct napi_unwrap still returns void* — so a CanvasGradient wearing Path2D.prototype is genuinely wrapped, unwraps successfully, and is used as a path. Only a tag distinguishes one wrapped type from another.
There was a problem hiding this comment.
[Responded by Copilot on behalf of @bghgary]
We're picking up the JsRuntimeHost side — type tags in the QuickJS, Chakra and JavaScriptCore ports, plus the napi_unwrap guards from JsRuntimeHost#226. Flagging it so you don't start the same work; happy to hand it over if you'd rather take it, since you're already in that code.
This PR then waits on that landing and a pin bump.
There was a problem hiding this comment.
Agreed — type tags are the right mechanism, and NativeInstanceRegistry.h deletes in favour of CheckTypeTag before Unwrap.
Your correction on #226 is right, and I'd overstated it: it only closes the never-wrapped Object.create case. A genuinely wrapped CanvasGradient wearing Path2D.prototype unwraps successfully no matter how correct napi_unwrap is. My registry separates the two types only because it is instantiated per T; the general form of that belongs in the port, not in Canvas.
One thing worth knowing before you start, because it makes this four ports rather than three: the V8 implementation is currently dead code.
js_native_api.h:4 carries a [BABYLON-NATIVE-ADDITION]:
#ifndef NAPI_VERSION
#define NAPI_VERSION 5
#endifThat shadows upstream's default of 8, so the #if NAPI_VERSION >= 8 guard beginning at js_native_api_v8.cc:2555 never compiles. Confirmed against the build rather than by reading: in napi.lib (V8, RelWithDebInfo) napi_type_tag_object and napi_check_object_type_tag are both absent, while napi_unwrap and napi_create_external are present.
Raising NAPI_VERSION alone will not build it. NAPI_PRIVATE_KEY is commented out at js_native_api_v8_internals.h:79:
// [BABYLON-NATIVE-ADDITION]: Increase perf by using internal field instead of private property
//#define NAPI_PRIVATE_KEY(context) \
// (v8::Private::New(context->GetIsolate()))and no other definition exists in the tree, so the uses at 2565 and 2595 are unresolved. The internal-field route that comment points at needs a free slot and there isn't one: SetInternalFieldCount(1) at js_native_api_v8.cc:929, with field 0 already holding the Reference (363, 598). So it is restore the private key for tags, go to two internal fields, or carry the tag on the Reference.
Worth deciding deliberately either way: NAPI_VERSION 5 gates every other post-5 API as well, so bumping it is a wider change than type tags alone.
On the handoff — happy for you to take it, and I'll stay off it. If you'd rather split, I'll take the QuickJS port, since I was just in that file for #225.
One question on the interim. Both spoofs are a hard 0xC0000005 reachable from script on master today, and the tagging work is now larger than it looked. Would you like me to land the uncontroversial part of this PR first — the call-site gating, the addPath arity check and the tests — which closes the original four holes via InstanceOf, and leave the two prototype-spoof crashes to be closed properly by the tag? That keeps the spoof tests in the suite (failing-but-known, or skipped) rather than sitting on a branch. Or I'm equally happy to hold the whole PR until the tag lands, if you'd prefer one clean change.
There was a problem hiding this comment.
[Responded by Copilot on behalf of @bghgary]
Hold the whole PR — one clean change once the tag lands. Nothing is blocked on it, and reaching either spoof takes script that deliberately re-prototypes a canvas object; nothing in Babylon.js does that.
We'll take all four ports.
There was a problem hiding this comment.
Understood — holding it. Agreed on the risk call: both spoofs need script that deliberately re-prototypes a canvas object, so there's nothing to race here.
I'll leave the branch rebased and green, and when the tags land I'll swap NativeInstanceRegistry.h for CheckTypeTag, bump the pin, and re-run. The two spoof tests stay in the suite as the regression cover for the port work.
Marking this a draft in the meantime so it doesn't sit in your review queue. Ping me if you want the QuickJS port after all.
68300c1 to
a1aab5e
Compare
|
Rebased onto The one conflict was against #1843: Re-verified after the rebase rather than assuming: 21/21 gtest suites, all the new spoof/gradient tests passing, and the visual sweep still Still blocked on the type-tag work as discussed above — the rebase is just to keep it mergeable. |
a1aab5e to
7e0caab
Compare
ObjectWrap::Unwrap does no type checking, so handing it an object that is
not a NativeCanvasPath2D reinterprets unrelated memory as one. Four call
sites reached it without establishing that:
ctx.stroke(x) no check at all -- ctx.stroke("x") cast a string
ctx.fill(x) gated on IsObject(), so ctx.fill({}) still got through
new Path2D(x) gated on IsObject(), same hole
path.addPath(x) no type check and no arity check, so addPath() also
unwrapped a missing argument
Add NativeCanvasPath2D::IsInstance, mirroring CanvasGradient::IsInstance,
and check it before every Unwrap. It tests against the constructor kept on
the native object rather than the global one, because the global is
writable and the question being asked is whether the object is safe to
unwrap, not whether it matches whatever Path2D currently names.
fill/stroke/addPath now throw TypeError for an argument that is not a
Path2D, as browsers do. The legal forms are unchanged: fill() and stroke()
with no argument, an explicit undefined (which selects the no-argument
overload), a fill rule string, and a Path2D. Per the (Path2D or DOMString)
union, new Path2D(x) converts a non-Path2D to a string and parses it as
path data rather than throwing.
A C++ Napi::TypeError surfaces as a JS TypeError on Chakra and V8 but as "InternalError: Uncaught C++ exception" on the QuickJS Node-API port, so asserting the constructor fails the Ubuntu_Clang_QuickJS job. Assert only that the call throws, which is what every other throw test in this suite already does. Also split addPath's arity check from its type check so a missing argument no longer reports that the first argument has the wrong type.
The IsInstance gates added earlier tested `instanceof` against the stored
constructor. That only walks the prototype chain, and a prototype is
assignable from script, so the check they were meant to make was still
bypassable:
Object.setPrototypeOf(gradient, Path2D.prototype);
ctx.fill(gradient); // unwraps a CanvasGradient as a NativeCanvasPath2D
Both directions crash with an access violation, confirmed on Win32 D3D11:
the Path2D case through fill/stroke/addPath, and the gradient case through
`ctx.fillStyle = spoofedObject` followed by any fill. Object.create with
the right prototype gets through the same way while having no native wrap
behind it at all.
napi_type_tag_object would be the idiomatic fix, but only the V8 port
implements it -- Chakra, JavaScriptCore and QuickJS do not -- and a brand
property is reachable through Object.getOwnPropertySymbols and copyable
onto any object. So the authority moves into C++, where script cannot
reach it: NativeInstanceRegistry records the address of every live
instance, and a candidate is accepted only when its unwrapped pointer is
one of them. The pointer is compared, never dereferenced, before being
accepted, so a foreign wrapped object is rejected instead of misread.
napi_unwrap is called directly rather than through ObjectWrap::Unwrap,
which throws for an object that was never wrapped.
The registry added in the previous commit still reached the instance pointer through ObjectWrap::Unwrap, which is not safe to call on an object that may never have been wrapped. Neither JsRuntimeHost port honours that contract: the V8 port dereferences internal field 0 unconditionally and access-violates, and the QuickJS port falls back to walking the prototype chain and returns some other object's pointer. Object.create(Path2D.prototype) crashed the V8 and QuickJS unit test runs for exactly this reason. Each instance now brands its own JS object with an External holding its address, and a candidate is accepted only if that address is still registered. Externals are opaque to script and the address is compared, never dereferenced, before it is accepted. Verified on Chakra, V8 and QuickJS: 21/21 gtest, 49 assertions. Reverting only the C++ change reproduces the access violation. Visual sweep 305/305. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
7e0caab to
6e1444c
Compare
Follow-up to #1824, as agreed in review.
ObjectWrap::Unwrapdoes no type checking -- it reinterprets the object's internal pointer as the target type. Four call sites reached it without establishing that the object was actually aNativeCanvasPath2D:ctx.stroke(x)ctx.stroke("x")cast a string straight intoUnwrapctx.fill(x)IsObject()ctx.fill({})still got throughnew Path2D(x)IsObject()path.addPath(x)addPath()unwrapped a missing argumentstrokewas the worst, as noted in review:Change
The check has to be one script cannot forge.
instanceofis not, because a prototype is assignable:Both directions are an access violation, reproduced on Win32 D3D11.
Object.create(Path2D.prototype)gets through the same way while having no native wrap behind it at all.CanvasGradienthad the identical gate, so it is fixed here too.napi_type_tag_objectwould be the idiomatic answer, but only the V8 port implements it in this tree -- Chakra, JavaScriptCore and QuickJS do not. A brand property is forgeable even under a symbol, sinceObject.getOwnPropertySymbolshands the symbol to script.So the authority lives in C++, where script cannot reach it.
NativeInstanceRegistry<T>records the address of every live instance, and a candidate is accepted only when its unwrapped pointer is one of them. The pointer is compared and never dereferenced before being accepted, so a foreign wrapped object is rejected rather than misread, and a reused address cannot alias because entries are removed in the destructor. It callsnapi_unwrapdirectly rather thanObjectWrap::Unwrap, which throws for an object that was never wrapped -- that is what closes theObject.createcase.fill,strokeandaddPathnow throwTypeErrorfor an argument that is not aPath2D, as browsers do, andaddPath()reports a missing argument separately from a wrong-typed one. An unusablefillStyle/strokeStyleassignment leaves the previous value in place, as the spec requires.The legal forms are unchanged and covered by a test:
fill()/stroke()with no argument, an explicitundefined(which selects the no-argument overload rather than being a badPath2D), a fill rule string,fill(path),fill(path, rule)andstroke(path).new Path2D(x)does not throw. Per the(Path2D or DOMString)union a non-Path2Dis converted to a string and parsed as path data, sonew Path2D({ toString() { return "M0 0 L10 10"; } })works andnew Path2D({})yields an empty path.Validation
ran=305 passed=305 failed=0; "Native Canvas" unchanged at 1.850%.0xC0000005before the fix.Throw assertions deliberately do not name the error type: a C++
Napi::TypeErrorsurfaces as a JSTypeErroron some engines and as anInternalErroron the QuickJS Node-API port, so only the fact that it throws is portable. Every other throw test in this suite does the same.