Better tests for stream synchronization

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<T> WithSeekableStreamAsync<T>(
@@ -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);
     }
 }

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<Rgb24> i = Image.Load<Rgb24>(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<Rgb24> i = Image.Load<Rgb24>(wrapper);
+
+        Assert.Equal(stream.Length, stream.Position);
+        Assert.Throws<UnknownImageFormatException>(() => { using Image<Rgb24> j = Image.Load<Rgb24>(wrapper); });
+    }
+
+    [Fact]
+    public async Task ChainedReadOriginIsRespectedForSeekableStreamsOnLoadAsync()
     {
         using FileStream stream = File.OpenRead(TestFile.GetInputFileFullPath(TestImages.Png.Issue2259));
         using Image<Rgb24> i = await Image.LoadAsync<Rgb24>(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<Rgb24> i = await Image.LoadAsync<Rgb24>(wrapper);
+
+        Assert.Equal(stream.Length, stream.Position);
+        await Assert.ThrowsAsync<UnknownImageFormatException>(async () => { using Image<Rgb24> j = await Image.LoadAsync<Rgb24>(wrapper); });
+    }
+
     [Fact]
     public void ImageCanEncodeToString()
     {

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<byte> 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<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
+        => this.dataStream.ReadAsync(buffer, offset, count, cancellationToken);
+
+    public override ValueTask<int> ReadAsync(Memory<byte> 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<byte> 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<byte> 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();