Skip to content

Release RsvgHandle and partial surface on SVG decode error paths#2585

Open
iurisilvio wants to merge 1 commit into
Automattic:masterfrom
iurisilvio:fix/svg-missing-dimensions-leak
Open

Release RsvgHandle and partial surface on SVG decode error paths#2585
iurisilvio wants to merge 1 commit into
Automattic:masterfrom
iurisilvio:fix/svg-missing-dimensions-leak

Conversation

@iurisilvio
Copy link
Copy Markdown

Fixes #2584.

Image::loadSVGFromBuffer() ignored the return value of rsvg_handle_get_intrinsic_size_in_pixels(), so when the SVG parsed cleanly but had no intrinsic size, d_width/d_height were read uninitialized and the function returned CAIRO_STATUS_READ_ERROR with _rsvg still attached and _is_svg still set. The orphaned RsvgHandle survived until the JS Image wrapper was finalized — visible as ~4 KiB/iter of steady RSS growth across repeated loadImage() of size-less SVG input.

Image::renderSVGToSurface() had the inverse problem on its own error branches: _rsvg was unreffed but not nulled, and _surface (and the cairo context on later branches) were left live. Image::surface() then ran a redundant g_object_unref(_rsvg) on the error path, double-touching state that may or may not have already been released.

Changes

  • loadSVGFromBuffer: initialize d_width/d_height, gate the assignment on the gboolean return value of rsvg_handle_get_intrinsic_size_in_pixels(), and unref/null _rsvg plus clear _is_svg before returning the missing-dimensions error.
  • renderSVGToSurface: on each error branch, destroy the cairo context if created, destroy and null _surface, unref and null _rsvg, clear _is_svg.
  • Image::surface: drop the now-redundant g_object_unref(_rsvg) on the renderSVGToSurface failure path — the inner function already cleaned up.

Verification

Repro from #2584, canvas@3.2.3 on Linux x86_64 / Node 20:

baseline rss=45.9 MiB
iter=10000 rss=94.7M slope=3.584 KiB/iter
baseline=45.9M end=94.7M delta=48.8M per-iter=4.997 KiB

After this patch the same loop is flat (slope in the range of N-API finalizer / cairo allocator noise).

Image::loadSVGFromBuffer() did not check the return value of
rsvg_handle_get_intrinsic_size_in_pixels(), so when the SVG parsed but had
no intrinsic size (no width/height attribute and no element that implies
one), d_width/d_height were read uninitialized and the function returned
CAIRO_STATUS_READ_ERROR with _rsvg still attached and _is_svg still set.
The orphaned RsvgHandle then survived until the JS Image wrapper was
finalized — visible as ~4 KiB/iter of steady RSS growth across repeated
loadImage() of size-less SVG input.

Image::renderSVGToSurface() had the inverse shape: each error branch
unreffed _rsvg but did not null it, and left _surface (and the cairo
context on later branches) live. Image::surface() then ran a redundant
g_object_unref(_rsvg) on the error path, double-touching state that may or
may not have already been released.

Fix:
- initialize d_width/d_height and gate the assignment on the return value
- on the missing-dimensions error path in loadSVGFromBuffer, unref and
  null _rsvg and clear _is_svg before returning
- on each error branch in renderSVGToSurface, destroy the cairo context if
  created, destroy and null _surface, unref and null _rsvg, clear _is_svg
- drop the now-redundant g_object_unref(_rsvg) in Image::surface()'s
  renderSVGToSurface failure path

Fixes Automattic#2584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

loadSVGFromBuffer leaks RsvgHandle when SVG has no intrinsic size

1 participant