Make Blob.FromStream GC safe

[jeremy-visionaid]

Description of Change

Avoids a potential crash from memory being moved by GC

Bugs Fixed

• Fixes [BUG] Blob FromStream is not GC safe #3472

API Changes

None.

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

[dotnet-policy-service]

Hey there @@jeremy-visionaid! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[Copilot]

Pull request overview

This pull request fixes a critical GC safety bug in Blob.FromStream where managed array memory could be moved by the garbage collector after the blob was created, potentially causing crashes. The fix replaces the managed memory approach (using a byte array with fixed) with unmanaged memory allocation using Marshal.AllocCoTaskMem, ensuring the memory location remains stable throughout the blob's lifetime.

Changes:

• Replaced managed memory pattern (MemoryStream + ToArray + fixed) with unmanaged memory allocation pattern (Marshal.AllocCoTaskMem + UnmanagedMemoryStream)
• Updated the existing test to use separate using statements for better readability
• Added a new GC safety test that explicitly verifies the blob data remains accessible after garbage collection

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
binding/HarfBuzzSharp/Blob.cs	Replaced GC-unsafe managed memory with unmanaged memory allocation using Marshal.AllocCoTaskMem/FreeCoTaskMem
tests/Tests/HarfBuzzSharp/HBBlobTest.cs	Refactored existing test and added new GC safety test that forces collection and verifies data access

[mattleibow]

PR Review: Make Blob.FromStream GC Safe (#3473)

Hey @jeremy-visionaid! Thanks for catching this GC safety issue — it's a subtle but serious bug that could cause random crashes. The core fix (using unmanaged memory instead of a pinned managed array) is the right approach. 👍

I have a couple of concerns before this can be merged:

---

🔴 Non-seekable streams will throw

The new implementation accesses stream.Length and stream.Position:

var length = (int)(stream.Length - stream.Position);

This throws NotSupportedException on non-seekable streams like NetworkStream, GZipStream, or HTTP response streams. The original implementation handled these correctly by using MemoryStream.CopyTo() which reads until EOF.

Suggested fix:

// For non-seekable streams, buffer into memory first
if (!stream.CanSeek)
{
    using var ms = new MemoryStream ();
    stream.CopyTo (ms);
    ms.Position = 0;
    return FromStream (ms);
}

---

🔴 Memory leak if an exception occurs

If stream.CopyTo(ums) throws, the allocated memory is never freed:

var dataPtr = Marshal.AllocCoTaskMem (length);
// If CopyTo throws, dataPtr leaks
using var ums = new UnmanagedMemoryStream ((byte*)dataPtr, length, length, FileAccess.ReadWrite);
stream.CopyTo (ums);

Suggested fix: Wrap in try/catch:

var dataPtr = Marshal.AllocCoTaskMem (length);
try
{
    using var ums = new UnmanagedMemoryStream ((byte*)dataPtr, length, length, FileAccess.ReadWrite);
    stream.CopyTo (ums);
    return new Blob (dataPtr, length, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem (dataPtr));
}
catch
{
    Marshal.FreeCoTaskMem (dataPtr);
    throw;
}

---

🟡 Test suggestions

A couple of additional test cases would help ensure edge cases are covered:

Non-seekable stream test:

[SkippableFact]
public void ShouldCreateFromNonSeekableStream()
{
    using var fileStream = File.Open(Path.Combine(PathToFonts, "Funkster.ttf"), FileMode.Open, FileAccess.Read);
    using var nonSeekable = new NonSeekableReadOnlyStream(fileStream);
    using var blob = Blob.FromStream(nonSeekable);
    Assert.Equal(236808, blob.Length);
}

Partially-read stream test:

[SkippableFact]
public void ShouldCreateFromPartiallyReadStream()
{
    using var stream = File.Open(Path.Combine(PathToFonts, "Funkster.ttf"), FileMode.Open, FileAccess.Read);
    
    // Read first 100 bytes, simulating a partially consumed stream
    var header = new byte[100];
    stream.Read(header, 0, 100);
    
    using var blob = Blob.FromStream(stream);
    Assert.Equal(236808 - 100, blob.Length);
}

---

Complete suggested implementation

Putting it all together:

public static unsafe Blob FromStream (Stream stream)
{
    if (stream == null)
        throw new ArgumentNullException (nameof (stream));

    // For non-seekable streams, buffer into memory first
    if (!stream.CanSeek)
    {
        using var ms = new MemoryStream ();
        stream.CopyTo (ms);
        ms.Position = 0;
        return FromStream (ms);
    }

    var length = (int)(stream.Length - stream.Position);
    if (length == 0)
        return Empty;

    var dataPtr = Marshal.AllocCoTaskMem (length);
    try
    {
        using var ums = new UnmanagedMemoryStream ((byte*)dataPtr, length, length, FileAccess.ReadWrite);
        stream.CopyTo (ums);
        return new Blob (dataPtr, length, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem (dataPtr));
    }
    catch
    {
        Marshal.FreeCoTaskMem (dataPtr);
        throw;
    }
}

---

Thanks again for the contribution! Let me know if you have any questions about the suggested changes.