Automattic / Automattic/node-canvas

putImageData segfaults if the given array is too small

Open
#1,601 3 comments 1 reaction 0 assignees View on GitHub
Bug
Dominant language
JavaScript
Stars
10.7k
Forks
1.2k
Avg merge
4d 8h
Merged PRs (30d)
1

Description

## Issue or Feature
Hi all,

A story of user stupidity: I'm tinkering with raw buffers representing RGB pixels piped from `ffpmeg` for a small hobby CCTV project. My assumption for a canvas context with `{ alpha: false, pixelFormat: 'RGB24' }` was for the `createImageData`/`putImageData` functions to act in a pure RGB mode, but peeking at the C++ source it seems it needs RGBA, but then goes on and ignores the A for the underlying implementation.

That's all fine now I know, but the actual behaviour for a buffer that's 3/4 of the expected size resulted in the entire Node process terminating via a segfault. I think the expected behaviour should be some sort of bounds checking and an exception? I'm not sure whether `createImageData` or `putImageData` should be the one checking. Unfortunately my knowledge of C++ is embarrassingly low, otherwise I would offer to help!

This also doesn't show until the difference is large enough I believe. While creating the reproduction code a square of 40 pixels _didn't_ crash, but shifting it to my original 1280x720 seems to reproduce it reliably.

## Steps to Reproduce

```js
const { createCanvas, createImageData } = require('canvas')

const width = 1280
const height = 720

// Generate some RGB pixels, all of the same colour.
let rawPixels = new Uint8ClampedArray(width * height * 3).fill(200)
console.log(`Passing in ${rawPixels.length} subpixels.`)

const canvas = createCanvas(width, height)
const ctx = canvas.getContext('2d', { alpha: false, pixelFormat: 'RGB24' })

const canvasBufferSize = ctx.getImageData(0, 0, width, height).data.length
console.log(`Filling this canvas requires ${canvasBufferSize} subpixels.`)

const image = createImageData(rawPixels, width, height)
ctx.putImageData(image, 0, 0)
canvas.toBuffer()

// Segfault.
// Doing a `getImageData` shows the buffer length
// of the canvas is actually 1280 * 720 * 4.
// But I was silly and fed it 1280 * 720 * 3...
```

## Your Environment
* Version of node-canvas (output of `npm list canvas` or `yarn list canvas`): 2.6.1
* Environment (e.g. node 4.2.0 on Mac OS X 10.8): Node v10.19.0 on Ubuntu 20.04 amd64. (Also occurred on Node v14 on Windows)

(P.S. I've now implemented a loop in JS to create a new buffer with a zeroed A channel but this will have a performance penalty. Would there be interest in an additional override to `putImageData` so that feeding the A channel could be made optional? The C++ source already loops over the image and does a skip here: https://github.com/Automattic/node-canvas/blob/v2.6.1/src/CanvasRenderingContext2d.cc#L889)

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.