Repository navigation
fix: Dispose chunk data buffer in ReadChunkData when read is cancelled - #3203
Conversation
Fixes SixLabors#3202. ReadChunkData rents a buffer from the MemoryAllocator before reading into it. If that read observes a cancellation request, the exception leaves the buffer rented but never returned, leaking the allocation.
| pngBytes = encoded.ToArray(); | ||
| } | ||
|
|
||
| using CancelAtPositionStream stream = new(pngBytes, firstChunkDataOffset); |
There was a problem hiding this comment.
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.
Fixes #3202.
ReadChunkData rents a buffer from the MemoryAllocator and then reads into it. If that read observes a cancellation request, the exception leaves ReadChunkData before the buffer is returned, so nothing disposes it.
Added a try/catch around the read: if it throws, dispose the buffer and rethrow. Added a regression test that forces a cancellation right at the IHDR chunk's data read and checks the rented buffer gets disposed either way, covering both Decode and Identify.