[memory-leak] Fix native use-after-free in SKNWayCanvas and SKOverdrawCanvas by github-actions[bot] · Pull Request #4372 · mono/SkiaSharp

github-actions · 2026-07-08T01:32:00Z

🤖 AI-generated fix. Produced automatically by the SkiaSharp Memory Leak Fixer agentic workflow using the memory-leak-fixer skill, and empirically validated (red→green) on linux-x64 against the milestone's pre-built native binaries.

The leak

SKNWayCanvas.AddCanvas (binding/SkiaSharp/SKNWayCanvas.cs) and the SKOverdrawCanvas(SKCanvas) constructor (binding/SkiaSharp/SKOverdrawCanvas.cs) hand a managed SKCanvas's handle to native factories (sk_nway_canvas_add_canvas, sk_overdraw_canvas_new). The native SkNWayCanvas / SkOverdrawCanvas keep raw, non-owning SkCanvas* pointers and forward draws to them.

The wrappers only issued a call-site GC.KeepAlive(canvas), which does not root the canvas for the wrapper's lifetime. If the added / wrapped SKCanvas is collected while the wrapper is still alive, its finalizer runs sk_canvas_destroy and the next forwarded draw dereferences freed memory → use-after-free / AccessViolationException.

The fix

Root the target canvas(es) in private managed fields for the lifetime of the wrapper, matching how the SKRegion / SKPath iterators already root their parent:

SKNWayCanvas: private readonly List<SKCanvas> canvases, kept in sync by AddCanvas / RemoveCanvas / RemoveAll.
SKOverdrawCanvas: private readonly SKCanvas canvas, set in the constructor.

Red → green

Adds two regression tests (tests/Tests/SkiaSharp/SKCanvasTest.cs) using WeakReference + a [MethodImpl(NoInlining)] helper so the source canvas has no managed root except the wrapper:

Test	Before fix	After fix
`NWayCanvasKeepsAddedCanvasesAlive`	❌ FAIL (added canvas collected)	✅ PASS
`OverdrawCanvasKeepsWrappedCanvasAlive`	❌ FAIL (wrapped canvas collected)	✅ PASS

Verified on linux-x64 (net10.0): both tests fail on the IsAlive assertion before the fix and pass after; the full SKCanvasTest class is 36 passed / 0 failed / 3 skipped (GPU-only skips).

Scope note

Genuine framework bug (native use-after-free), not a caller footgun.
Empirically proven (red→green), not merely statically reasoned.
Additive — private instance fields only, no public API surface change. ABI-safe.

Fixes #4371

Generated by Fixer - Memory Leak · ● 51.2M · ◷

The native SkNWayCanvas and SkOverdrawCanvas store raw, non-owning SkCanvas* pointers to the canvases added via AddCanvas / wrapped by the SKOverdrawCanvas constructor. The managed wrappers only issued a call-site GC.KeepAlive(canvas), which does not root the target for the wrapper's lifetime. If the target SKCanvas is collected while the wrapper is still alive, its finalizer runs sk_canvas_destroy and subsequent draws forwarded through the wrapper dereference freed memory (AccessViolation). Root the target canvas(es) in private managed fields for the lifetime of the wrapper, matching how the region/path iterators root their parent. This is additive and ABI-safe (private fields only). Adds red->green regression tests using WeakReference + a NoInlining helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Replace the hand-rolled List<SKCanvas> in SKNWayCanvas and the private field in SKOverdrawCanvas with the framework's built-in keep-alive mechanism: - Add symmetric Unreferenced(owner, child) and UnreferencedAll(owner) helpers to SKObject as the inverse of the existing Referenced(owner, child), so derived types never touch KeepAliveObjects directly. - SKNWayCanvas: Referenced/Unreferenced/UnreferencedAll for add/remove/removeAll. - SKOverdrawCanvas: Referenced(this, canvas) in the constructor. Behaviour is identical (the child is rooted for the wrapper's lifetime and released on dispose); the existing regression tests still pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-08T14:36:45Z

📦 Try the packages from this PR

Warning

Do not run these scripts without first reviewing the code in this PR.

Step 1 — Download the packages

bash / macOS / Linux:

curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4372

PowerShell / Windows:

iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4372"

Step 2 — Add the local NuGet source

dotnet nuget add source ~/.skiasharp/hives/pr-4372/packages --name skiasharp-pr-4372

More options

Option	Description
`--successful-only` / `-SuccessfulOnly`	Only use successful builds
`--force` / `-Force`	Overwrite previously downloaded packages
`--list` / `-List`	List available artifacts without downloading
`--build-id ID` / `-BuildId ID`	Download from a specific build

Or download manually from Azure Pipelines — look for the nuget artifact on the build for this PR.

Remove the source when you're done:

dotnet nuget remove source skiasharp-pr-4372

mattleibow · 2026-07-08T14:44:44Z

Updated to use SkiaSharp's built-in keep-alive mechanism instead of the hand-rolled List<SKCanvas>/field.

Added symmetric Unreferenced(owner, child) and UnreferencedAll(owner) helpers to SKObject as the inverse of the existing Referenced(owner, child), so derived types never touch KeepAliveObjects directly.
SKNWayCanvas now uses Referenced/Unreferenced/UnreferencedAll for add/remove/removeAll; SKOverdrawCanvas uses Referenced(this, canvas).

Behaviour is identical and the existing regression tests still pass (full suite: same results as base — the only failure is the pre-existing ganesh-gl/DiagonalLines GPU golden flake, unrelated to this change). The keep-alive helper is documented in #4377.

github-actions · 2026-07-08T14:56:35Z

📖 Documentation Preview

The documentation for this PR has been deployed and is available at:

🔗 View Staging Site
🔗 View Staging Docs
🔗 View Staging Gallery (Blazor)
🔗 View Staging Gallery (Uno Platform)
🔗 View Staging SkiaFiddle

This preview will be updated automatically when you push new commits to this PR.

This comment is automatically updated by the documentation staging workflow.

… hand-rolled fields (#4377) Teach memory-leak-fixer to use SKObject keep-alive helpers instead of hand-rolled fields (#4377) Context: documentation/dev/memory-management.md Related: #4372, #4355 SkiaSharp already has a built-in mechanism for the "root a related object so the GC can't collect it while its owner lives" problem: SKObject's static `Referenced(owner, child)` (backed by the per-object `KeepAliveObjects` dictionary), plus `Owned`/`OwnedBy` (backed by `OwnedObjects`) for the root-and-dispose case. `Referenced` is used idiomatically by SKDocument, SKColorSpace, and SKSVG — but it was never documented in the authoritative memory-management model, and the memory-leak-fixer skill's area 5 taught a hand-rolled `private readonly` field (or a `List<T>`) instead. Because the guidance never pointed at the real feature, recent generated leak fixes reinvented it: #4372 hand-rolled a `List<SKCanvas>` plus a field, and #4355 added a `private readonly SKRegion` field. This PR closes that gap so future runs reach for the framework mechanism instead. Changes: * documentation/dev/memory-management.md — new "Keeping Related Objects Alive" section documenting Referenced / Unreferenced / UnreferencedAll and Owned / OwnedBy, and the KeepAliveObjects vs OwnedObjects dictionaries (roots-only vs roots-and-disposes). * memory-leak-fixer/references/types-of-leaks.md — area 5 now teaches `Referenced(this, parent)` as the idiomatic fix, with the mutable-set variant (`Unreferenced` / `UnreferencedAll`); keeps a plain field as an accepted local-convention fallback (e.g. the SKRegion iterators) to avoid mixed styles; updates the Where-to-look / How-to-find-it / Watch-out notes. * memory-leak-fixer/SKILL.md — self-review gate now flags a hand-rolled keep-alive field/List<T> where the helpers suffice. Docs/skill only — no binding code changes here. The symmetric inverse helpers (`Unreferenced`, `UnreferencedAll`) that the docs reference are introduced and consumed in #4372; #4355 consumes the pre-existing `Referenced`. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…4476) Add regression-tracking benchmarks for common colour/interop paths (#4476) Derived from: #4370, #4385, #4442, #4453, #4455 CI: Track - Benchmarks run 29608575980 (green, all OS/roles) The permanent regression-tracking suite (SkiaSharp.Benchmarks.Tracking) landed recently alongside a wave of perf/agentic PRs, but it had no coverage for the managed colour/interop paths those PRs actually touch. This adds a small, curated set of trackers so a future regression (or the improvement itself) shows up as a step in the persisted per-OS time/allocation history. Following the suite's scaled-pyramid philosophy (a few small common-path trackers plus one broad composite, not a micro-benchmark per PR), four benchmarks are added under Tracking/Benchmarks/ (auto-linked into the source-mode project, so they also drive the PR column): * ColorMathBenchmark - managed SKColor->SKColorF (#4370) and SKPMColor PreMultiply/UnPreMultiply (#4385). Zero-alloc; the ToColorF/ToColor ratio (the reverse operator stays native) is a managed-regression signal even on noisy shared runners. These two ports are the real, dashboard-visible speed wins - their time lines should drop when the ports merge. * RasterImageLifecycleBenchmark - create+dispose churn of SKImage.Create raster (#4455) and SKData.Create with a managed release proc (#4453). Allocation is the tracked signal for that object-lifecycle area. * RuntimeEffectShaderBenchmark - a per-frame animated SkSL shader: build uniforms (the SKColor uniform pays the #4370 convert), ToShader, draw. Covers the uniforms lifecycle whose SKData leak #4442 touched. * SceneRenderBenchmark - one broad frame (transforms, linear+radial gradients, filled/stroked primitives, a built path, a scaled image draw, a colour-filter layer, clipping, managed colour maths) so many unrelated future optimizations each nudge it - a merge indicator rather than a micro-benchmark. Scope notes: the failure-path leak fixes (#4453, #4455), the native uniforms leak (#4442), and the UAF/double-free fixes (#4372, #4468) are correctness/native-memory changes that a throughput + managed-allocation dashboard largely will not register; they are represented here by area, not as leak assertions (those belong in soak/ leak tests). #4372 and #4468 are intentionally not benchmarked (niche debug canvases; HarfBuzz lives in a separate assembly the tracking project does not reference). All benchmarks use public API only (the project references the nightly NuGet, not internal SkiaApi), are deterministic and machine-independent (fixed seeds, no fonts or external content), and compile against every benchmarked role - verified against the 3.119.4 baseline (prev-major) and 4.151 nightly, with only SKPath/SKPathBuilder behind the existing #if. A short local run and the full CI matrix (Linux/Windows/ macOS, all version roles plus the source-built PR column) emit time and allocation data for every case. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…170e6

github-actions Bot added perf/memory-leak Unbounded memory growth: leaked native handles or undisposed objects. Implies tenet/performance. tenet/performance Performance related issues labels Jul 8, 2026

github-project-automation Bot added this to SkiaSharp Backlog Jul 8, 2026

mattleibow mentioned this pull request Jul 8, 2026

Teach memory-leak-fixer to use SKObject.Referenced/KeepAliveObjects instead of hand-rolled fields #4377

Merged

mattleibow added the partner/agentic-workflows Issues and PRs created by SkiaSharp agentic workflows. label Jul 11, 2026

mattleibow mentioned this pull request Jul 17, 2026

Add regression-tracking benchmarks for common colour/interop paths #4476

Merged

Merge branch 'main' into dev/fix-nway-overdraw-canvas-uaf-4a1467a42e8…

0f0c020

…170e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[memory-leak] Fix native use-after-free in SKNWayCanvas and SKOverdrawCanvas#4372

[memory-leak] Fix native use-after-free in SKNWayCanvas and SKOverdrawCanvas#4372
github-actions[bot] wants to merge 3 commits into
mainfrom
dev/fix-nway-overdraw-canvas-uaf-4a1467a42e8170e6

github-actions Bot commented Jul 8, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

mattleibow commented Jul 8, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

github-actions Bot commented Jul 8, 2026

The leak

The fix

Red → green

Scope note

Uh oh!

github-actions Bot commented Jul 8, 2026

📦 Try the packages from this PR

Uh oh!

mattleibow commented Jul 8, 2026

Uh oh!

github-actions Bot commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant