Merge pull request #924 from SixLabors/js/zigzag-overflow

Prevent zigzag overflow. Fix #922

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;
         }
     }
-}
+}

src/ImageSharp/Formats/Jpeg/Components/ZigZag.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;
@@ -16,10 +16,22 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components
     [StructLayout(LayoutKind.Sequential)]
     internal unsafe struct ZigZag
     {
+        /// <summary>
+        /// When reading corrupted data, the Huffman decoders could attempt
+        /// to reference an entry beyond the end of this array (if the decoded
+        /// zero run length reaches past the end of the block).  To prevent
+        /// wild stores without adding an inner-loop test, we put some extra
+        /// "63"s after the real entries.  This will cause the extra coefficient
+        /// to be stored in location 63 of the block, not somewhere random.
+        /// The worst case would be a run-length of 15, which means we need 16
+        /// fake entries.
+        /// </summary>
+        private const int Size = 64 + 16;
+
         /// <summary>
         /// Copy of <see cref="Unzig"/> in a value type
         /// </summary>
-        public fixed byte Data[64];
+        public fixed byte Data[Size];
 
         /// <summary>
         /// Unzig maps from the zigzag ordering to the natural ordering. For example,
@@ -27,22 +39,18 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components
         /// value is 16, which means first column (16%8 == 0) and third row (16/8 == 2).
         /// </summary>
         private static readonly byte[] Unzig =
+        new byte[Size]
         {
-            0,
+            0,  1,  8, 16,  9,  2,  3, 10,
-            1, 8,
+            17, 24, 32, 25, 18, 11,  4,  5,
-            16, 9, 2,
+            12, 19, 26, 33, 40, 48, 41, 34,
-            3, 10, 17, 24,
+            27, 20, 13,  6,  7, 14, 21, 28,
-            32, 25, 18, 11, 4,
+            35, 42, 49, 56, 57, 50, 43, 36,
-            5, 12, 19, 26, 33, 40,
+            29, 22, 15, 23, 30, 37, 44, 51,
-            48, 41, 34, 27, 20, 13, 6,
+            58, 59, 52, 45, 38, 31, 39, 46,
-            7, 14, 21, 28, 35, 42, 49, 56,
+            53, 60, 61, 54, 47, 55, 62, 63,
-            57, 50, 43, 36, 29, 22, 15,
+            63, 63, 63, 63, 63, 63, 63, 63, // Extra entries for safety in decoder
-            23, 30, 37, 44, 51, 58,
+            63, 63, 63, 63, 63, 63, 63, 63
-            59, 52, 45, 38, 31,
-            39, 46, 53, 60,
-            61, 54, 47,
-            55, 62,
-            63
         };
 
         /// <summary>
@@ -68,7 +76,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components
         {
             ZigZag result = default;
             byte* unzigPtr = result.Data;
-            Marshal.Copy(Unzig, 0, (IntPtr)unzigPtr, 64);
+            Marshal.Copy(Unzig, 0, (IntPtr)unzigPtr, Size);
             return result;
         }
 
@@ -79,7 +87,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components
         {
             Block8x8F result = default;
 
-            for (int i = 0; i < 64; i++)
+            for (int i = 0; i < Block8x8F.Size; i++)
             {
                 result[Unzig[i]] = qt[i];
             }
@@ -87,4 +95,4 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components
             return result;
         }
     }
-}
+}

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

@@ -18,6 +18,11 @@ 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,
                 TestImages.Jpeg.Baseline.Bad.BadEOF,
                 TestImages.Jpeg.Baseline.MultiScanBaselineCMYK,
@@ -106,4 +111,4 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg
                 [TestImages.Jpeg.Progressive.Bad.ExifUndefType] = 0.011f / 100,
             };
     }
-}
+}

tests/ImageSharp.Tests/Formats/Jpg/ZigZagTests.cs

@@ -0,0 +1,44 @@
+// Copyright (c) Six Labors and contributors.
+// Licensed under the Apache License, Version 2.0.
+
+using SixLabors.ImageSharp.Formats.Jpeg.Components;
+using Xunit;
+
+namespace SixLabors.ImageSharp.Tests.Formats.Jpg
+{
+    public class ZigZagTests
+    {
+        [Fact]
+        public void ZigZagCanHandleAllPossibleCoefficients()
+        {
+            // Mimic the behaviour of the huffman scan decoder using all possible byte values
+            short[] block = new short[64];
+            var zigzag = ZigZag.CreateUnzigTable();
+
+            for (int h = 0; h < 255; h++)
+            {
+                for (int i = 1; i < 64; i++)
+                {
+                    int s = h;
+                    int r = s >> 4;
+                    s &= 15;
+
+                    if (s != 0)
+                    {
+                        i += r;
+                        block[zigzag[i++]] = (short)s;
+                    }
+                    else
+                    {
+                        if (r == 0)
+                        {
+                            break;
+                        }
+
+                        i += 16;
+                    }
+                }
+            }
+        }
+    }
+}

tests/ImageSharp.Tests/TestImages.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.Linq;
@@ -202,6 +202,7 @@ namespace SixLabors.ImageSharp.Tests
                     public const string ArgumentException826C = "Jpg/issues/fuzz/Issue826-ArgumentException-C.jpg";
                     public const string AccessViolationException827 = "Jpg/issues/fuzz/Issue827-AccessViolationException.jpg";
                     public const string ExecutionEngineException839 = "Jpg/issues/fuzz/Issue839-ExecutionEngineException.jpg";
+                    public const string AccessViolationException922 = "Jpg/issues/fuzz/Issue922-AccessViolationException.jpg";
                 }
             }