Fix segfault, use-after-free and per-parse leak in the SVG paths

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
 ==================

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",
         }
     },
 }

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<InstanceData>()->CanvasCtor.Value()).UnwrapOr(false)) {

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<uint8_t*>(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());

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 = '<svg xmlns="http://www.w3.org/2000/svg" width="50" height="50">' +
+      `<image width="50" height="50" href="data:image/webp;base64,${junk}"/></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 = '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16">' +
+      '<rect width="16" height="16" fill="red"/></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(
+        \`<svg xmlns="http://www.w3.org/2000/svg" width="\${size}" height="\${size}">\` +
+        \`<image width="\${size}" height="\${size}" href="data:\${mime};base64,\` +
+        raster.toBuffer(mime).toString('base64') + '"/></svg>')
+      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)`
+    )
+  })
+})