[release/2.1] PBM decoder robustness improvements and BufferedReadStream observability (#2555)

* PBM decoder robustness improvements and BufferedReadStream observability

Backport of #2551 & #2552

* Remove DoesNotReturn attribute

---------

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

src/ImageSharp/Formats/ImageDecoderUtilities.cs

@@ -46,7 +46,8 @@ namespace SixLabors.ImageSharp.Formats
             CancellationToken cancellationToken)
             where TPixel : unmanaged, IPixel<TPixel>
         {
-            using var bufferedReadStream = new BufferedReadStream(configuration, stream);
+            // 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);
 
             try
             {
@@ -56,6 +57,13 @@ namespace SixLabors.ImageSharp.Formats
             {
                 throw largeImageExceptionFactory(ex, decoder.Dimensions);
             }
+            finally
+            {
+                if (bufferedReadStream != stream)
+                {
+                    bufferedReadStream.Dispose();
+                }
+            }
         }
 
         private static InvalidImageContentException DefaultLargeImageExceptionFactory(

src/ImageSharp/Formats/Pbm/BinaryDecoder.cs

@@ -72,7 +72,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
 
             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,
@@ -94,7 +98,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
 
             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,
@@ -116,7 +124,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
 
             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,
@@ -138,7 +150,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
 
             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,
@@ -153,7 +169,6 @@ namespace SixLabors.ImageSharp.Formats.Pbm
         {
             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();
@@ -163,23 +178,17 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                 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

@@ -12,14 +12,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)
@@ -28,8 +34,12 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     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;
@@ -39,18 +49,31 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             }
             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;
@@ -59,7 +82,7 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                 value = (value * 10) + current;
             }
 
-            return value;
+            return true;
         }
     }
 }

src/ImageSharp/Formats/Pbm/PbmDecoderCore.cs

@@ -90,6 +90,7 @@ namespace SixLabors.ImageSharp.Formats.Pbm
         /// 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];
@@ -139,14 +140,22 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     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;
@@ -169,6 +178,8 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             meta.Encoding = this.Encoding;
             meta.ColorType = this.ColorType;
             meta.ComponentType = this.ComponentType;
+
+            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

@@ -66,13 +66,18 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             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.SkipWhitespaceAndComments();
-                    rowSpan[x] = new L8(value);
+                    stream.ReadDecimal(out int value);
+                    rowSpan[x] = new L8((byte)value);
+                    eofReached = !stream.SkipWhitespaceAndComments();
+                    if (eofReached)
+                    {
+                        break;
+                    }
                 }
 
                 Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -80,6 +85,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     configuration,
                     rowSpan,
                     pixelSpan);
+
+                if (eofReached)
+                {
+                    return;
+                }
             }
         }
 
@@ -92,13 +102,18 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             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.SkipWhitespaceAndComments();
-                    rowSpan[x] = new L16(value);
+                    stream.ReadDecimal(out int value);
+                    rowSpan[x] = new L16((ushort)value);
+                    eofReached = !stream.SkipWhitespaceAndComments();
+                    if (eofReached)
+                    {
+                        break;
+                    }
                 }
 
                 Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -106,6 +121,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     configuration,
                     rowSpan,
                     pixelSpan);
+
+                if (eofReached)
+                {
+                    return;
+                }
             }
         }
 
@@ -118,17 +138,29 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             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();
-                    stream.SkipWhitespaceAndComments();
-                    byte green = (byte)stream.ReadDecimal();
-                    stream.SkipWhitespaceAndComments();
-                    byte blue = (byte)stream.ReadDecimal();
-                    stream.SkipWhitespaceAndComments();
-                    rowSpan[x] = new Rgb24(red, green, blue);
+                    if (!stream.ReadDecimal(out int red) ||
+                        !stream.SkipWhitespaceAndComments() ||
+                        !stream.ReadDecimal(out int green) ||
+                        !stream.SkipWhitespaceAndComments())
+                    {
+                        // Reached EOF before reading a full RGB value
+                        eofReached = true;
+                        break;
+                    }
+
+                    stream.ReadDecimal(out int blue);
+
+                    rowSpan[x] = new Rgb24((byte)red, (byte)green, (byte)blue);
+                    eofReached = !stream.SkipWhitespaceAndComments();
+                    if (eofReached)
+                    {
+                        break;
+                    }
                 }
 
                 Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -136,6 +168,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     configuration,
                     rowSpan,
                     pixelSpan);
+
+                if (eofReached)
+                {
+                    return;
+                }
             }
         }
 
@@ -148,17 +185,29 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             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();
-                    stream.SkipWhitespaceAndComments();
-                    ushort green = (ushort)stream.ReadDecimal();
-                    stream.SkipWhitespaceAndComments();
-                    ushort blue = (ushort)stream.ReadDecimal();
-                    stream.SkipWhitespaceAndComments();
-                    rowSpan[x] = new Rgb48(red, green, blue);
+                    if (!stream.ReadDecimal(out int red) ||
+                        !stream.SkipWhitespaceAndComments() ||
+                        !stream.ReadDecimal(out int green) ||
+                        !stream.SkipWhitespaceAndComments())
+                    {
+                        // Reached EOF before reading a full RGB value
+                        eofReached = true;
+                        break;
+                    }
+
+                    stream.ReadDecimal(out int blue);
+
+                    rowSpan[x] = new Rgb48((ushort)red, (ushort)green, (ushort)blue);
+                    eofReached = !stream.SkipWhitespaceAndComments();
+                    if (eofReached)
+                    {
+                        break;
+                    }
                 }
 
                 Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -166,6 +215,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     configuration,
                     rowSpan,
                     pixelSpan);
+
+                if (eofReached)
+                {
+                    return;
+                }
             }
         }
 
@@ -178,13 +232,19 @@ namespace SixLabors.ImageSharp.Formats.Pbm
             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.SkipWhitespaceAndComments();
+                    stream.ReadDecimal(out int value);
+
                     rowSpan[x] = value == 0 ? White : Black;
+                    eofReached = !stream.SkipWhitespaceAndComments();
+                    if (eofReached)
+                    {
+                        break;
+                    }
                 }
 
                 Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(y);
@@ -192,6 +252,11 @@ namespace SixLabors.ImageSharp.Formats.Pbm
                     configuration,
                     rowSpan,
                     pixelSpan);
+
+                if (eofReached)
+                {
+                    return;
+                }
             }
         }
     }

src/ImageSharp/IO/BufferedReadStream.cs

@@ -65,6 +65,11 @@ namespace SixLabors.ImageSharp.IO
             this.readBufferIndex = this.BufferSize;
         }
 
+        /// <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>
@@ -138,6 +143,7 @@ namespace SixLabors.ImageSharp.IO
         {
             if (this.readerPosition >= this.Length)
             {
+                this.EofHitCount++;
                 return -1;
             }
 
@@ -303,7 +309,7 @@ namespace SixLabors.ImageSharp.IO
 
             this.readerPosition += n;
             this.readBufferIndex += n;
-
+            this.CheckEof(n);
             return n;
         }
 
@@ -361,6 +367,7 @@ namespace SixLabors.ImageSharp.IO
 
             this.Position += n;
 
+            this.CheckEof(n);
             return n;
         }
 
@@ -427,5 +434,14 @@ namespace SixLabors.ImageSharp.IO
                 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

@@ -2,8 +2,10 @@
 // Licensed under the Apache License, Version 2.0.
 
 using System.IO;
+using System.Text;
 using SixLabors.ImageSharp.Formats.Pbm;
 using SixLabors.ImageSharp.PixelFormats;
+using SixLabors.ImageSharp.Tests.TestUtilities;
 using Xunit;
 using static SixLabors.ImageSharp.Tests.TestImages.Pbm;
 
@@ -97,5 +99,25 @@ namespace SixLabors.ImageSharp.Tests.Formats.Pbm
             bool isGrayscale = extension is "pgm" or "pbm";
             image.CompareToReferenceOutput(provider, grayscale: isGrayscale);
         }
+
+
+        [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

@@ -86,13 +86,10 @@ namespace SixLabors.ImageSharp.Tests.Formats.Pbm
         }
 
         [Fact]
-        public void Identify_HandlesCraftedDenialOfServiceString()
+        public void Identify_EofInHeader_ThrowsInvalidImageContentException()
         {
             byte[] bytes = Convert.FromBase64String("UDEjWAAACQAAAAA=");
-            IImageInfo info = Image.Identify(bytes);
-            Assert.Equal(default, info.Size());
-            IImageFormat format = Configuration.Default.ImageFormatsManager.FindFormatByFileExtension("pbm");
-            Assert.Equal("PBM", format.Name);
+            Assert.Throws<InvalidImageContentException>(() => Image.Identify(bytes));
         }
     }
 }

tests/ImageSharp.Tests/TestImages.cs

@@ -978,6 +978,7 @@ namespace SixLabors.ImageSharp.Tests
             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,39 @@
+// Copyright (c) Six Labors.
+// Licensed under the Apache License, Version 2.0.
+
+using System;
+using System.IO;
+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