From fe50e0a748bd9d4f256bfdd965a62c4e01fcc519 Mon Sep 17 00:00:00 2001 From: Jason Finch Date: Sun, 18 Oct 2026 09:15:25 +1000 Subject: [PATCH 1/2] Keep buffered non-seekable encoder output identical to a seekable save Encoding to a non-seekable stream buffers the output in a ChunkedMemoryStream. Overwriting bytes after seeking back, as the TIFF, BMP and ANI encoders do to patch offsets, extended the stream length, so trailing uninitialized bytes were copied to the destination. Seeking past the end of the buffered data, as the ICO and CUR encoders do to leave room for the directory, threw an ArgumentOutOfRangeException on an empty buffer and wrote one byte early at a chunk boundary. The length now only grows when a write ends past the current end, and a position beyond the allocated chunks is resolved after the buffer expands. --- src/ImageSharp/IO/ChunkedMemoryStream.cs | 33 +++++++++---- .../Formats/Tiff/TiffEncoderTests.cs | 23 +++++++++ .../IO/ChunkedMemoryStreamTests.cs | 47 +++++++++++++++++++ .../ImageSharp.Tests/Image/ImageTests.Save.cs | 46 ++++++++++++++++++ 4 files changed, 141 insertions(+), 8 deletions(-) diff --git a/src/ImageSharp/IO/ChunkedMemoryStream.cs b/src/ImageSharp/IO/ChunkedMemoryStream.cs index c164045df5..8be3fbb75e 100644 --- a/src/ImageSharp/IO/ChunkedMemoryStream.cs +++ b/src/ImageSharp/IO/ChunkedMemoryStream.cs @@ -193,10 +193,21 @@ public override void Write(ReadOnlySpan buffer) long remaining = this.memoryChunkBuffer.Length - this.position; // Ensure we have enough capacity to write the data. - while (remaining < count) + if (remaining < count) { - this.memoryChunkBuffer.Expand(); - remaining = this.memoryChunkBuffer.Length - this.position; + // A position beyond the allocated chunks, reached by seeking past the end, has no chunk + // until the buffer grows, so its chunk location is resolved again after expanding. + bool resolvePosition = remaining < 0; + while (remaining < count) + { + this.memoryChunkBuffer.Expand(); + remaining = this.memoryChunkBuffer.Length - this.position; + } + + if (resolvePosition) + { + this.SetPosition(this.position); + } } int bytesToWrite = count; @@ -231,7 +242,11 @@ public override void Write(ReadOnlySpan buffer) } this.position += bytesWritten; - this.length += bytesWritten; + + // Encoders such as TIFF and BMP seek back to patch offsets that were written earlier. + // Overwriting existing bytes must not extend the stream, so the length only grows + // when the write ends beyond the current end of the data. + this.length = Math.Max(this.length, this.position); } /// @@ -344,11 +359,13 @@ private void SetPosition(long value) int currentChunkIndex = 0; long offset = newPosition; - // If the new position is greater than the length of the stream, set the position to the end of the stream - if (offset > 0 && offset >= this.memoryChunkBuffer.Length) + // A position at or beyond the end of the allocated chunks starts the chunk that the next write allocates. + // This is exact at the end of the chunks, which is also where a write that fills the last chunk leaves + // the position. Write resolves a position further beyond the end once the chunks reach it. + if (offset >= this.memoryChunkBuffer.Length) { - this.bufferIndex = this.memoryChunkBuffer.ChunkCount - 1; - this.chunkIndex = this.memoryChunkBuffer[this.bufferIndex].Length - 1; + this.bufferIndex = this.memoryChunkBuffer.ChunkCount; + this.chunkIndex = 0; return; } diff --git a/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs index e4a8eeebbb..2f1c36f8df 100644 --- a/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs @@ -813,6 +813,29 @@ public void TiffEncoder_WritesIfdOffsetAtWordBoundary() Assert.Equal(imageBytes[5], expectedIfdOffsetBytes[1]); } + [Fact] + public async Task TiffEncoder_ToNonSeekableStream_MatchesSeekableStream() + { + // arrange + // Each frame adds an IFD offset that is patched after the frame data is written. + using Image image = new(32, 24, Color.Red.ToPixel()); + image.Frames.AddFrame(new ImageFrame(image.Configuration, 32, 24, Color.Blue.ToPixel())); + TiffEncoder encoder = new(); + using MemoryStream seekable = new(); + image.Save(seekable, encoder); + + using MemoryStream syncTarget = new(); + using MemoryStream asyncTarget = new(); + + // act + image.Save(new NonSeekableStream(syncTarget), encoder); + await image.SaveAsync(new NonSeekableStream(asyncTarget), encoder, TestContext.Current.CancellationToken); + + // assert + Assert.Equal(seekable.ToArray(), syncTarget.ToArray()); + Assert.Equal(seekable.ToArray(), asyncTarget.ToArray()); + } + [Theory] [WithFile(RgbUncompressed, PixelTypes.Rgba32, TiffCompression.CcittGroup3Fax, TiffCompression.CcittGroup3Fax)] [WithFile(RgbUncompressed, PixelTypes.Rgba32, TiffCompression.CcittGroup4Fax, TiffCompression.CcittGroup4Fax)] diff --git a/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs b/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs index 63c1f7fa31..e358b37f56 100644 --- a/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs +++ b/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs @@ -224,6 +224,53 @@ public void MemoryStream_WriteToSpanTests(int length) } } + [Theory] + [InlineData(16)] + [InlineData(DefaultSmallChunkSize + 16)] + public void MemoryStream_OverwriteAfterSeek_DoesNotChangeLength(int length) + { + using ChunkedMemoryStream ms = new(this.allocator); + byte[] expected = this.CreateTestBuffer(length); + ms.Write(expected, 0, expected.Length); + + // Patch bytes inside the written data, as encoders do for offsets that are only known later. + byte[] patch = [1, 2, 3, 4]; + ms.Position = length - 10; + ms.Write(patch, 0, patch.Length); + patch.CopyTo(expected, length - 10); + + Assert.Equal(length - 6, ms.Position); + Assert.Equal(length, ms.Length); + Assert.Equal(expected, ms.ToArray()); + } + + [Theory] + [InlineData(0, 16)] + [InlineData(16, 16)] + [InlineData(DefaultSmallChunkSize, 0)] + [InlineData(DefaultSmallChunkSize - 8, 16)] + [InlineData(16, DefaultSmallChunkSize * 2)] + public void MemoryStream_WriteAfterSeekPastEnd_WritesAtPosition(int length, int gap) + { + using ChunkedMemoryStream ms = new(this.allocator); + byte[] head = this.CreateTestBuffer(length); + ms.Write(head, 0, head.Length); + + // Leave a gap after the data, as encoders do for a header that is written last. + byte[] tail = [1, 2, 3, 4]; + ms.Seek(gap, SeekOrigin.Current); + ms.Write(tail, 0, tail.Length); + Assert.Equal(length + gap + tail.Length, ms.Length); + + byte[] header = this.CreateTestBuffer(gap); + ms.Position = length; + ms.Write(header, 0, header.Length); + + byte[] expected = [.. head, .. header, .. tail]; + Assert.Equal(expected.Length, ms.Length); + Assert.Equal(expected, ms.ToArray()); + } + [Fact] public void MemoryStream_WriteByteTests() { diff --git a/tests/ImageSharp.Tests/Image/ImageTests.Save.cs b/tests/ImageSharp.Tests/Image/ImageTests.Save.cs index 867930c4cf..f1df925bb1 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.Save.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.Save.cs @@ -2,6 +2,7 @@ // Licensed under the Six Labors Split License. using Moq; +using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Formats; using SixLabors.ImageSharp.Formats.Png; using SixLabors.ImageSharp.PixelFormats; @@ -57,6 +58,51 @@ public void SetEncoding() Assert.True(format is PngFormat); } + [Theory] + [InlineData("test.ani")] + [InlineData("test.bmp")] + [InlineData("test.cur")] + [InlineData("test.exr")] + [InlineData("test.gif")] + [InlineData("test.heif")] + [InlineData("test.ico")] + [InlineData("test.jpg")] + [InlineData("test.pbm")] + [InlineData("test.png")] + [InlineData("test.qoi")] + [InlineData("test.tga")] + [InlineData("test.tiff")] + [InlineData("test.webp")] + public async Task ToNonSeekableStream_SyncAndAsync_MatchSeekableStream(string filename) + { + using Image image = new(32, 24); + image.ProcessPixelRows(accessor => + { + for (int y = 0; y < accessor.Height; y++) + { + Span row = accessor.GetRowSpan(y); + for (int x = 0; x < row.Length; x++) + { + row[x] = new Rgba32((byte)(x * 8), (byte)(y * 10), (byte)((x + y) * 4)); + } + } + }); + + IImageEncoder encoder = image.DetectEncoder(filename); + + using MemoryStream seekable = new(); + image.Save(seekable, encoder); + + using MemoryStream syncTarget = new(); + image.Save(new NonSeekableStream(syncTarget), encoder); + + using MemoryStream asyncTarget = new(); + await image.SaveAsync(new NonSeekableStream(asyncTarget), encoder, TestContext.Current.CancellationToken); + + Assert.Equal(seekable.ToArray(), syncTarget.ToArray()); + Assert.Equal(seekable.ToArray(), asyncTarget.ToArray()); + } + [Fact] public void ThrowsWhenDisposed() { From 3433388f2e530338e60c2cdc87313c77700fe2e3 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 27 Oct 2026 11:54:17 +1000 Subject: [PATCH 2/2] Initialize seek gaps and strengthen buffered stream regressions Clear only bytes exposed between the previous stream end and a later write. Reuse allocator-owned chunks and leave sequential writes on the existing path. Compare gap contents, overwrites across chunk boundaries, and partial extensions with MemoryStream using the existing dirty allocator. Use CreateFrame in the multi-frame TIFF test so the image owns the added frame. Document the added regressions and reuse the expected encoded byte array. --- src/ImageSharp/IO/ChunkedMemoryStream.cs | 46 +++++++++---- .../Formats/Tiff/TiffEncoderTests.cs | 10 ++- .../IO/ChunkedMemoryStreamTests.cs | 69 +++++++++++++------ .../ImageSharp.Tests/Image/ImageTests.Save.cs | 9 ++- 4 files changed, 94 insertions(+), 40 deletions(-) diff --git a/src/ImageSharp/IO/ChunkedMemoryStream.cs b/src/ImageSharp/IO/ChunkedMemoryStream.cs index 8be3fbb75e..e12caf2dba 100644 --- a/src/ImageSharp/IO/ChunkedMemoryStream.cs +++ b/src/ImageSharp/IO/ChunkedMemoryStream.cs @@ -193,20 +193,35 @@ public override void Write(ReadOnlySpan buffer) long remaining = this.memoryChunkBuffer.Length - this.position; // Ensure we have enough capacity to write the data. - if (remaining < count) + while (remaining < count) { - // A position beyond the allocated chunks, reached by seeking past the end, has no chunk - // until the buffer grows, so its chunk location is resolved again after expanding. - bool resolvePosition = remaining < 0; - while (remaining < count) - { - this.memoryChunkBuffer.Expand(); - remaining = this.memoryChunkBuffer.Length - this.position; - } + this.memoryChunkBuffer.Expand(); + remaining = this.memoryChunkBuffer.Length - this.position; + } + + if (this.position > this.length) + { + // Seeking does not expose bytes or allocate storage. A later write exposes the skipped range, + // which must contain zeros rather than pooled data. Clear only that gap, not each new chunk. + long gap = this.position - this.length; - if (resolvePosition) + // Locate the previous end after expansion. Clearing the gap also advances the cursor to the write position. + this.SetPosition(this.length); + + while (gap > 0) { - this.SetPosition(this.position); + MemoryChunk chunk = this.memoryChunkBuffer[this.bufferIndex]; + int bytesToClear = (int)Math.Min(gap, chunk.Length - this.chunkIndex); + chunk.Buffer.Slice(this.chunkIndex, bytesToClear).Clear(); + + gap -= bytesToClear; + this.position += bytesToClear; + this.chunkIndex += bytesToClear; + if (this.chunkIndex == chunk.Length) + { + this.chunkIndex = 0; + this.bufferIndex++; + } } } @@ -345,6 +360,10 @@ protected override void Dispose(bool disposing) } } + /// + /// Moves the logical position without allocating storage or changing the stream length. + /// + /// The nonnegative byte offset from the start of the stream. private void SetPosition(long value) { long newPosition = value; @@ -359,9 +378,8 @@ private void SetPosition(long value) int currentChunkIndex = 0; long offset = newPosition; - // A position at or beyond the end of the allocated chunks starts the chunk that the next write allocates. - // This is exact at the end of the chunks, which is also where a write that fills the last chunk leaves - // the position. Write resolves a position further beyond the end once the chunks reach it. + // At the capacity boundary, the next byte belongs to the next chunk. Positions beyond capacity have + // no allocated chunk yet. Write expands storage, then traverses the gap from the previous stream end. if (offset >= this.memoryChunkBuffer.Length) { this.bufferIndex = this.memoryChunkBuffer.ChunkCount; diff --git a/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs index 2f1c36f8df..d742fa3b2a 100644 --- a/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tiff/TiffEncoderTests.cs @@ -813,13 +813,16 @@ public void TiffEncoder_WritesIfdOffsetAtWordBoundary() Assert.Equal(imageBytes[5], expectedIfdOffsetBytes[1]); } + /// + /// Checks that backpatching multiple frame directories produces identical seekable and buffered output. + /// [Fact] public async Task TiffEncoder_ToNonSeekableStream_MatchesSeekableStream() { // arrange // Each frame adds an IFD offset that is patched after the frame data is written. using Image image = new(32, 24, Color.Red.ToPixel()); - image.Frames.AddFrame(new ImageFrame(image.Configuration, 32, 24, Color.Blue.ToPixel())); + image.Frames.CreateFrame(Color.Blue.ToPixel()); TiffEncoder encoder = new(); using MemoryStream seekable = new(); image.Save(seekable, encoder); @@ -832,8 +835,9 @@ public async Task TiffEncoder_ToNonSeekableStream_MatchesSeekableStream() await image.SaveAsync(new NonSeekableStream(asyncTarget), encoder, TestContext.Current.CancellationToken); // assert - Assert.Equal(seekable.ToArray(), syncTarget.ToArray()); - Assert.Equal(seekable.ToArray(), asyncTarget.ToArray()); + byte[] expected = seekable.ToArray(); + Assert.Equal(expected, syncTarget.ToArray()); + Assert.Equal(expected, asyncTarget.ToArray()); } [Theory] diff --git a/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs b/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs index e358b37f56..9ab3a8b866 100644 --- a/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs +++ b/tests/ImageSharp.Tests/IO/ChunkedMemoryStreamTests.cs @@ -4,6 +4,7 @@ using SixLabors.ImageSharp.IO; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; +using SixLabors.ImageSharp.Tests.Memory; using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison; namespace SixLabors.ImageSharp.Tests.IO; @@ -224,51 +225,77 @@ public void MemoryStream_WriteToSpanTests(int length) } } + /// + /// Checks overwrites within a chunk, across chunks, and beyond the previous end of the stream. + /// + /// The initial stream length. + /// The offset to overwrite. [Theory] - [InlineData(16)] - [InlineData(DefaultSmallChunkSize + 16)] - public void MemoryStream_OverwriteAfterSeek_DoesNotChangeLength(int length) + [InlineData(16, 6)] + [InlineData(DefaultSmallChunkSize + 16, DefaultSmallChunkSize - 2)] + [InlineData(16, 14)] + public void MemoryStream_OverwriteAfterSeek_MatchesMemoryStream(int length, int position) { using ChunkedMemoryStream ms = new(this.allocator); - byte[] expected = this.CreateTestBuffer(length); - ms.Write(expected, 0, expected.Length); + using MemoryStream expected = new(); + byte[] head = this.CreateTestBuffer(length); + ms.Write(head, 0, head.Length); + expected.Write(head, 0, head.Length); // Patch bytes inside the written data, as encoders do for offsets that are only known later. byte[] patch = [1, 2, 3, 4]; - ms.Position = length - 10; + ms.Position = position; + expected.Position = position; ms.Write(patch, 0, patch.Length); - patch.CopyTo(expected, length - 10); + expected.Write(patch, 0, patch.Length); - Assert.Equal(length - 6, ms.Position); - Assert.Equal(length, ms.Length); - Assert.Equal(expected, ms.ToArray()); + Assert.Equal(expected.Position, ms.Position); + Assert.Equal(expected.Length, ms.Length); + Assert.Equal(expected.ToArray(), ms.ToArray()); } + /// + /// Checks seek gaps before filling the reserved header, so dirty allocator bytes cannot go undetected. + /// + /// The initial stream length. + /// The number of bytes to skip before writing. + /// The number of tail bytes to write. [Theory] - [InlineData(0, 16)] - [InlineData(16, 16)] - [InlineData(DefaultSmallChunkSize, 0)] - [InlineData(DefaultSmallChunkSize - 8, 16)] - [InlineData(16, DefaultSmallChunkSize * 2)] - public void MemoryStream_WriteAfterSeekPastEnd_WritesAtPosition(int length, int gap) + [InlineData(0, 16, 4)] + [InlineData(16, 16, 4)] + [InlineData(DefaultSmallChunkSize, 0, 4)] + [InlineData(DefaultSmallChunkSize - 8, 16, 4)] + [InlineData(16, DefaultSmallChunkSize * 2, 4)] + [InlineData(0, DefaultSmallChunkSize, 0)] + public void MemoryStream_WriteAfterSeekPastEnd_WritesAtPosition(int length, int gap, int count) { - using ChunkedMemoryStream ms = new(this.allocator); + using ChunkedMemoryStream ms = new(new TestMemoryAllocator()); + using MemoryStream expected = new(); byte[] head = this.CreateTestBuffer(length); ms.Write(head, 0, head.Length); + expected.Write(head, 0, head.Length); // Leave a gap after the data, as encoders do for a header that is written last. byte[] tail = [1, 2, 3, 4]; ms.Seek(gap, SeekOrigin.Current); - ms.Write(tail, 0, tail.Length); - Assert.Equal(length + gap + tail.Length, ms.Length); + expected.Seek(gap, SeekOrigin.Current); + ms.Write(tail, 0, count); + expected.Write(tail, 0, count); + + // MemoryStream exposes zeros in the gap. The dirty allocator makes missing initialization deterministic. + Assert.Equal(expected.Position, ms.Position); + Assert.Equal(expected.Length, ms.Length); + Assert.Equal(expected.ToArray(), ms.ToArray()); byte[] header = this.CreateTestBuffer(gap); ms.Position = length; + expected.Position = length; ms.Write(header, 0, header.Length); + expected.Write(header, 0, header.Length); - byte[] expected = [.. head, .. header, .. tail]; + Assert.Equal(expected.Position, ms.Position); Assert.Equal(expected.Length, ms.Length); - Assert.Equal(expected, ms.ToArray()); + Assert.Equal(expected.ToArray(), ms.ToArray()); } [Fact] diff --git a/tests/ImageSharp.Tests/Image/ImageTests.Save.cs b/tests/ImageSharp.Tests/Image/ImageTests.Save.cs index f1df925bb1..8f75ee383f 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.Save.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.Save.cs @@ -58,6 +58,10 @@ public void SetEncoding() Assert.True(format is PngFormat); } + /// + /// Checks that synchronous and asynchronous buffering preserve the bytes written by each encoder. + /// + /// The filename used to select the encoder. [Theory] [InlineData("test.ani")] [InlineData("test.bmp")] @@ -99,8 +103,9 @@ public async Task ToNonSeekableStream_SyncAndAsync_MatchSeekableStream(string fi using MemoryStream asyncTarget = new(); await image.SaveAsync(new NonSeekableStream(asyncTarget), encoder, TestContext.Current.CancellationToken); - Assert.Equal(seekable.ToArray(), syncTarget.ToArray()); - Assert.Equal(seekable.ToArray(), asyncTarget.ToArray()); + byte[] expected = seekable.ToArray(); + Assert.Equal(expected, syncTarget.ToArray()); + Assert.Equal(expected, asyncTarget.ToArray()); } [Fact]