From 069a825f5486f3f0c25ff5e11d912da42cbd305b Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 12 Oct 2022 22:07:54 +1000 Subject: [PATCH 1/6] Better buffered stream fix --- src/ImageSharp/IO/BufferedReadStream.cs | 28 +++++++------------------ 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/ImageSharp/IO/BufferedReadStream.cs b/src/ImageSharp/IO/BufferedReadStream.cs index 1afff21923..5a8caa5e8b 100644 --- a/src/ImageSharp/IO/BufferedReadStream.cs +++ b/src/ImageSharp/IO/BufferedReadStream.cs @@ -60,7 +60,7 @@ internal sealed class BufferedReadStream : Stream } // This triggers a full read on first attempt. - this.readBufferIndex = this.BufferSize; + this.readBufferIndex = int.MinValue; } /// @@ -96,8 +96,9 @@ internal sealed class BufferedReadStream : Stream else { // Base stream seek will throw for us if invalid. + this.BaseStream.Seek(value, SeekOrigin.Begin); this.readerPosition = value; - this.FillReadBuffer(); + this.readBufferIndex = int.MinValue; } } } @@ -140,7 +141,7 @@ internal sealed class BufferedReadStream : Stream // Our buffer has been read. // We need to refill and start again. - if (this.readBufferIndex > this.maxBufferIndex) + if (this.readBufferIndex < 0 || this.readBufferIndex > this.maxBufferIndex) { this.FillReadBuffer(); } @@ -156,22 +157,7 @@ internal sealed class BufferedReadStream : Stream /// [MethodImpl(MethodImplOptions.AggressiveInlining)] public override int Read(byte[] buffer, int offset, int count) - { - // Too big for our buffer. Read directly from the stream. - if (count > this.BufferSize) - { - return this.ReadToBufferDirectSlow(buffer, offset, count); - } - - // Too big for remaining buffer but less than entire buffer length - // Copy to buffer then read from there. - if (count + this.readBufferIndex > this.BufferSize) - { - return this.ReadToBufferViaCopySlow(buffer, offset, count); - } - - return this.ReadToBufferViaCopyFast(buffer, offset, count); - } + => this.Read(buffer.AsSpan(offset, count)); /// [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -186,7 +172,7 @@ internal sealed class BufferedReadStream : Stream // Too big for remaining buffer but less than entire buffer length // Copy to buffer then read from there. - if (count + this.readBufferIndex > this.BufferSize) + if (this.readBufferIndex < 0 || (count + this.readBufferIndex > this.BufferSize)) { return this.ReadToBufferViaCopySlow(buffer); } @@ -206,7 +192,7 @@ internal sealed class BufferedReadStream : Stream } // Reset to trigger full read on next attempt. - this.readBufferIndex = this.BufferSize; + this.readBufferIndex = int.MinValue; } /// From f941581de18fda73b5fbc1e491ddb9af64d0bac9 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 12 Oct 2022 22:33:22 +1000 Subject: [PATCH 2/6] Add test for #2259 --- src/ImageSharp/Image.FromStream.cs | 1 + .../Formats/GeneralFormatTests.cs | 27 ++++++++++++++----- tests/ImageSharp.Tests/TestImages.cs | 3 +++ tests/Images/Input/Png/issues/Issue_2259.png | 3 +++ 4 files changed, 28 insertions(+), 6 deletions(-) create mode 100644 tests/Images/Input/Png/issues/Issue_2259.png diff --git a/src/ImageSharp/Image.FromStream.cs b/src/ImageSharp/Image.FromStream.cs index eb33ea9978..0efdd5f653 100644 --- a/src/ImageSharp/Image.FromStream.cs +++ b/src/ImageSharp/Image.FromStream.cs @@ -517,6 +517,7 @@ public abstract partial class Image /// The input stream. /// The action to perform. /// The . + /// Cannot read from the stream. internal static T WithSeekableStream( DecoderOptions options, Stream stream, diff --git a/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs b/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs index fa5e4bdeff..3d50618e63 100644 --- a/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs +++ b/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs @@ -46,6 +46,21 @@ public class GeneralFormatTests image.DebugSave(provider); } + [Fact] + public void ReadOriginIsRespectedOnLoad() + { + using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259)); + using Image i = Image.Load(stream); + long position1 = stream.Position; + Assert.NotEqual(0, position1); + + using Image j = Image.Load(stream); + long position2 = stream.Position; + Assert.True(position2 > position1); + + Assert.NotEqual(i[5, 5], j[5, 5]); + } + [Fact] public void ImageCanEncodeToString() { @@ -155,15 +170,15 @@ public class GeneralFormatTests foreach (TestFile file in Files) { byte[] serialized; - using (var image = Image.Load(file.Bytes, out IImageFormat mimeType)) - using (var memoryStream = new MemoryStream()) + using (Image image = Image.Load(file.Bytes, out IImageFormat mimeType)) + using (MemoryStream memoryStream = new()) { image.Save(memoryStream, mimeType); memoryStream.Flush(); serialized = memoryStream.ToArray(); } - using var image2 = Image.Load(serialized); + using Image image2 = Image.Load(serialized); image2.Save($"{path}{Path.DirectorySeparatorChar}{file.FileName}"); } } @@ -198,8 +213,8 @@ public class GeneralFormatTests public void CanIdentifyImageLoadedFromBytes(int width, int height, string extension) { - using var image = Image.LoadPixelData(new Rgba32[width * height], width, height); - using var memoryStream = new MemoryStream(); + using Image image = Image.LoadPixelData(new Rgba32[width * height], width, height); + using MemoryStream memoryStream = new(); IImageFormat format = GetFormat(extension); image.Save(memoryStream, format); memoryStream.Position = 0; @@ -220,7 +235,7 @@ public class GeneralFormatTests { byte[] invalid = new byte[10]; - using var memoryStream = new MemoryStream(invalid); + using MemoryStream memoryStream = new(invalid); IImageInfo imageInfo = Image.Identify(memoryStream, out IImageFormat format); Assert.Null(imageInfo); diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 96b9381517..767ec34234 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -130,6 +130,9 @@ public static class TestImages // Issue 2209: https://github.com/SixLabors/ImageSharp/issues/2209 public const string Issue2209IndexedWithTransparency = "Png/issues/Issue_2209.png"; + // Issue 2259: https://github.com/SixLabors/ImageSharp/issues/2259 + public const string Issue2259 = "Png/issues/Issue_2259.png"; + public static class Bad { public const string MissingDataChunk = "Png/xdtn0g01.png"; diff --git a/tests/Images/Input/Png/issues/Issue_2259.png b/tests/Images/Input/Png/issues/Issue_2259.png new file mode 100644 index 0000000000..25bf261e41 --- /dev/null +++ b/tests/Images/Input/Png/issues/Issue_2259.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:45b635fe3104e96b7c6d8fa9d28f7fb86fb184c7a407c44a2f90f3f88c140ef0 +size 6037 From bf07e4507c38362cc2a2c6faeb52fef13d8dab21 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 12 Oct 2022 22:50:47 +1000 Subject: [PATCH 3/6] Add test and fix for asynchronous streams. --- src/ImageSharp/Image.FromStream.cs | 15 ++++++++++++++- .../Formats/GeneralFormatTests.cs | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/ImageSharp/Image.FromStream.cs b/src/ImageSharp/Image.FromStream.cs index 0efdd5f653..eefb087764 100644 --- a/src/ImageSharp/Image.FromStream.cs +++ b/src/ImageSharp/Image.FromStream.cs @@ -588,7 +588,20 @@ public abstract partial class Image await stream.CopyToAsync(memoryStream, configuration.StreamProcessingBufferSize, cancellationToken).ConfigureAwait(false); memoryStream.Position = 0; - return action(memoryStream, cancellationToken); + T Action(Stream ms, CancellationToken ct) + { + // Reset the position of the seekable stream if we did not read to the end + // to allow additional reads. + T result = action(ms, ct); + if (stream.CanSeek && ms.Position != ms.Length) + { + stream.Position = ms.Position; + } + + return result; + } + + return Action(memoryStream, cancellationToken); } [DoesNotReturn] diff --git a/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs b/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs index 3d50618e63..172821acc9 100644 --- a/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs +++ b/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs @@ -61,6 +61,21 @@ public class GeneralFormatTests Assert.NotEqual(i[5, 5], j[5, 5]); } + [Fact] + public async Task ReadOriginIsRespectedOnLoadAsync() + { + using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259)); + using Image i = await Image.LoadAsync(stream); + long position1 = stream.Position; + Assert.NotEqual(0, position1); + + using Image j = await Image.LoadAsync(stream); + long position2 = stream.Position; + Assert.True(position2 > position1); + + Assert.NotEqual(i[5, 5], j[5, 5]); + } + [Fact] public void ImageCanEncodeToString() { From 544c14d12140a9ef421a47cfb9aba54d12df5081 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 13 Oct 2022 20:09:24 +1000 Subject: [PATCH 4/6] Update src/ImageSharp/IO/BufferedReadStream.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/ImageSharp/IO/BufferedReadStream.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/IO/BufferedReadStream.cs b/src/ImageSharp/IO/BufferedReadStream.cs index 5a8caa5e8b..6607c2637b 100644 --- a/src/ImageSharp/IO/BufferedReadStream.cs +++ b/src/ImageSharp/IO/BufferedReadStream.cs @@ -172,7 +172,7 @@ internal sealed class BufferedReadStream : Stream // Too big for remaining buffer but less than entire buffer length // Copy to buffer then read from there. - if (this.readBufferIndex < 0 || (count + this.readBufferIndex > this.BufferSize)) + if (this.readBufferIndex < 0 || (this.readBufferIndex > this.BufferSize - count)) { return this.ReadToBufferViaCopySlow(buffer); } From 4b71d25bffc8261efb34dcbfccb3ce0e9509b054 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 13 Oct 2022 21:25:09 +1100 Subject: [PATCH 5/6] Update src/ImageSharp/IO/BufferedReadStream.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/ImageSharp/IO/BufferedReadStream.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/IO/BufferedReadStream.cs b/src/ImageSharp/IO/BufferedReadStream.cs index 6607c2637b..f8444740da 100644 --- a/src/ImageSharp/IO/BufferedReadStream.cs +++ b/src/ImageSharp/IO/BufferedReadStream.cs @@ -141,7 +141,7 @@ internal sealed class BufferedReadStream : Stream // Our buffer has been read. // We need to refill and start again. - if (this.readBufferIndex < 0 || this.readBufferIndex > this.maxBufferIndex) + if ((uint)this.readBufferIndex > (uint)this.maxBufferIndex) { this.FillReadBuffer(); } From b14622a3391a7ff40ccc44c0a29b1e525c1895ed Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 13 Oct 2022 20:27:28 +1000 Subject: [PATCH 6/6] Manually commit @gfoidl suggestion --- src/ImageSharp/IO/BufferedReadStream.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/IO/BufferedReadStream.cs b/src/ImageSharp/IO/BufferedReadStream.cs index f8444740da..291a318864 100644 --- a/src/ImageSharp/IO/BufferedReadStream.cs +++ b/src/ImageSharp/IO/BufferedReadStream.cs @@ -172,7 +172,7 @@ internal sealed class BufferedReadStream : Stream // Too big for remaining buffer but less than entire buffer length // Copy to buffer then read from there. - if (this.readBufferIndex < 0 || (this.readBufferIndex > this.BufferSize - count)) + if ((uint)this.readBufferIndex > (uint)(this.BufferSize - count)) { return this.ReadToBufferViaCopySlow(buffer); }