[memory-leak] Fix double-free in HarfBuzzSharp.Buffer.UnicodeFunctions getter - #4468
Draft
github-actions[bot] wants to merge 4 commits into
Draft
[memory-leak] Fix double-free in HarfBuzzSharp.Buffer.UnicodeFunctions getter#4468github-actions[bot] wants to merge 4 commits into
github-actions[bot] wants to merge 4 commits into
Conversation
hb_buffer_get_unicode_funcs returns a borrowed (transfer-none) reference owned by the buffer. The getter wrapped it in an owning UnicodeFunctions, so disposing the returned wrapper called hb_unicode_funcs_destroy and over-freed a handle still referenced by the live buffer (double-free / premature destroy). Make UnicodeFunctions track ownership and wrap the borrowed getter result non-owning so Dispose is a no-op for it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
partner/agentic-workflows
mattleibow
added a commit
that referenced
this pull request
Jul 17, 2026
…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>
Contributor
Author
📦 Try the packages from this PRWarning 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 -- 4468PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4468"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4468/packages --name skiasharp-pr-4468More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4468 |
Contributor
Author
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a native lifetime/ownership bug in HarfBuzzSharp where Buffer.UnicodeFunctions wrapped a borrowed hb_unicode_funcs_t* as owned, leading to premature destroy / double-free when the wrapper was disposed.
Changes:
- Track native-handle ownership inside
UnicodeFunctionsand only callhb_unicode_funcs_destroywhen the wrapper truly owns the handle. - Update
Buffer.UnicodeFunctionsgetter to wrap the HarfBuzz “transfer-none” result asowns: false. - Add a regression test to ensure disposing the getter result does not destroy the underlying shared handle while the buffer is still alive.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
binding/HarfBuzzSharp/UnicodeFunctions.cs |
Adds an internal ownership flag and gates hb_unicode_funcs_destroy behind it. |
binding/HarfBuzzSharp/Buffer.cs |
Wraps hb_buffer_get_unicode_funcs result as non-owning (owns: false) per HarfBuzz contract. |
tests/Tests/HarfBuzzSharp/HBUnicodeFuncsTest.cs |
Adds regression test covering the borrowed-handle disposal scenario. |
Comment on lines
+159
to
+163
| // The buffer is intentionally left alive (and its funcs referenced) for the | ||
| // whole test, so the funcs must remain valid. | ||
| var buffer = new Buffer(); | ||
| buffer.UnicodeFunctions = unicodeFunctions; | ||
|
|
Contributor
There was a problem hiding this comment.
Addressed in efba6ff — switched to using var buffer so it stays alive through the assertion and is disposed cleanly at scope exit, matching the other HarfBuzzSharp tests.
The regression test BufferUnicodeFunctionsGetterDoesNotOwnBorrowedHandle was inserted inside ShouldSetDecomposeDelegate's body: the method's closing brace was missing and the new [Fact] method was nested as an invalid public local function. This produced two compile errors in SkiaSharp.Tests.csproj: HBUnicodeFuncsTest.cs(149,3): error CS0106: The modifier 'public' is not valid for this item HBUnicodeFuncsTest.cs(173,2): error CS1513: } expected A non-compiling test assembly caused every Tests leg to fail on every platform (WASM/Windows/Linux/macOS/Catalyst/Android/iOS) while the Build Managed jobs passed. Close ShouldSetDecomposeDelegate properly and de-nest the new test as a class member. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 278bdb81-233e-4f16-bbf3-ec801600bd12
Addresses Copilot review feedback: use 'using var buffer' so the buffer stays alive through the assertion and is disposed cleanly at scope exit, matching the disposal convention of the other HarfBuzzSharp tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 278bdb81-233e-4f16-bbf3-ec801600bd12
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.
The bug
HarfBuzzSharp.Buffer.UnicodeFunctions(binding/HarfBuzzSharp/Buffer.cs) wrapped the result ofhb_buffer_get_unicode_funcs— a borrowed (transfer-none) reference owned by the buffer — in an owningUnicodeFunctions. Disposing that wrapper calledhb_unicode_funcs_destroyon a handle the still-alive buffer references, causing a double-free / premature destroy (Area 1: wrongowns:flag).The fix
UnicodeFunctionsnow tracks ownership via a privateownsflag;DisposeHandleronly destroys the native handle when it owns it.owns: false, so itsDisposeis a no-op — matching HarfBuzz's transfer-none contract.binding/HarfBuzzSharp/**; no public signature changed (ABI-safe).This is the idiomatic borrowed → don't free fix from the leak catalogue's Area 1.
Proof (red→green)
A regression test (
HBUnicodeFuncsTest.BufferUnicodeFunctionsGetterDoesNotOwnBorrowedHandle) sets a customUnicodeFunctionson a buffer, reads it back via the getter, disposes the wrapper + original while the buffer is still alive, and asserts the shared handle was not destroyed.Executed locally against the shipped
HarfBuzzSharp.NativeAssets.Linuxnative library (MSBuild was unavailable in the runner, so sources were compiled directly withcsc):destroyed=True→ RED (over-freed; ausing-buffer variant crashed withmalloc(): unaligned tcache chunk detected)destroyed=False→ GREENBoth directions verified (revert ⇒ red, re-apply ⇒ green).
Scope note
Fixes #4467