Release RsvgHandle and partial surface on SVG decode error paths#2585
Open
iurisilvio wants to merge 1 commit into
Open
Release RsvgHandle and partial surface on SVG decode error paths#2585iurisilvio wants to merge 1 commit into
iurisilvio wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2584.
Image::loadSVGFromBuffer()ignored the return value ofrsvg_handle_get_intrinsic_size_in_pixels(), so when the SVG parsed cleanly but had no intrinsic size,d_width/d_heightwere read uninitialized and the function returnedCAIRO_STATUS_READ_ERRORwith_rsvgstill attached and_is_svgstill set. The orphanedRsvgHandlesurvived until the JSImagewrapper was finalized — visible as ~4 KiB/iter of steady RSS growth across repeatedloadImage()of size-less SVG input.Image::renderSVGToSurface()had the inverse problem on its own error branches:_rsvgwas unreffed but not nulled, and_surface(and the cairo context on later branches) were left live.Image::surface()then ran a redundantg_object_unref(_rsvg)on the error path, double-touching state that may or may not have already been released.Changes
loadSVGFromBuffer: initialized_width/d_height, gate the assignment on thegbooleanreturn value ofrsvg_handle_get_intrinsic_size_in_pixels(), and unref/null_rsvgplus clear_is_svgbefore 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-redundantg_object_unref(_rsvg)on therenderSVGToSurfacefailure path — the inner function already cleaned up.Verification
Repro from #2584,
canvas@3.2.3on Linux x86_64 / Node 20:After this patch the same loop is flat (slope in the range of N-API finalizer / cairo allocator noise).