Canvas: implement getImageData and fix drawImage crash on plain ImageBitmap

[bkaradzic-microsoft]

Two Canvas 2D fixes that together make the drawImage → getImageData round trip work, plus input validation for the new entry points.

1. getImageData returned all zeros

ImageData::GetData was a stub:

// return a well size array with 0
// TODO: Get datas from context/canvas

It allocated a buffer and memset it to 0, so every getImageData() call read back a fully transparent image no matter what had been drawn. Context::GetImageData also discarded the source x/y arguments (// TODO: support source x and y).

BN's canvas is GPU-backed (nanovg), and a framebuffer readback is a poor fit for the data-texture callers that rely on getImageData: the surface is premultiplied and may be resampled, so the bytes that come back are not the bytes that went in. Instead this keeps a CPU-side RGBA8 mirror — drawImage copies the decoded image's exact pixels (Image::GetPixels → bimg m_data) into Context::m_cpuPixels, and getImageData reads that region back out. BlitPixelsToCpu covers all three drawImage arities with nearest-neighbour sampling, which is exact for the 1:1 draws these callers use. The mirror is reallocated and cleared on canvas resize.

Also fixes a pre-existing ImageData constructor bug that read the height from info[1] instead of info[2], so a non-square ImageData reported its width as its height.

2. drawImage access violation on a plain ImageBitmap

Babylon Native sets forceBitmapOverHTMLImageElement, which routes LoadImage through NativeEngine::CreateImageBitmap. That returns a plain JS object {data, width, height, format} — not a wrapped NativeCanvasImage. Context::DrawImage called NativeCanvasImage::Unwrap on whatever it was handed, so napi_unwrap dereferenced a never-wrapped object and crashed with an access violation (0xC0000005).

DrawImage now detects the plain ImageBitmap by its own data typed array, converts its pixels to RGBA8 via bimg::imageConvert (memcpy fast path when the source is already RGBA8), creates a transient nanovg image, and shares the arity-3/5/9 draw plus CPU-mirror blit with the existing path through a new DrawImageCommon helper. The NativeCanvasImage path is behaviour-identical.

3. Input validation

Both new paths take sizes straight from JS and hand them to bimg and to raw pointer arithmetic, so the third commit validates them at the boundary:

• drawImage rejects an out-of-range bimg texture format, dimensions whose pixel count would overflow the destination allocation, and a data array shorter than width*height for its format. bimg::imageConvert is given neither buffer's real length, so without these it would read past the end of the source.
• getImageData requires its four arguments and rejects a region whose byte size would overflow size_t before ImageData tries to allocate it.
• BlitPixelsToCpu clamps its iteration range to the destination rect's intersection with the canvas up front. The destination size is caller-controlled and arrives unsigned, so a negative width wraps to ~4e9; the previous per-pixel bounds test still walked the whole rect and could stall the JS thread for billions of no-op iterations.

Range checks are computed in uint64_t so they stay meaningful where size_t is 32-bit (Android armeabi-v7a), and the new arithmetic uses fixed-width types rather than long, whose width differs between MSVC and Apple clang.

Validation

• "Displacement map" (idx 200) — Mesh.applyDisplacementMap is exactly this drawImage → getImageData round trip. It crashes on master; with this change it renders at 0.155% pixel difference against a 2.5% limit. It is still excludeFromAutomaticTesting in config.json, so reproduce with --include-excluded --test-index=200. Un-excluding it is deliberately left to a separate test-enablement PR.
• Full validation suite: ran=295 passed=295 failed=0 — no regressions.

Why 295 and not 300

The 5 tests at indices 53–57 (Scissor test, Scissor test with 0.9 hardware scaling, … with 1.5 hardware scaling, … with negative x and y, … with out of bounds width and height) are excluded from that run because they crash on unmodified master, not because of anything here:

--- BN: CRASH ---
Unknown signal? Exception Code 87a
  7: bgfx/src/renderer_d3d11.cpp  6173  bgfx::d3d11::RendererContextD3D11::submit
  8: bgfx/src/bgfx.cpp            2917  bgfx::Context::renderFrame
 13: Core/Graphics/Source/DeviceImpl.cpp  539  Babylon::Graphics::DeviceImpl::Frame

Exit code 3, Win32 x64 D3D11 Debug. Verified pre-existing by reverting Polyfills/Canvas to master and rebuilding — identical crash at the same test. (0x87a is the DXGI 0x887A… device-removed family.) The duplicate Scissor test at idx 311 passes. Worth noting because it truncates any full-suite run at 52 tests; happy to file it separately.

Not included

My working branch also un-excluded idx 106 "Gaussian Splatting Part Test" alongside this change. That does not hold on stock upstream, so it is left out: Babylon's SOG loader unpacks the .sog container and materialises its WebP images through URL.createObjectURL, and upstream JsRuntimeHost has no blob-URL support, so the load fails before any canvas work happens. That un-exclusion needs a JsRuntimeHost change first.

There are no config.json changes in this PR — it is native-only and enables no tests.

[Copilot]

Pull request overview

This PR fixes Canvas 2D interoperability in Babylon Native by making getImageData() return meaningful pixel data (via a CPU-side RGBA8 mirror populated from drawImage) and by preventing a crash when drawImage is called with Babylon’s “plain object” ImageBitmap ({data,width,height,format}) produced by NativeEngine.createImageBitmap.

Changes:

• Implement getImageData() by copying from a CPU-side pixel mirror and fix ImageData’s constructor argument indexing for non-square sizes.
• Update drawImage() to safely handle plain ImageBitmap objects (convert to RGBA8, create transient NVG image, and share common draw/blit path).
• Add boundary validation to reduce the risk of overflow/OOB during pixel conversion and image-data allocation.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Polyfills/Canvas/Source/ImageData.h	Extends ImageData to carry a pixel buffer and accept sx/sy for region reads.
Polyfills/Canvas/Source/ImageData.cpp	Populates ImageData from Context::ReadPixels, fixes height parsing bug, and returns real pixel data instead of zeros.
Polyfills/Canvas/Source/Image.h	Adds GetPixels() API to expose decoded RGBA8 bytes for CPU mirroring.
Polyfills/Canvas/Source/Image.cpp	Implements GetPixels() backed by the decoded bimg::ImageContainer.
Polyfills/Canvas/Source/Context.h	Introduces CPU-side mirror storage and helper methods (ReadPixels, BlitPixelsToCpu, DrawImageCommon).
Polyfills/Canvas/Source/Context.cpp	Implements CPU mirror maintenance/readback, adds ImageBitmap draw path, and validates getImageData inputs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[CedricGuillemet]

LGTM

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

The two fixes are the right ones and the validation work is thorough, but the ImageBitmap path has a memory-safety problem that should block: the pixel-format guard does not work on MSVC, so a JS-supplied format in the high half of uint32_t becomes a negative index into bimg's global tables, including an indirect call through an out-of-bounds function pointer. Details and a repro inline.

Two related ones on the same path - the plain-object check leaves every other object shape falling through to an unvalidated Unwrap, and the typed-array pointer is captured before three property reads that can run attacker JS. Both are made reachable in a new way by the CPU mirror, since getImageData now returns real bytes instead of zeros.

The rest is correctness and cost: the mirror ignores transform, compositing, globalAlpha and clipping, and several copies look avoidable.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

The mirror is written only by the three BlitPixelsToCpu calls in DrawImageCommon, using the raw destination coordinates and a straight byte assignment. So it diverges from what actually gets rendered in more ways than just the missing draw calls:

• Transform - translate/rotate/scale/setTransform go to nanovg, but BlitPixelsToCpu gets the raw dx/dy arguments. ctx.translate(10, 10); ctx.drawImage(img, 0, 0) renders at (10, 10) and mirrors at (0, 0). That breaks the 1:1 drawImage -> getImageData round trip this is built for, not just the general case.
• Compositing - drawImage blends source-over (the NVG_COPY in ClearRect is scoped to that call), while the mirror replaces RGBA including alpha. Overlapping draws, or any image with transparent margins, differ.
• globalAlpha - applied by nanovg, ignored by the mirror.
• Clipping - nvgScissor bounds the GPU draw; the mirror writes the full rect.
• Everything that is not drawImage - fillRect, fillText, paths and gradients never reach the mirror, and clearRect leaves stale pixels in it.

Previously getImageData was uniformly zeros, so nobody could mistake it for working. Now it is exact on one narrow path and silently wrong elsewhere, which is harder to catch. Worth deciding whether the mirror tracks the full 2D state, or whether getImageData should fail loudly once anything it cannot mirror has touched the canvas.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

DrawImageCommon throws on an unexpected argument count, so drawImage(bitmap, 1, 2, 3) skips nvgDeleteImage and leaks the nanovg image and its texture on every call. The NativeCanvasImage path is unaffected because it caches into m_nvgImageIndices. Worth scoping the handle so it is released on the throw path.

Three smaller things in the same block:

• nvgCreateImageRGBA's result is not checked. On failure the draw silently produces nothing while BlitPixelsToCpu still writes the mirror, so the two disagree. The NativeCanvasImage path at least asserts.
• Every call creates and destroys a GPU texture for the same bitmap. That is fine for a one-shot applyDisplacementMap, but anything drawing the same ImageBitmap per frame pays a full upload and teardown each time, where the NativeCanvasImage path caches.
• std::vector<uint8_t> rgba(pixelCount * 4) zero-fills the buffer before it is completely overwritten on both branches. For a 2048x2048 bitmap that is 16 MB of zeroing on top of the 16 MB copy.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

sx -= static_cast<int32_t>(sw) can overflow: getImageData(-2000000000, 0, -2000000000, 1) clears the INT32_MIN guard, then subtracts 2e9 from -2e9. ReadPixels bounds-checks every pixel so nothing is read out of range, but it is still signed overflow; computing the normalized origin in int64_t avoids it.

Separately, the only bound on the region is the size_t overflow check, so getImageData(0, 0, 2000000000, 1) allocates 8 GB in m_pixels, and GetData copies it again. Worth a ceiling?

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

This inner loop does a 64-bit integer division per pixel (i * sw / dw) and four separate byte stores, which the compiler generally will not merge into one 32-bit store through std::vector<uint8_t> indexing.

For the 1:1 draws this is aimed at, none of it is needed: when dw == sw and dh == sh, srcX is just sx + i, so each row is a contiguous run and the whole inner loop is one memcpy of runLength * 4. Worth special-casing that and keeping the sampling loop for the scaled case - and even there, hoisting the division out with an accumulator.

ReadPixels has the same shape: clipping the rect up front lets each row be a memcpy of w * 4, which also drops the two per-pixel bounds checks and makes the full-buffer memset unnecessary except for the out-of-range margin.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

m_pixels only exists to be copied out of. ReadPixels fills it, then every .data access allocates a fresh Uint8Array and copies it again - so one getImageData plus one .data read walks the pixels three times (memset, mirror -> m_pixels, m_pixels -> JS) where once would do, and the buffer stays resident for the object's lifetime alongside every array handed out.

Having ReadPixels write straight into the typed array, created once, removes the intermediate.

Copying per access also diverges from the browser, where .data is a stable live Uint8ClampedArray - id.data === id.data, and mutating it then calling putImageData is the normal round trip. Here each read is a detached copy, so writes are silently dropped. That predates this PR, but it only starts to matter now the data is real rather than always zero.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

This guard does not work on MSVC, and it is the check the whole ImageBitmap path rests on.

bimg::TextureFormat::Enum is an unscoped enum with no fixed underlying type and enumerators in [0, 104], so MSVC picks signed int. static_cast<Enum>(0xFFFFFFFFu) is therefore -1, and format >= bimg::TextureFormat::Count is false. Every format in [0x80000000, 0xFFFFFFFF] becomes an attacker-chosen negative index. Compiled against the repo's toolchain (MSVC 17.0 x64, /O2 /std:c++17):

sizeof=4 signed=1 Count=96
raw=0xFFFFFFFF as_int=-1           guard rejects? NO
raw=0x80000000 as_int=-2147483648  guard rejects? NO
raw=0x000000C8 as_int=200          guard rejects? YES

bimg then indexes its global tables with no bounds check of its own - s_imageBlockInfo[_format] (image.cpp:282/287/292) and, in the convert path, s_packUnpack[_srcFormat].unpack (image.cpp:1234) which is then called. So a negative index gives an out-of-bounds read of s_imageBlockInfo at an attacker-chosen offset, an attacker-controlled srcBpp (and therefore source stride), and an indirect call through a function pointer read out of bounds. It also defeats the requiredBytes check below, since that value is derived from the same table read - a small non-zero result lets imageConvert read far past the end of a small Uint8Array, and the result reaches JS through the now-working getImageData.

GCC and Clang on non-MS ABIs select unsigned int, so the comparison holds there. This is Windows/UWP-specific, and both are shipped targets. The cast is formally UB either way.

ctx.drawImage({ data: new Uint8Array(64), width: 1, height: 1, format: 0xFFFFFFFF }, 0, 0);

Validate the raw uint32_t before it becomes an enum:

Suggested change

	if (format >= bimg::TextureFormat::Count)
	{
	throw Napi::Error::New(info.Env(), "drawImage: ImageBitmap has an out-of-range pixel format.");
	}
	const uint32_t rawFormat = imageObj.Get("format").As<Napi::Number>().Uint32Value();
	if (rawFormat >= static_cast<uint32_t>(bimg::TextureFormat::Count))
	{
	throw Napi::Error::New(info.Env(), "drawImage: ImageBitmap has an out-of-range pixel format.");
	}
	const auto format = static_cast<bimg::TextureFormat::Enum>(rawFormat);

Worth rejecting Unknown and UnknownDepth too.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

This closes the crash for one object shape and leaves every other shape falling through to NativeCanvasImage::Unwrap below - and the unwrap does not validate what it is handed. JsRuntimeHost's napi fork replaced upstream's IsExternal() check with a raw internal-field read:

// js_native_api_v8.cc:361-367
// [BABYLON-NATIVE-ADDITION]: Increase perf by using internal field instead of private property
Reference* reference =
    static_cast<v8impl::Reference*>(obj->GetAlignedPointerFromInternalField(0));

DefineClass sets SetInternalFieldCount(1) on every napi ObjectWrap class with no type tag, so passing any other wrapped object returns a valid pointer to an unrelated C++ object reinterpreted as NativeCanvasImage*. That is deterministic type confusion rather than a crash. ImageData lines up particularly well: its m_width/m_height sit at the same offsets and are attacker-set via getImageData(0, 0, W, H), while m_imageContainer - the last member of the larger NativeCanvasImage - reads past the end of the smaller object into groomable heap. CreateNVGImageForContext then does nvgCreateImageRGBA(nvg, m_width, m_height, 0, m_imageContainer->m_data), and GetPixels() feeds the same pointer to BlitPixelsToCpu.

The reason this matters more in this PR than before it: previously getImageData returned zeros, so a wild read was unobservable. The CPU mirror turns it into a read-back channel.

ctx.drawImage(ctx.getImageData(0, 0, 64, 64), 0, 0);
ctx.getImageData(0, 0, 64, 64).data;

Worth identifying the argument positively - InstanceOf against the persisted Image constructor, or a napi_type_tag - and throwing a TypeError otherwise, rather than duck-typing on data. Note drawImage(canvasElement, ...) is legitimate Canvas2D, so ordinary content reaches this path.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Napi::Uint8Array caches the backing-store pointer and length at construction - Data() and ByteLength() are plain accessors that never re-query the engine. data is captured on the first line here, and the three Get(...) calls that follow are full property lookups that run accessors and Proxy traps, so attacker JS executes after the pointer and length are frozen. The memcpy and imageConvert below then use the stale pair.

const buf = new ArrayBuffer(BIG);
const view = new Uint8Array(buf);
ctx.drawImage({
  data: view,
  get width()  { buf.transfer(BIG + 1); return W; },  // detaches, frees old store
  get height() { return H; },
  get format() { return 76; },
}, 0, 0);
ctx.getImageData(0, 0, W, H).data;

ArrayBuffer.prototype.transfer with a changed length allocates a new store and detaches the old one, dropping the last reference. The stale _length still satisfies the requiredBytes check while _data dangles, so the copy reads freed heap into rgba - and out through getImageData.

Reading the scalars first and fetching data last would close it, with no JS-invoking call between capturing Data()/ByteLength() and the copy. A napi_is_detached_arraybuffer check immediately before use would make it explicit.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

The rationale here is wrong for the pinned bimg: imageGetSize takes uint32_t _width, uint32_t _height (bimg.h) and accumulates in uint64_t - the uint16_t in that signature is _numLayers. So nothing is truncated to 16 bits and the "65537x1 comes back as 1x1" example cannot happen.

The cap itself is harmless, but the comment presents it as the load-bearing check against a truncation that does not exist. Worth either dropping it or rewriting the reason.