Skip to content

Conversation

@michaldobrodenka
Copy link
Contributor

Extension public static Blob ToHarfBuzzBlob(this SKStreamAsset asset) may leak when asset MemoryBase is not IntPtr.Zero.

After freeing memory, Dispose to SKStreamAsset (SKNativeObject) is added.

Bugs Fixed

Behavioral Changes

None.

Required skia PR

None.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

@michaldobrodenka
Copy link
Contributor Author

michaldobrodenka commented Oct 3, 2025 via email

@michaldobrodenka
Copy link
Contributor Author

Fixes #3378

@molesmoke
Copy link

molesmoke commented Oct 6, 2025

It is kind of odd that SKStreamAsset is disposed on one path but not the other (and the caller doesn't know which). My two cents is that the problem is more that SKShaper doesn't dispose the intermediate SKStreamAsset from the Typeface. The leaking stream could already have been destroyed by the time the constructor exits (assuming that the Font no longer requires access to the Blob - since it's written that way already).

@mattleibow
Copy link
Contributor

The extension method copies the data and does not own or keep a reference to the asset. Would it be better to maybe fix the caller?

Since the stream may be in use by some othercode, disposing the harfbuzz object will now cause issues.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a resource leak in ToHarfBuzzBlob(SKStreamAsset) by ensuring the underlying SKStreamAsset is disposed when the HarfBuzzSharp.Blob releases its backing memory.

Changes:

  • Dispose the SKStreamAsset in the GetMemoryBase() == IntPtr.Zero (copy-to-unmanaged-memory) path via the blob release callback.

Comment on lines 28 to +30
var ptr = Marshal.AllocCoTaskMem(size);
asset.Read(ptr, size);
blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem(ptr));
blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => { Marshal.FreeCoTaskMem(ptr); asset.Dispose(); });
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asset.Read(ptr, size) can return fewer bytes than requested; the return value is ignored and the blob is created with length = size anyway. This can lead to HarfBuzz reading uninitialized memory and producing incorrect shaping results. Consider reading in a loop until size bytes are read (or throw if EOF/0 bytes read early), and create the Blob with the actual number of bytes read.

Copilot uses AI. Check for mistakes.
var ptr = Marshal.AllocCoTaskMem(size);
asset.Read(ptr, size);
blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem(ptr));
blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => { Marshal.FreeCoTaskMem(ptr); asset.Dispose(); });
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes a resource-leak scenario; please add a unit test that asserts the SKStreamAsset is disposed when the returned Blob is disposed for both paths: (1) a stream with GetMemoryBase() != IntPtr.Zero (e.g., SKMemoryStream) and (2) a stream with GetMemoryBase() == IntPtr.Zero (e.g., SKFileStream).

Copilot uses AI. Check for mistakes.
var ptr = Marshal.AllocCoTaskMem(size);
asset.Read(ptr, size);
blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem(ptr));
blob = new Blob(ptr, size, MemoryMode.ReadOnly, () => { Marshal.FreeCoTaskMem(ptr); asset.Dispose(); });
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title/description mention the leak when MemoryBase is not IntPtr.Zero, but this change disposes the asset in the memoryBase == IntPtr.Zero branch. Please update the PR title/description (or clarify the scenario) to match what is being fixed.

Copilot uses AI. Check for mistakes.
@molesmoke
Copy link

Since the stream may be in use by some othercode, disposing the harfbuzz object will now cause issues.

Unfortunately, the asset is already being disposed on the other path (that should probably be removed), and SKShaper should probably be something like:

		public SKShaper(SKTypeface typeface)
		{
			Typeface = typeface ?? throw new ArgumentNullException(nameof(typeface));

			int index;
			using (var stream = Typeface.OpenStream(out index))
			using (var blob = stream.ToHarfBuzzBlob())
			using (var face = new Face(blob, index))
			{
				face.Index = index;
				face.UnitsPerEm = Typeface.UnitsPerEm;

				hbFont = new Font(face);
				hbFont.SetScale(FONT_SIZE_SCALE, FONT_SIZE_SCALE);

				hbFont.SetFunctionsOpenType();
			}

			hbBuffer = new Buffer();
		}

I can put a PR for that instead if you want?

@jeremy-visionaid
Copy link
Contributor

I've put an alternate PR here to remove the stream disposal on the other path and fix the SKShaper constructor: #3466

@michaldobrodenka
Copy link
Contributor Author

I've put an alternate PR here to remove the stream disposal on the other path and fix the SKShaper constructor: #3466

Great, that may be the way forward

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants