Improve robustness of huffman decoder.

Directory.Build.props

@@ -27,9 +27,12 @@
     <PackageRequireLicenseAcceptance>true</PackageRequireLicenseAcceptance>
     <SignAssembly>false</SignAssembly>
     <SuppressNETCoreSdkPreviewMessage>true</SuppressNETCoreSdkPreviewMessage>
-    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
   </PropertyGroup>
 
+  <PropertyGroup Condition="'$(Configuration)' == 'Release'">
+    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+  </PropertyGroup>
+  
   <!--TODO: Check what this is testing for and why does it fail?-->
   <PropertyGroup Condition="'$(Configuration)' == 'Debug'">
     <CheckForOverflowUnderflow>false</CheckForOverflowUnderflow>

src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Six Labors and contributors.
+// Copyright (c) Six Labors and contributors.
 // Licensed under the Apache License, Version 2.0.
 
 using System.Runtime.CompilerServices;
@@ -19,8 +19,8 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
         // The number of valid bits left to read in the buffer.
         private int remain;
 
-        // Whether there is more data to pull from the stream for the current mcu.
+        // Whether there is no more good data to pull from the stream for the current mcu.
-        private bool noMore;
+        private bool badData;
 
         public HuffmanScanBuffer(DoubleBufferedStreamReader stream)
         {
@@ -30,29 +30,29 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
             this.Marker = JpegConstants.Markers.XFF;
             this.MarkerPosition = 0;
             this.BadMarker = false;
-            this.noMore = false;
+            this.badData = false;
-            this.Eof = false;
+            this.NoData = false;
         }
 
         /// <summary>
-        /// Gets or sets the current, if any, marker in the input stream.
+        /// Gets the current, if any, marker in the input stream.
         /// </summary>
-        public byte Marker { get; set; }
+        public byte Marker { get; private set; }
 
         /// <summary>
-        /// Gets or sets the opening position of an identified marker.
+        /// Gets the opening position of an identified marker.
         /// </summary>
-        public long MarkerPosition { get; set; }
+        public long MarkerPosition { get; private set; }
 
         /// <summary>
-        /// Gets or sets a value indicating whether we have a bad marker, I.E. One that is not between RST0 and RST7
+        /// Gets a value indicating whether a bad marker has been detected, I.E. One that is not between RST0 and RST7
         /// </summary>
-        public bool BadMarker { get; set; }
+        public bool BadMarker { get; private set; }
 
         /// <summary>
-        /// Gets or sets a value indicating whether we have prematurely reached the end of the file.
+        /// Gets a value indicating whether to continue reading the input stream.
         /// </summary>
-        public bool Eof { get; set; }
+        public bool NoData { get; private set; }
 
         [MethodImpl(InliningOptions.ShortMethod)]
         public void CheckBits()
@@ -71,8 +71,8 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
             this.Marker = JpegConstants.Markers.XFF;
             this.MarkerPosition = 0;
             this.BadMarker = false;
-            this.noMore = false;
+            this.badData = false;
-            this.Eof = false;
+            this.NoData = false;
         }
 
         [MethodImpl(InliningOptions.ShortMethod)]
@@ -141,39 +141,28 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
             ulong temp = 0;
             for (int i = 0; i < 6; i++)
             {
-                int b = this.noMore ? 0 : this.stream.ReadByte();
+                int b = this.ReadStream();
-
-                if (b == -1)
-                {
-                    // We've encountered the end of the file stream which means there's no EOI marker in the image
-                    // or the SOS marker has the wrong dimensions set.
-                    this.Eof = true;
-                    b = 0;
-                }
 
                 // Found a marker.
                 if (b == JpegConstants.Markers.XFF)
                 {
-                    this.MarkerPosition = this.stream.Position - 1;
+                    int c = this.ReadStream();
-                    int c = this.stream.ReadByte();
                     while (c == JpegConstants.Markers.XFF)
                     {
-                        c = this.stream.ReadByte();
+                        // Loop here to discard any padding FF's on terminating marker,
-
+                        // so that we can save a valid marker value.
-                        if (c == -1)
+                        c = this.ReadStream();
-                        {
-                            this.Eof = true;
-                            c = 0;
-                            break;
-                        }
                     }
 
+                    // We accept multiple FF's followed by a 0 as meaning a single FF data byte.
+                    // This data pattern is not valid according to the standard.
                     if (c != 0)
                     {
                         this.Marker = (byte)c;
-                        this.noMore = true;
+                        this.badData = true;
                         if (!this.HasRestart())
                         {
+                            this.MarkerPosition = this.stream.Position - 2;
                             this.BadMarker = true;
                         }
                     }
@@ -184,5 +173,21 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
 
             return temp;
         }
+
+        [MethodImpl(InliningOptions.ShortMethod)]
+        private int ReadStream()
+        {
+            int value = this.badData ? 0 : this.stream.ReadByte();
+            if (value == -1)
+            {
+                // We've encountered the end of the file stream which means there's no EOI marker
+                // in the image or the SOS marker has the wrong dimensions set.
+                this.badData = true;
+                this.NoData = true;
+                value = 0;
+            }
+
+            return value;
+        }
     }
-}
+}

src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Six Labors and contributors.
+// Copyright (c) Six Labors and contributors.
 // Licensed under the Apache License, Version 2.0.
 
 using System;
@@ -130,6 +130,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
             int mcu = 0;
             int mcusPerColumn = this.frame.McusPerColumn;
             int mcusPerLine = this.frame.McusPerLine;
+            ref HuffmanScanBuffer buffer = ref this.scanBuffer;
 
             // Pre-derive the huffman table to avoid in-loop checks.
             for (int i = 0; i < this.componentsLength; i++)
@@ -171,6 +172,11 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
 
                             for (int x = 0; x < h; x++)
                             {
+                                if (buffer.NoData)
+                                {
+                                    return;
+                                }
+
                                 int blockCol = (mcuCol * h) + x;
 
                                 this.DecodeBlockBaseline(
@@ -193,6 +199,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
         private unsafe void ParseBaselineDataNonInterleaved()
         {
             JpegComponent component = this.components[this.frame.ComponentOrder[0]];
+            ref HuffmanScanBuffer buffer = ref this.scanBuffer;
 
             int w = component.WidthInBlocks;
             int h = component.HeightInBlocks;
@@ -210,6 +217,11 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
 
                 for (int i = 0; i < w; i++)
                 {
+                    if (buffer.NoData)
+                    {
+                        return;
+                    }
+
                     this.DecodeBlockBaseline(
                         component,
                         ref Unsafe.Add(ref blockRef, i),
@@ -294,6 +306,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
             int mcu = 0;
             int mcusPerColumn = this.frame.McusPerColumn;
             int mcusPerLine = this.frame.McusPerLine;
+            ref HuffmanScanBuffer buffer = ref this.scanBuffer;
 
             // Pre-derive the huffman table to avoid in-loop checks.
             for (int k = 0; k < this.componentsLength; k++)
@@ -316,7 +329,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
                         int order = this.frame.ComponentOrder[k];
                         JpegComponent component = this.components[order];
                         ref HuffmanTable dcHuffmanTable = ref this.dcHuffmanTables[component.DCHuffmanTableId];
-                        ref HuffmanScanBuffer buffer = ref this.scanBuffer;
 
                         int h = component.HorizontalSamplingFactor;
                         int v = component.VerticalSamplingFactor;
@@ -331,7 +343,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
 
                             for (int x = 0; x < h; x++)
                             {
-                                if (buffer.Eof)
+                                if (buffer.NoData)
                                 {
                                     return;
                                 }
@@ -375,7 +387,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
 
                     for (int i = 0; i < w; i++)
                     {
-                        if (buffer.Eof)
+                        if (buffer.NoData)
                         {
                             return;
                         }
@@ -404,7 +416,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
 
                     for (int i = 0; i < w; i++)
                     {
-                        if (buffer.Eof)
+                        if (buffer.NoData)
                         {
                             return;
                         }
@@ -691,4 +703,4 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
             return false;
         }
     }
-}
+}

tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Images.cs

@@ -17,6 +17,10 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg
 
                 // BUG: The following image has a high difference compared to the expected output:
                 // TestImages.Jpeg.Baseline.Jpeg420Small,
+
+                // BUG: While we can decode this image we do not return the same output as libjpeg
+                // based decoders and are inserting a number of lines equal to the corrupted lines
+                // below said lines.
                 // TestImages.Jpeg.Issues.Fuzz.AccessViolationException922,
 
                 TestImages.Jpeg.Baseline.Jpeg444,