Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/ImageSharp/Formats/Png/PngDecoderCore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2485,7 +2485,17 @@ private IMemoryOwner<byte> ReadChunkData(int length)
length = (int)Math.Min(length, this.currentStream.Length - this.currentStream.Position);
IMemoryOwner<byte> buffer = this.configuration.MemoryAllocator.Allocate<byte>(length, AllocationOptions.Clean);

this.currentStream.Read(buffer.GetSpan(), 0, length);
try
{
this.currentStream.Read(buffer.GetSpan(), 0, length);
}
catch
{
// The read can throw before the caller takes ownership of the buffer (e.g. the stream
// observes a cancellation request), so dispose it here to avoid leaking the rented memory.
buffer.Dispose();
throw;
}

return buffer;
}
Expand Down
84 changes: 84 additions & 0 deletions tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.Cancellation.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using SixLabors.ImageSharp.Formats;
using SixLabors.ImageSharp.Formats.Png;
using SixLabors.ImageSharp.PixelFormats;

namespace SixLabors.ImageSharp.Tests.Formats.Png;

public partial class PngDecoderTests
{
[Theory]
[InlineData(true)]
[InlineData(false)]
public void Decode_DisposesChunkDataBuffer_WhenChunkReadIsCancelled(bool identifyOnly)
{
// Every PNG starts with an 8-byte signature followed by the first chunk's 4-byte length
// and 4-byte type fields, so byte 16 is always the start of that chunk's data (IHDR here).
// Cancelling exactly there reproduces the leak from GH issue #3202: ReadChunkData rents
// its buffer from the MemoryAllocator and only then reads into it, so a cancellation
// observed by that read leaves the rented buffer undisposed without the try/finally.
//
// PngDecoder.Instance is used directly (rather than Image.Load/Identify) to bypass the
// generic format-detection probe, which reads the stream header independently and would
// otherwise observe the cancellation itself before PNG-specific parsing starts.
const int firstChunkDataOffset = 16;
byte[] pngBytes;
using (Image<Rgba32> source = new(4, 4))
using (MemoryStream encoded = new())
{
source.SaveAsPng(encoded);
pngBytes = encoded.ToArray();
}

using CancelAtPositionStream stream = new(pngBytes, firstChunkDataOffset);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we use the existing PausedMemoryStream and cancel a real token here?

The current helper overrides only Read(byte[], int, int), but the chunk-data read reaches ReadToBufferDirectSlow(Span<byte>). That bypasses the override, so the test does not trigger cancellation at the intended allocation.

This replacement uses a four-byte buffer and cancels while the IHDR type is being read. That read completes, then ReadChunkData allocates its buffer and the following read observes the cancelled token. The existing [ValidateDisposedMemoryAllocations] on PngDecoderTests checks that the allocation is disposed.

// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using SixLabors.ImageSharp.Formats;
using SixLabors.ImageSharp.Formats.Png;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp.Tests.TestUtilities;

namespace SixLabors.ImageSharp.Tests.Formats.Png;

public partial class PngDecoderTests
{
    [Theory]
    [InlineData(true)]
    [InlineData(false)]
    public async Task Decode_DisposesChunkDataBuffer_WhenChunkReadIsCancelled(bool identifyOnly)
    {
        byte[] pngBytes;
        using (Image<Rgba32> source = new(4, 4))
        using (MemoryStream encoded = new())
        {
            source.SaveAsPng(encoded);
            pngBytes = encoded.ToArray();
        }

        using CancellationTokenSource cts = new();
        using PausedMemoryStream stream = new(pngBytes);

        stream.OnWaiting(s =>
        {
            // The signature occupies bytes 0-7 and the chunk length occupies bytes 8-11.
            // Cancel during the type read, after BufferedReadStream has checked its token.
            // That read completes; the next read checks cancellation after allocating chunk data.
            if (s.Position == 12)
            {
                cts.Cancel();
                stream.Release();
            }
            else
            {
                stream.Next();
            }
        });

        Configuration configuration = Configuration.CreateDefaultInstance();

        // Read the chunk length and type separately so cancellation occurs during the type read.
        configuration.StreamProcessingBufferSize = 4;
        DecoderOptions options = new() { Configuration = configuration };

        // Calling the decoder directly avoids cancellation during format detection.
        if (identifyOnly)
        {
            await Assert.ThrowsAnyAsync<OperationCanceledException>(async () =>
            {
                await PngDecoder.Instance.IdentifyAsync(options, stream, cts.Token);
            });
        }
        else
        {
            await Assert.ThrowsAnyAsync<OperationCanceledException>(async () =>
            {
                using Image<Rgba32> image =
                    await PngDecoder.Instance.DecodeAsync<Rgba32>(options, stream, cts.Token);
            });
        }

        Assert.True(cts.IsCancellationRequested);
    }
}

I verified this replacement in Release on .NET 10. Both cases fail without the disposal fix, each detecting one undisposed buffer, and both pass with the fix.


// BufferedReadStream prefetches up to this size into its own internal buffer, so a large
// size would let it read straight through the cancellation point in one call before
// PngDecoderCore.ReadChunkData ever allocates its buffer. Matching the buffer size to the
// cancellation point makes the length/type fields resolve from one prefetch and the
// chunk-data read (which exceeds the remaining buffer) go directly to the stream instead.
Configuration configuration = Configuration.CreateDefaultInstance();
configuration.StreamProcessingBufferSize = firstChunkDataOffset - 8;
DecoderOptions options = new() { Configuration = configuration };

if (identifyOnly)
{
Assert.ThrowsAny<OperationCanceledException>(() => PngDecoder.Instance.Identify(options, stream));
}
else
{
Assert.ThrowsAny<OperationCanceledException>(() =>
{
using Image<Rgba32> image = PngDecoder.Instance.Decode<Rgba32>(options, stream);
});
}
}

/// <summary>
/// A stream over an in-memory PNG that throws <see cref="OperationCanceledException"/> once a
/// read is requested at or past <paramref name="cancelAtPosition"/>, regardless of the
/// requested length. Using the stream position (rather than the requested length) to decide
/// when to throw keeps the trigger point exact even though <see cref="BufferedReadStream"/>
/// prefetches more than the caller asks for.
/// </summary>
/// <remarks>
/// Only the <c>byte[]</c> overload of <see cref="Read(byte[], int, int)"/> is overridden.
/// <see cref="MemoryStream"/> does not implement <see cref="Stream.Read(Span{byte})"/> in
/// terms of this overload (or vice versa), so overriding both and having each delegate to the
/// other would recurse indefinitely.
/// </remarks>
private sealed class CancelAtPositionStream(byte[] data, int cancelAtPosition) : MemoryStream(data)
{
public override int Read(byte[] buffer, int offset, int count)
{
if (this.Position >= cancelAtPosition)
{
throw new OperationCanceledException();
}

return base.Read(buffer, offset, count);
}
}
}
Loading