diff --git a/CHANGELOG.md b/CHANGELOG.md index 26fb99743..e24ce13c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ project adheres to [Semantic Versioning](http://semver.org/). ### Added ### Fixed * Load images from Node.js object URLs (#2525) +* Fix crash loading an SVG with an embedded image in an undecodable format (#2613) +* Fix use-after-free when re-rendering an SVG image at a size cairo rejects (#2613) +* Fix leak of one decoded frame per parse of an SVG with an embedded JPEG, GIF or BMP (#2613) 3.2.3 ================== diff --git a/pkg/lunasvg/build.zig.zon b/pkg/lunasvg/build.zig.zon index 30ddec25c..59a6f1269 100644 --- a/pkg/lunasvg/build.zig.zon +++ b/pkg/lunasvg/build.zig.zon @@ -5,8 +5,8 @@ .dependencies = .{ .cairo = .{ .path = "../cairo" }, .lunasvg = .{ - .url = "https://github.com/chearon/lunasvg/archive/3fd8ba9e0cd5cf0af36b1bda2d0a3bbaf8bd8da7.tar.gz", - .hash = "N-V-__8AAFD4FABzdNrtG-6ulAVAPIPJZde38BQWKRcKgkgV", + .url = "https://github.com/iurisilvio/lunasvg/archive/b03b0dd7ba9967bd507c4a13ab12ff37e1d4128a.tar.gz", + .hash = "N-V-__8AAGcLFQB2sVL8Tvu9IeCPuaczmK9v1-aj16-Uasup", } }, } diff --git a/src/CanvasRenderingContext2d.cc b/src/CanvasRenderingContext2d.cc index 6183837c0..4cdc71099 100644 --- a/src/CanvasRenderingContext2d.cc +++ b/src/CanvasRenderingContext2d.cc @@ -1268,6 +1268,8 @@ Context2d::DrawImage(const Napi::CallbackInfo& info) { source_w = sw = img->surface.width; source_h = sh = img->surface.height; surface = img->surface.surface(); + // An SVG image re-renders on demand and can fail, throwing from surface(). + if (!surface) return; // Canvas } else if (obj.InstanceOf(env.GetInstanceData()->CanvasCtor.Value()).UnwrapOr(false)) { diff --git a/src/Image.cc b/src/Image.cc index 2639989b5..c0d6b37e3 100644 --- a/src/Image.cc +++ b/src/Image.cc @@ -81,10 +81,31 @@ ImageSurface::clearData() { state = DEFAULT; } +// Identifies the pixel buffer attached to a transferred surface. Only the +// address matters to cairo. +static cairo_user_data_key_t transferred_data_key; + cairo_surface_t* ImageSurface::transferSurface() { cairo_surface_t* surface = _surface; + // The JPEG, GIF and BMP decoders back the surface with a buffer of our own + // that cairo does not own. We are giving up the only pointer to it, so hand + // it to the surface before letting go. + if (surface && _data) { + cairo_status_t status = cairo_surface_set_user_data( + surface, &transferred_data_key, _data, + [](void* data) { delete[] static_cast(data); }); + + if (status != CAIRO_STATUS_SUCCESS) { + // Out of memory. The buffer would outlive every pointer to it, so drop + // the surface with it rather than leak. + cairo_surface_destroy(surface); + delete[] _data; + surface = nullptr; + } + } + if (env) Napi::MemoryManagement::AdjustExternalMemory(*env, -_data_len); _data_len = 0; _surface = nullptr; @@ -1236,9 +1257,13 @@ cairo_status_t ImageSurface::loadSVGFromBuffer(uint8_t *buf, unsigned len) { lunasvg::GraphicsCallbacks callbacks; - callbacks.setDecoderFn([](char* data, int length) { + callbacks.setDecoderFn([](char* data, int length) -> cairo_surface_t* { ImageSurface bitmap(std::nullopt); - bitmap.loadFromBuffer((uint8_t*)data, length); + // Embedded images come in whatever format the document author used, so + // failing to decode one is routine. lunasvg renders nothing for a null + // surface; handing it a half-built one would render garbage. + if (bitmap.loadFromBuffer((uint8_t*)data, length) != CAIRO_STATUS_SUCCESS) + return nullptr; return bitmap.transferSurface(); }); @@ -1251,10 +1276,15 @@ ImageSurface::loadSVGFromBuffer(uint8_t *buf, unsigned len) { if (width <= 0 || height <= 0) { this->errorInfo.set("Width and height must be set on the svg element"); + width = naturalWidth = height = naturalHeight = 0; + svgdoc = nullptr; return CAIRO_STATUS_READ_ERROR; } - return renderSVGToSurface(); + cairo_status_t status = renderSVGToSurface(); + if (status != CAIRO_STATUS_SUCCESS) svgdoc = nullptr; + + return status; } /* @@ -1265,8 +1295,16 @@ cairo_status_t ImageSurface::renderSVGToSurface() { cairo_status_t status; - if (_surface) cairo_surface_destroy(_surface); + // Null as well as destroy: every early return below leaves the caller + // holding this object, and clearData() would destroy the surface again. + if (_surface) { + cairo_surface_destroy(_surface); + _surface = nullptr; + } + lunasvg::Bitmap bitmap = svgdoc->renderToBitmap(width, height, 0); + if (bitmap.isNull()) return CAIRO_STATUS_NO_MEMORY; + status = cairo_surface_status(bitmap.surface()); if (status != CAIRO_STATUS_SUCCESS) return status; _surface = cairo_surface_reference(bitmap.surface()); diff --git a/test/svg.test.js b/test/svg.test.js new file mode 100644 index 000000000..fe502be49 --- /dev/null +++ b/test/svg.test.js @@ -0,0 +1,124 @@ +/* eslint-env mocha */ + +'use strict' + +const assert = require('assert') +const path = require('path') +const { spawnSync } = require('child_process') + +// The failure modes below abort the process (SIGSEGV / cairo assertion), so +// each one has to run in a child process for mocha to be able to report it. +// Keep the scripts small: Windows caps a command line at 32k. +function runInChild (body, nodeArgs = []) { + const src = `const canvas = require(${JSON.stringify(path.join(__dirname, '..'))})\n${body}` + const child = spawnSync(process.execPath, [...nodeArgs, '-e', src], { encoding: 'utf8' }) + if (child.error) assert.fail(`could not run the child process: ${child.error.message}`) + if (child.signal || child.status !== 0) { + assert.fail(`child process died with ${child.signal || `exit code ${child.status}`}\n` + + (child.stderr || '').trim().split('\n').slice(-3).join('\n')) + } + return child.stdout +} + +describe('SVG', function () { + it('does not crash on an embedded image it cannot decode', function () { + // A data URI that base64-decodes fine but is not a format node-canvas + // decodes (WebP here). The decode callback hands back a null surface. + const junk = Buffer.from('RIFF____WEBPVP8 not a real image, but long enough to sniff').toString('base64') + const svg = '' + + `` + + const out = runInChild(` + const img = new canvas.Image() + img.onerror = () => {} + img.src = Buffer.from(${JSON.stringify(svg)}) + console.log('survived') + `) + assert.strictEqual(out.trim(), 'survived') + }) + + it('does not double-free the surface when a re-render fails', function () { + // Rendering at a size cairo rejects (> 32767) must not leave the previous + // surface destroyed-but-not-nulled: the next render destroys it again. + // Only an assertion-enabled cairo turns that into an abort, so this needs + // a debug build -- the plain `zig build` a contributor runs before the + // tests. In a release build the same code is a silent use-after-free. + const svg = '' + + '' + + const out = runInChild(` + const img = new canvas.Image() + img.src = Buffer.from(${JSON.stringify(svg)}) + const ctx = canvas.createCanvas(64, 64).getContext('2d') + ctx.drawImage(img, 0, 0) + img.width = img.height = 40000 + for (let i = 0; i < 3; i++) { + try { ctx.drawImage(img, 0, 0) } catch (err) { /* expected: invalid size */ } + } + console.log('survived') + `) + assert.strictEqual(out.trim(), 'survived') + }) + + it('does not leak the pixel buffer of an embedded raster', function () { + this.timeout(120000) + const size = 1200 + const warmup = 50 + const measured = 100 + // W * H * 4 bytes are leaked per parse if the decoded buffer is orphaned. + const frameBytes = size * size * 4 + + // An embedded PNG is the control: cairo owns the pixels on that path, so + // whatever RSS slope it shows is this build's allocator noise. JPEG (and + // GIF and BMP) hand cairo a buffer of ours instead, and orphaning it costs + // one whole decoded frame per parse on top of that noise. + const out = runInChild(` + const size = ${size} + const raster = canvas.createCanvas(size, size) + const rctx = raster.getContext('2d') + for (let i = 0; i < 64; i++) { + rctx.fillStyle = \`hsl(\${(i * 11) % 360} 80% 50%)\` + rctx.fillRect((i * 37) % size, (i * 53) % size, 40, 40) + } + const svg = mime => Buffer.from( + \`\` + + \`') + const png = svg('image/png') + const jpeg = svg('image/jpeg') + + const load = svg => { + const img = new canvas.Image() + img.src = svg + if (img.width !== size) throw new Error('svg failed to load') + } + const drain = async () => { + for (let i = 0; i < 3; i++) { global.gc(); await new Promise(setImmediate) } + global.gc() + } + const run = async (svg, n) => { + for (let i = 1; i <= n; i++) { + load(svg) + if (i % 10 === 0) await new Promise(setImmediate) + } + await drain() + return process.memoryUsage().rss + } + const slope = async svg => { + const warm = await run(svg, ${warmup}) + const end = await run(svg, ${measured}) + return (end - warm) / ${measured} + } + ;(async () => console.log(await slope(png), await slope(jpeg)))() + `, ['--expose-gc']) + + const [png, jpeg] = out.trim().split(' ').map(Number) + assert.ok( + jpeg - png < frameBytes / 2, + `embedded JPEG retains ${Math.round(jpeg / 1024)} KiB per parse against ` + + `${Math.round(png / 1024)} KiB for the same image as PNG, a difference of ` + + `${Math.round((jpeg - png) / 1024)} KiB (one decoded frame is ` + + `${Math.round(frameBytes / 1024)} KiB)` + ) + }) +})