PBM decoder robustness improvements and BufferedReadStream observability (#2551)

Author: antonfirsov

src/ImageSharp/Formats/ImageDecoderUtilities.cs

@@ -50,7 +50,8 @@ internal static Image<TPixel> Decode<TPixel>(
         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 Image<TPixel> Decode<TPixel>(
         {
             throw;
         }
+        finally
+        {
+            if (bufferedReadStream != stream)
+            {
+                bufferedReadStream.Dispose();
+            }
+        }
     }
 
     private static InvalidImageContentException DefaultLargeImageExceptionFactory(

src/ImageSharp/Formats/Pbm/BinaryDecoder.cs

@@ -71,7 +71,11 @@ private static void ProcessGrayscale<TPixel>(Configuration configuration, Buffer
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) < rowSpan.Length)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromL8Bytes(
                 configuration,
@@ -93,7 +97,11 @@ private static void ProcessWideGrayscale<TPixel>(Configuration configuration, Bu
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) < rowSpan.Length)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromL16Bytes(
                 configuration,
@@ -115,7 +123,11 @@ private static void ProcessRgb<TPixel>(Configuration configuration, Buffer2D<TPi
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) < rowSpan.Length)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromRgb24Bytes(
                 configuration,
@@ -137,7 +149,11 @@ private static void ProcessWideRgb<TPixel>(Configuration configuration, Buffer2D
 
         for (int y = 0; y < height; y++)
         {
-            stream.Read(rowSpan);
+            if (stream.Read(rowSpan) < rowSpan.Length)
+            {
+                return;
+            }
+
             Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
             PixelOperations<TPixel>.Instance.FromRgb48Bytes(
                 configuration,
@@ -152,7 +168,6 @@ private static void ProcessBlackAndWhite<TPixel>(Configuration configuration, Bu
     {
         int width = pixels.Width;
         int height = pixels.Height;
-        int startBit = 0;
         MemoryAllocator allocator = configuration.MemoryAllocator;
         using IMemoryOwner<L8> row = allocator.Allocate<L8>(width);
         Span<L8> rowSpan = row.GetSpan();
@@ -162,23 +177,17 @@ private static void ProcessBlackAndWhite<TPixel>(Configuration configuration, Bu
             for (int x = 0; x < width;)
             {
                 int raw = stream.ReadByte();
-                int bit = startBit;
-                startBit = 0;
-                for (; bit < 8; bit++)
+                if (raw < 0)
+                {
+                    return;
+                }
+
+                int stopBit = Math.Min(8, width - x);
+                for (int bit = 0; bit < stopBit; bit++)
                 {
                     bool bitValue = (raw & (0x80 >> bit)) != 0;
                     rowSpan[x] = bitValue ? black : white;
                     x++;
-                    if (x == width)
-                    {
-                        startBit = (bit + 1) & 7; // Round off to below 8.
-                        if (startBit != 0)
-                        {
-                            stream.Seek(-1, System.IO.SeekOrigin.Current);
-                        }
-
-                        break;
-                    }
                 }
             }
 

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 SkipWhitespaceAndComments(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 @@ public static void SkipWhitespaceAndComments(this BufferedReadStream stream)
                 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 @@ public static void SkipWhitespaceAndComments(this BufferedReadStream stream)
         }
         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>
-    public static int ReadDecimal(this BufferedReadStream stream)
+    /// <param name="stream">The buffered read stream.</param>
+    /// <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 ReadDecimal(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 @@ public static int ReadDecimal(this BufferedReadStream stream)
             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 @@ public ImageInfo Identify(BufferedReadStream stream, CancellationToken cancellat
     /// 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 @@ private void ProcessHeader(BufferedReadStream stream)
                 throw new InvalidImageContentException("Unknown of not implemented image type encountered.");
         }
 
-        stream.SkipWhitespaceAndComments();
-        int width = stream.ReadDecimal();
-        stream.SkipWhitespaceAndComments();
-        int height = stream.ReadDecimal();
-        stream.SkipWhitespaceAndComments();
+        if (!stream.SkipWhitespaceAndComments() ||
+            !stream.ReadDecimal(out int width) ||
+            !stream.SkipWhitespaceAndComments() ||
+            !stream.ReadDecimal(out int height) ||
+            !stream.SkipWhitespaceAndComments())
+        {
+            ThrowPrematureEof();
+        }
+
         if (this.colorType != PbmColorType.BlackAndWhite)
         {
-            this.maxPixelValue = stream.ReadDecimal();
+            if (!stream.ReadDecimal(out this.maxPixelValue))
+            {
+                ThrowPrematureEof();
+            }
+
             if (this.maxPixelValue > 255)
             {
                 this.componentType = PbmComponentType.Short;
@@ -174,6 +184,9 @@ private void ProcessHeader(BufferedReadStream stream)
         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)