From 28243df8a4d58e629d1bce7677d6716a0cfb7a5b Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Fri, 25 Nov 2022 12:49:07 +1000 Subject: [PATCH] Better tests for stream synchronization --- src/ImageSharp/Formats/ImageDecoder.cs | 17 +++-- .../Formats/GeneralFormatTests.cs | 27 +++++++- .../Image/NonSeekableStream.cs | 64 +++++++++++++++++-- 3 files changed, 95 insertions(+), 13 deletions(-) diff --git a/src/ImageSharp/Formats/ImageDecoder.cs b/src/ImageSharp/Formats/ImageDecoder.cs index b1ef3cbcb..2ef3c7b75 100644 --- a/src/ImageSharp/Formats/ImageDecoder.cs +++ b/src/ImageSharp/Formats/ImageDecoder.cs @@ -153,12 +153,13 @@ public abstract class ImageDecoder : IImageDecoder throw new NotSupportedException("Cannot read from the stream."); } - T PeformActionAndResetPosittion(Stream s, long position) + T PeformActionAndResetPosition(Stream s, long position) { T result = action(s); // Issue #2259. Our buffered reads may have left the stream in an incorrect non-zero position. // Reset the position of the seekable stream if we did not read to the end to allow additional reads. + // The stream is always seekable in this scenario. if (stream.Position != s.Position && s.Position != s.Length) { stream.Position = position + s.Position; @@ -169,7 +170,7 @@ public abstract class ImageDecoder : IImageDecoder if (stream.CanSeek) { - return PeformActionAndResetPosittion(stream, stream.Position); + return PeformActionAndResetPosition(stream, stream.Position); } Configuration configuration = options.Configuration; @@ -177,7 +178,7 @@ public abstract class ImageDecoder : IImageDecoder stream.CopyTo(memoryStream, configuration.StreamProcessingBufferSize); memoryStream.Position = 0; - return PeformActionAndResetPosittion(memoryStream, 0); + return action(memoryStream); } internal static async Task WithSeekableStreamAsync( @@ -200,14 +201,15 @@ public abstract class ImageDecoder : IImageDecoder // Issue #2259. Our buffered reads may have left the stream in an incorrect non-zero position. // Reset the position of the seekable stream if we did not read to the end to allow additional reads. - if (stream.Position != s.Position && s.Position != s.Length) + // We check here that the input stream is seekable because it is not guaranteed to be so since + // we always copy input streams of unknown type. + if (stream.CanSeek && stream.Position != s.Position && s.Position != s.Length) { stream.Position = position + s.Position; } // TODO: This is a hack. Our decoders do not check for cancellation requests. - // We need to fix this properly by implemented each decoder or allowing passing a token to BufferedReadStream - // which can do the check. + // We need to fix this properly by implemented each decoder. if (ct.IsCancellationRequested) { throw new TaskCanceledException(); @@ -229,10 +231,11 @@ public abstract class ImageDecoder : IImageDecoder return PeformActionAndResetPosition(cms, cms.Position, cancellationToken); } + long position = stream.CanSeek ? stream.Position : 0; Configuration configuration = options.Configuration; using ChunkedMemoryStream memoryStream = new(configuration.MemoryAllocator); await stream.CopyToAsync(memoryStream, configuration.StreamProcessingBufferSize, cancellationToken).ConfigureAwait(false); memoryStream.Position = 0; - return PeformActionAndResetPosition(memoryStream, 0, cancellationToken); + return PeformActionAndResetPosition(memoryStream, position, cancellationToken); } } diff --git a/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs b/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs index 172821acc..c9e50337e 100644 --- a/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs +++ b/tests/ImageSharp.Tests/Formats/GeneralFormatTests.cs @@ -7,6 +7,7 @@ using SixLabors.ImageSharp.Formats.Png; using SixLabors.ImageSharp.PixelFormats; using SixLabors.ImageSharp.Processing; using SixLabors.ImageSharp.Processing.Processors.Quantization; +using SixLabors.ImageSharp.Tests.TestUtilities; namespace SixLabors.ImageSharp.Tests.Formats; @@ -47,7 +48,7 @@ public class GeneralFormatTests } [Fact] - public void ReadOriginIsRespectedOnLoad() + public void ChainedReadOriginIsRespectedForSeekableStreamsOnLoad() { using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259)); using Image i = Image.Load(stream); @@ -62,7 +63,18 @@ public class GeneralFormatTests } [Fact] - public async Task ReadOriginIsRespectedOnLoadAsync() + public void ChainedReadOnLoadNonSeekable_ThrowsUnknownImageFormatException() + { + using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259)); + using NonSeekableStream wrapper = new(stream); + using Image i = Image.Load(wrapper); + + Assert.Equal(stream.Length, stream.Position); + Assert.Throws(() => { using Image j = Image.Load(wrapper); }); + } + + [Fact] + public async Task ChainedReadOriginIsRespectedForSeekableStreamsOnLoadAsync() { using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259)); using Image i = await Image.LoadAsync(stream); @@ -76,6 +88,17 @@ public class GeneralFormatTests Assert.NotEqual(i[5, 5], j[5, 5]); } + [Fact] + public async Task ChainedReadOnLoadNonSeekable_ThrowsUnknownImageFormatException_Async() + { + using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259)); + using NonSeekableStream wrapper = new(stream); + using Image i = await Image.LoadAsync(wrapper); + + Assert.Equal(stream.Length, stream.Position); + await Assert.ThrowsAsync(async () => { using Image j = await Image.LoadAsync(wrapper); }); + } + [Fact] public void ImageCanEncodeToString() { diff --git a/tests/ImageSharp.Tests/Image/NonSeekableStream.cs b/tests/ImageSharp.Tests/Image/NonSeekableStream.cs index e9b64d3b2..4b1f6e156 100644 --- a/tests/ImageSharp.Tests/Image/NonSeekableStream.cs +++ b/tests/ImageSharp.Tests/Image/NonSeekableStream.cs @@ -1,4 +1,4 @@ -// Copyright (c) Six Labors. +// Copyright (c) Six Labors. // Licensed under the Six Labors Split License. namespace SixLabors.ImageSharp.Tests; @@ -16,17 +16,73 @@ internal class NonSeekableStream : Stream public override bool CanWrite => false; + public override bool CanTimeout => this.dataStream.CanTimeout; + public override long Length => throw new NotSupportedException(); public override long Position { - get { throw new NotSupportedException(); } - set { throw new NotSupportedException(); } + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override int ReadTimeout + { + get => this.dataStream.ReadTimeout; + set => this.dataStream.ReadTimeout = value; + } + + public override int WriteTimeout + { + get => this.dataStream.WriteTimeout; + set => this.dataStream.WriteTimeout = value; } public override void Flush() => this.dataStream.Flush(); - public override int Read(byte[] buffer, int offset, int count) => this.dataStream.Read(buffer, offset, count); + public override int ReadByte() => this.dataStream.ReadByte(); + + public override int Read(byte[] buffer, int offset, int count) + => this.dataStream.Read(buffer, offset, count); + + public override int Read(Span buffer) + => this.dataStream.Read(buffer); + + public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, object state) + => this.dataStream.BeginRead(buffer, offset, count, callback, state); + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + => this.dataStream.ReadAsync(buffer, offset, count, cancellationToken); + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + => this.dataStream.ReadAsync(buffer, cancellationToken); + + public override int EndRead(IAsyncResult asyncResult) + => this.dataStream.EndRead(asyncResult); + + public override void WriteByte(byte value) => this.dataStream.WriteByte(value); + + public override void Write(ReadOnlySpan buffer) => this.dataStream.Write(buffer); + + public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object state) + => this.dataStream.BeginWrite(buffer, offset, count, callback, state); + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + => this.dataStream.WriteAsync(buffer, offset, count, cancellationToken); + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + => this.dataStream.WriteAsync(buffer, cancellationToken); + + public override void EndWrite(IAsyncResult asyncResult) => this.dataStream.EndWrite(asyncResult); + + public override void CopyTo(Stream destination, int bufferSize) => this.dataStream.CopyTo(destination, bufferSize); + + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) + => this.dataStream.CopyToAsync(destination, bufferSize, cancellationToken); + + public override Task FlushAsync(CancellationToken cancellationToken) => this.dataStream.FlushAsync(cancellationToken); + + public override void Close() => this.dataStream.Close(); public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException();