From ca99578fec64b96aa84197eca914f3dba71c1d48 Mon Sep 17 00:00:00 2001 From: Iuri de Silvio Date: Thu, 20 Aug 2026 09:22:33 +0200 Subject: [PATCH 1/4] Add regression tests for the lunasvg SVG paths Covers three failures reproduced against canvas@4.0.0-rc2: - An SVG whose embedded `` is a format node-canvas cannot decode segfaults. The decoder callback in `loadSVGFromBuffer` ignores the status of `loadFromBuffer`, so `transferSurface()` returns a null surface and lunasvg passes it straight to `cairo_surface_set_user_data`. - A failed SVG re-render aborts the process on the next render or on GC. `renderSVGToSurface` destroys `_surface` without nulling it before its error return, so the pointer is destroyed a second time. - Every parse of an SVG with an embedded JPEG (also GIF and BMP) leaks the decoded frame: those decoders back the surface with `cairo_image_surface_create_for_data`, and `transferSurface()` drops `_data` without freeing it or attaching a destroy callback. Measured 1.4 MiB per parse of a 600x600 JPEG, linear and surviving a full finalizer drain. The same test over an embedded PNG passes and acts as the control, since cairo owns the pixels on that path. The two crashes take the process down, so each case runs in a child process. Co-Authored-By: Claude Opus 5 --- test/svg.test.js | 124 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 test/svg.test.js 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)` + ) + }) +}) From f67c93404b218fae2b988ae8e0d642a4e2b68868 Mon Sep 17 00:00:00 2001 From: Iuri de Silvio Date: Thu, 20 Aug 2026 09:22:33 +0200 Subject: [PATCH 2/4] Fix the crash, abort and leak the SVG tests cover `transferSurface()` hands the decoded surface to lunasvg and drops every pointer we hold. The JPEG, GIF and BMP decoders back their surface with a `new uint8_t[]` buffer through cairo_image_surface_create_for_data, which cairo does not own, so that buffer was leaked on every parse of an SVG with an embedded raster in one of those formats -- a whole decoded frame each time, 1.4 MiB for a 600x600 JPEG, unbounded. Attach it to the surface with a destroy callback so it dies with the surface it belongs to. `renderSVGToSurface()` destroyed `_surface` without nulling it before its error returns, so a re-render that fails at a size cairo rejects left a dangling pointer for the next render and for clearData() to destroy again. Null it, and treat a null Bitmap -- what renderToBitmap() returns when the document has no intrinsic size or the allocation fails -- as an error instead of reading a status through a null pointer. DrawImage now also stops when surface() returns null, which it does once the re-render throws. The decoder callback ignored the status of loadFromBuffer() and returned whatever surface the failed decode left behind. Return null there, and take lunasvg's fix for the null it has always been able to get back: it fed that straight to cairo_surface_set_user_data, so an SVG carrying an image in a format node-canvas cannot decode segfaulted. `loadSVGFromBuffer()` also kept the parsed document alive on both of its error paths, where it can only be reached again through an Image that never completed loading. Co-Authored-By: Claude Opus 5 --- pkg/lunasvg/build.zig.zon | 4 +-- src/CanvasRenderingContext2d.cc | 2 ++ src/Image.cc | 46 ++++++++++++++++++++++++++++++--- 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/pkg/lunasvg/build.zig.zon b/pkg/lunasvg/build.zig.zon index 30ddec25c..bdeb50ae3 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/c8fbe875e4c241a8b45c8e274647297db12bda22.tar.gz", + .hash = "N-V-__8AAPD5FADyrh8KcLt8S2MOxVcQuxCvX9DkrI5zKsUZ", } }, } 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()); From 4a4e4234b266920056056b53dec2e75bd67b90b2 Mon Sep 17 00:00:00 2001 From: Iuri de Silvio Date: Thu, 20 Aug 2026 09:22:33 +0200 Subject: [PATCH 3/4] Add changelog entries for the SVG fixes Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) 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 ================== From d2bc7f58953b290fc3b95cebcc5615684ce64c11 Mon Sep 17 00:00:00 2001 From: Iuri de Silvio Date: Thu, 20 Aug 2026 13:24:10 +0200 Subject: [PATCH 4/4] Point lunasvg at the commit carrying its own test The branch it pinned was rebased to add the regression test and the cairo lookup its build needs. Co-Authored-By: Claude Opus 5 --- pkg/lunasvg/build.zig.zon | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/lunasvg/build.zig.zon b/pkg/lunasvg/build.zig.zon index bdeb50ae3..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/iurisilvio/lunasvg/archive/c8fbe875e4c241a8b45c8e274647297db12bda22.tar.gz", - .hash = "N-V-__8AAPD5FADyrh8KcLt8S2MOxVcQuxCvX9DkrI5zKsUZ", + .url = "https://github.com/iurisilvio/lunasvg/archive/b03b0dd7ba9967bd507c4a13ab12ff37e1d4128a.tar.gz", + .hash = "N-V-__8AAGcLFQB2sVL8Tvu9IeCPuaczmK9v1-aj16-Uasup", } }, }