PBM decoder robustness improvements and BufferedReadStream observability (#2551)

* handle premature EOF in the PBM decoder

* BufferedReadStreamExtensions: remove the 'Try' prefix

* count EOF hits in BufferedReadStream

* use EofHitCounter in pbm tests

* Naming convention tweaks

---------

Co-authored-by: James Jackson-South <james_south@hotmail.com>

src/ImageSharp/Formats/ImageDecoderUtilities.cs

@@ -50,7 +50,8 @@ internal static class ImageDecoderUtilities
         CancellationToken cancellationToken)
         where TPixel : unmanaged, IPixel<TPixel>
     {
-        using BufferedReadStream bufferedReadStream = new(configuration, stream, cancellationToken);
+        // Test may pass a BufferedReadStream in order to monitor EOF hits, if so, use the existing instance.
+        BufferedReadStream bufferedReadStream = stream as BufferedReadStream ?? new BufferedReadStream(configuration, stream, cancellationToken);
 
         try
         {
@@ -64,6 +65,13 @@ internal static class ImageDecoderUtilities
         {
             throw;
         }
+        finally
+        {
+            if (bufferedReadStream != stream)
+            {
+                bufferedReadStream.Dispose();
+            }
+        }
     }
 
     private static InvalidImageContentException DefaultLargeImageExceptionFactory(

src/ImageSharp/Formats/Pbm/BinaryDecoder.cs

@@ -71,7 +71,11 @@ internal class BinaryDecoder
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) == 0)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromL8Bytes(
                 configuration,
@@ -93,7 +97,11 @@ internal class BinaryDecoder
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) == 0)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromL16Bytes(
                 configuration,
@@ -115,7 +123,11 @@ internal class BinaryDecoder
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) == 0)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromRgb24Bytes(
                 configuration,
@@ -137,7 +149,11 @@ internal class BinaryDecoder
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) == 0)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromRgb48Bytes(
                 configuration,
@@ -161,6 +177,11 @@ internal class BinaryDecoder
             for (int x = 0; x < width;)
             {
                 int raw = stream.ReadByte();
+                if (raw < 0)
+                {
+                    return;
+                }
+
                 int stopBit = Math.Min(8, width - x);
                 for (int bit = 0; bit < stopBit; bit++)
                 {

src/ImageSharp/Formats/Pbm/BufferedReadStreamExtensions.cs

@@ -11,14 +11,20 @@ namespace SixLabors.ImageSharp.Formats.Pbm;
 internal static class BufferedReadStreamExtensions
 {
     /// <summary>
-    /// Skip over any whitespace or any comments.
+    /// Skip over any whitespace or any comments and signal if EOF has been reached.
     /// </summary>
-    public static void SkipWhitespaceAndComments(this BufferedReadStream stream)
+    /// <param name="stream">The buffered read stream.</param>
+    /// <returns><see langword="false"/> if EOF has been reached while reading the stream; see langword="true"/> otherwise.</returns>
+    public static bool TrySkipWhitespaceAndComments(this BufferedReadStream stream)
     {
         bool isWhitespace;
         do
         {
             int val = stream.ReadByte();
+            if (val < 0)
+            {
+                return false;
+            }
 
             // Comments start with '#' and end at the next new-line.
             if (val == 0x23)
@@ -27,8 +33,12 @@ internal static class BufferedReadStreamExtensions
                 do
                 {
                     innerValue = stream.ReadByte();
+                    if (innerValue < 0)
+                    {
+                        return false;
+                    }
                 }
-                while (innerValue is not 0x0a and not -0x1);
+                while (innerValue is not 0x0a);
 
                 // Continue searching for whitespace.
                 val = innerValue;
@@ -38,18 +48,31 @@ internal static class BufferedReadStreamExtensions
         }
         while (isWhitespace);
         stream.Seek(-1, SeekOrigin.Current);
+        return true;
     }
 
     /// <summary>
-    /// Read a decimal text value.
+    /// Read a decimal text value and signal if EOF has been reached.
     /// </summary>
-    /// <returns>The integer value of the decimal.</returns>
+    /// <param name="stream">The buffered read stream.</param>
-    public static int ReadDecimal(this BufferedReadStream stream)
+    /// <param name="value">The read value.</param>
+    /// <returns><see langword="false"/> if EOF has been reached while reading the stream; <see langword="true"/> otherwise.</returns>
+    /// <remarks>
+    /// A 'false' return value doesn't mean that the parsing has been failed, since it's possible to reach EOF while reading the last decimal in the file.
+    /// It's up to the call site to handle such a situation.
+    /// </remarks>
+    public static bool TryReadDecimal(this BufferedReadStream stream, out int value)
     {
-        int value = 0;
+        value = 0;
         while (true)
         {
-            int current = stream.ReadByte() - 0x30;
+            int current = stream.ReadByte();
+            if (current < 0)
+            {
+                return false;
+            }
+
+            current -= 0x30;
             if ((uint)current > 9)
             {
                 break;
@@ -58,6 +81,6 @@ internal static class BufferedReadStreamExtensions
             value = (value * 10) + current;
         }
 
-        return value;
+        return true;
     }
 }

src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
+using System.Diagnostics.CodeAnalysis;
 using SixLabors.ImageSharp.IO;
 using SixLabors.ImageSharp.Memory;
 using SixLabors.ImageSharp.Metadata;
@@ -95,6 +96,7 @@ internal sealed class PbmDecoderCore : IImageDecoderInternals
     /// Processes the ppm header.
     /// </summary>
     /// <param name="stream">The input stream.</param>
+    /// <exception cref="InvalidImageContentException">An EOF marker has been read before the image has been decoded.</exception>
     private void ProcessHeader(BufferedReadStream stream)
     {
         Span<byte> buffer = stackalloc byte[2];
@@ -144,14 +146,22 @@ internal sealed class PbmDecoderCore : IImageDecoderInternals
                 throw new InvalidImageContentException("Unknown of not implemented image type encountered.");
         }
 
-        stream.SkipWhitespaceAndComments();
+        if (!stream.TrySkipWhitespaceAndComments() ||
-        int width = stream.ReadDecimal();
+            !stream.TryReadDecimal(out int width) ||
-        stream.SkipWhitespaceAndComments();
+            !stream.TrySkipWhitespaceAndComments() ||
-        int height = stream.ReadDecimal();
+            !stream.TryReadDecimal(out int height) ||
-        stream.SkipWhitespaceAndComments();
+            !stream.TrySkipWhitespaceAndComments())
+        {
+            ThrowPrematureEof();
+        }
+
         if (this.colorType != PbmColorType.BlackAndWhite)
         {
-            this.maxPixelValue = stream.ReadDecimal();
+            if (!stream.TryReadDecimal(out this.maxPixelValue))
+            {
+                ThrowPrematureEof();
+            }
+
             if (this.maxPixelValue > 255)
             {
                 this.componentType = PbmComponentType.Short;
@@ -161,7 +171,7 @@ internal sealed class PbmDecoderCore : IImageDecoderInternals
                 this.componentType = PbmComponentType.Byte;
             }
 
-            stream.SkipWhitespaceAndComments();
+            stream.TrySkipWhitespaceAndComments();
         }
         else
         {
@@ -174,6 +184,9 @@ internal sealed class PbmDecoderCore : IImageDecoderInternals
         meta.Encoding = this.encoding;
         meta.ColorType = this.colorType;
         meta.ComponentType = this.componentType;
+
+        [DoesNotReturn]
+        static void ThrowPrematureEof() => throw new InvalidImageContentException("Reached EOF while reading the header.");
     }
 
     private void ProcessPixels<TPixel>(BufferedReadStream stream, Buffer2D<TPixel> pixels)

src/ImageSharp/Formats/Pbm/PlainDecoder.cs

@@ -65,13 +65,18 @@ internal class PlainDecoder
         using IMemoryOwner<L8> row = allocator.Allocate<L8>(width);
         Span<L8> rowSpan = row.GetSpan();
 
+        bool eofReached = false;
         for (int y = 0; y < height; y++)
         {
             for (int x = 0; x < width; x++)
             {
-                byte value = (byte)stream.ReadDecimal();
+                stream.TryReadDecimal(out int value);
-                stream.SkipWhitespaceAndComments();
+                rowSpan[x] = new L8((byte)value);
-                rowSpan[x] = new L8(value);
+                eofReached = !stream.TrySkipWhitespaceAndComments();
+                if (eofReached)
+                {
+                    break;
+                }
             }
 
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -79,6 +84,11 @@ internal class PlainDecoder
                 configuration,
                 rowSpan,
                 pixelSpan);
+
+            if (eofReached)
+            {
+                return;
+            }
         }
     }
 
@@ -91,13 +101,18 @@ internal class PlainDecoder
         using IMemoryOwner<L16> row = allocator.Allocate<L16>(width);
         Span<L16> rowSpan = row.GetSpan();
 
+        bool eofReached = false;
         for (int y = 0; y < height; y++)
         {
             for (int x = 0; x < width; x++)
             {
-                ushort value = (ushort)stream.ReadDecimal();
+                stream.TryReadDecimal(out int value);
-                stream.SkipWhitespaceAndComments();
+                rowSpan[x] = new L16((ushort)value);
-                rowSpan[x] = new L16(value);
+                eofReached = !stream.TrySkipWhitespaceAndComments();
+                if (eofReached)
+                {
+                    break;
+                }
             }
 
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -105,6 +120,11 @@ internal class PlainDecoder
                 configuration,
                 rowSpan,
                 pixelSpan);
+
+            if (eofReached)
+            {
+                return;
+            }
         }
     }
 
@@ -117,17 +137,29 @@ internal class PlainDecoder
         using IMemoryOwner<Rgb24> row = allocator.Allocate<Rgb24>(width);
         Span<Rgb24> rowSpan = row.GetSpan();
 
+        bool eofReached = false;
         for (int y = 0; y < height; y++)
         {
             for (int x = 0; x < width; x++)
             {
-                byte red = (byte)stream.ReadDecimal();
+                if (!stream.TryReadDecimal(out int red) ||
-                stream.SkipWhitespaceAndComments();
+                    !stream.TrySkipWhitespaceAndComments() ||
-                byte green = (byte)stream.ReadDecimal();
+                    !stream.TryReadDecimal(out int green) ||
-                stream.SkipWhitespaceAndComments();
+                    !stream.TrySkipWhitespaceAndComments())
-                byte blue = (byte)stream.ReadDecimal();
+                {
-                stream.SkipWhitespaceAndComments();
+                    // Reached EOF before reading a full RGB value
-                rowSpan[x] = new Rgb24(red, green, blue);
+                    eofReached = true;
+                    break;
+                }
+
+                stream.TryReadDecimal(out int blue);
+
+                rowSpan[x] = new Rgb24((byte)red, (byte)green, (byte)blue);
+                eofReached = !stream.TrySkipWhitespaceAndComments();
+                if (eofReached)
+                {
+                    break;
+                }
             }
 
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -135,6 +167,11 @@ internal class PlainDecoder
                 configuration,
                 rowSpan,
                 pixelSpan);
+
+            if (eofReached)
+            {
+                return;
+            }
         }
     }
 
@@ -147,17 +184,29 @@ internal class PlainDecoder
         using IMemoryOwner<Rgb48> row = allocator.Allocate<Rgb48>(width);
         Span<Rgb48> rowSpan = row.GetSpan();
 
+        bool eofReached = false;
         for (int y = 0; y < height; y++)
         {
             for (int x = 0; x < width; x++)
             {
-                ushort red = (ushort)stream.ReadDecimal();
+                if (!stream.TryReadDecimal(out int red) ||
-                stream.SkipWhitespaceAndComments();
+                    !stream.TrySkipWhitespaceAndComments() ||
-                ushort green = (ushort)stream.ReadDecimal();
+                    !stream.TryReadDecimal(out int green) ||
-                stream.SkipWhitespaceAndComments();
+                    !stream.TrySkipWhitespaceAndComments())
-                ushort blue = (ushort)stream.ReadDecimal();
+                {
-                stream.SkipWhitespaceAndComments();
+                    // Reached EOF before reading a full RGB value
-                rowSpan[x] = new Rgb48(red, green, blue);
+                    eofReached = true;
+                    break;
+                }
+
+                stream.TryReadDecimal(out int blue);
+
+                rowSpan[x] = new Rgb48((ushort)red, (ushort)green, (ushort)blue);
+                eofReached = !stream.TrySkipWhitespaceAndComments();
+                if (eofReached)
+                {
+                    break;
+                }
             }
 
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -165,6 +214,11 @@ internal class PlainDecoder
                 configuration,
                 rowSpan,
                 pixelSpan);
+
+            if (eofReached)
+            {
+                return;
+            }
         }
     }
 
@@ -177,13 +231,19 @@ internal class PlainDecoder
         using IMemoryOwner<L8> row = allocator.Allocate<L8>(width);
         Span<L8> rowSpan = row.GetSpan();
 
+        bool eofReached = false;
         for (int y = 0; y < height; y++)
         {
             for (int x = 0; x < width; x++)
             {
-                int value = stream.ReadDecimal();
+                stream.TryReadDecimal(out int value);
-                stream.SkipWhitespaceAndComments();
+
                 rowSpan[x] = value == 0 ? White : Black;
+                eofReached = !stream.TrySkipWhitespaceAndComments();
+                if (eofReached)
+                {
+                    break;
+                }
             }
 
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -191,6 +251,11 @@ internal class PlainDecoder
                 configuration,
                 rowSpan,
                 pixelSpan);
+
+            if (eofReached)
+            {
+                return;
+            }
         }
     }
 }

src/ImageSharp/IO/BufferedReadStream.cs

@@ -68,6 +68,11 @@ internal sealed class BufferedReadStream : Stream
         this.readBufferIndex = int.MinValue;
     }
 
+    /// <summary>
+    /// Gets the number indicating the EOF hits occured while reading from this instance.
+    /// </summary>
+    public int EofHitCount { get; private set; }
+
     /// <summary>
     /// Gets the size, in bytes, of the underlying buffer.
     /// </summary>
@@ -142,6 +147,7 @@ internal sealed class BufferedReadStream : Stream
     {
         if (this.readerPosition >= this.Length)
         {
+            this.EofHitCount++;
             return -1;
         }
 
@@ -294,7 +300,7 @@ internal sealed class BufferedReadStream : Stream
 
         this.readerPosition += n;
         this.readBufferIndex += n;
-
+        this.CheckEof(n);
         return n;
     }
 
@@ -352,6 +358,7 @@ internal sealed class BufferedReadStream : Stream
 
         this.Position += n;
 
+        this.CheckEof(n);
         return n;
     }
 
@@ -418,4 +425,13 @@ internal sealed class BufferedReadStream : Stream
             Buffer.BlockCopy(this.readBuffer, this.readBufferIndex, buffer, offset, count);
         }
     }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    private void CheckEof(int read)
+    {
+        if (read == 0)
+        {
+            this.EofHitCount++;
+        }
+    }
 }

tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs

@@ -1,9 +1,11 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
+using System.Text;
 using SixLabors.ImageSharp.Formats;
 using SixLabors.ImageSharp.Formats.Pbm;
 using SixLabors.ImageSharp.PixelFormats;
+using SixLabors.ImageSharp.Tests.TestUtilities;
 using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison;
 using static SixLabors.ImageSharp.Tests.TestImages.Pbm;
 
@@ -120,4 +122,23 @@ public class PbmDecoderTests
             testOutputDetails: details,
             appendPixelTypeToFileName: false);
     }
+
+    [Fact]
+    public void PlainText_PrematureEof()
+    {
+        byte[] bytes = Encoding.ASCII.GetBytes($"P1\n100 100\n1 0 1 0 1 0");
+        using EofHitCounter eofHitCounter = EofHitCounter.RunDecoder(bytes);
+
+        Assert.True(eofHitCounter.EofHitCount <= 2);
+        Assert.Equal(new Size(100, 100), eofHitCounter.Image.Size);
+    }
+
+    [Fact]
+    public void Binary_PrematureEof()
+    {
+        using EofHitCounter eofHitCounter = EofHitCounter.RunDecoder(RgbBinaryPrematureEof);
+
+        Assert.True(eofHitCounter.EofHitCount <= 2);
+        Assert.Equal(new Size(29, 30), eofHitCounter.Image.Size);
+    }
 }

tests/ImageSharp.Tests/Formats/Pbm/PbmMetadataTests.cs

@@ -83,12 +83,9 @@ public class PbmMetadataTests
     }
 
     [Fact]
-    public void Identify_HandlesCraftedDenialOfServiceString()
+    public void Identify_EofInHeader_ThrowsInvalidImageContentException()
     {
         byte[] bytes = Convert.FromBase64String("UDEjWAAACQAAAAA=");
-        ImageInfo info = Image.Identify(bytes);
+        Assert.Throws<InvalidImageContentException>(() => Image.Identify(bytes));
-        Assert.Equal(default, info.Size);
-        Configuration.Default.ImageFormatsManager.TryFindFormatByFileExtension("pbm", out IImageFormat format);
-        Assert.Equal(format!, info.Metadata.DecodedImageFormat);
     }
 }

tests/ImageSharp.Tests/TestImages.cs

@@ -1043,6 +1043,7 @@ public static class TestImages
         public const string GrayscalePlainNormalized = "Pbm/grayscale_plain_normalized.pgm";
         public const string GrayscalePlainMagick = "Pbm/grayscale_plain_magick.pgm";
         public const string RgbBinary = "Pbm/00000_00000.ppm";
+        public const string RgbBinaryPrematureEof = "Pbm/00000_00000_premature_eof.ppm";
         public const string RgbPlain = "Pbm/rgb_plain.ppm";
         public const string RgbPlainNormalized = "Pbm/rgb_plain_normalized.ppm";
         public const string RgbPlainMagick = "Pbm/rgb_plain_magick.ppm";

tests/ImageSharp.Tests/TestUtilities/EofHitCounter.cs

@@ -0,0 +1,36 @@
+// Copyright (c) Six Labors.
+// Licensed under the Six Labors Split License.
+
+using SixLabors.ImageSharp.IO;
+
+namespace SixLabors.ImageSharp.Tests.TestUtilities;
+
+internal class EofHitCounter : IDisposable
+{
+    private readonly BufferedReadStream stream;
+
+    public EofHitCounter(BufferedReadStream stream, Image image)
+    {
+        this.stream = stream;
+        this.Image = image;
+    }
+
+    public int EofHitCount => this.stream.EofHitCount;
+
+    public Image Image { get; private set; }
+
+    public static EofHitCounter RunDecoder(string testImage) => RunDecoder(TestFile.Create(testImage).Bytes);
+
+    public static EofHitCounter RunDecoder(byte[] imageData)
+    {
+        BufferedReadStream stream = new(Configuration.Default, new MemoryStream(imageData));
+        Image image = Image.Load(stream);
+        return new EofHitCounter(stream, image);
+    }
+
+    public void Dispose()
+    {
+        this.stream.Dispose();
+        this.Image.Dispose();
+    }
+}

tests/Images/Input/Pbm/00000_00000_premature_eof.ppm

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:39cf6ca5b2f9d428c0c33e0fc7ab5e92c31e0c8a7d9e0276b9285f51a8ff547c
+size 69