Merge pull request #2147 from br3aker/dp/jpeg-decoder-invalid-marker-fix

Skip invalid markers during jpeg decoding

Author: brianpopow

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

@@ -155,12 +155,14 @@ private ulong GetBytes()
                         c = this.ReadStream();
                     }
 
+                    // Found a marker
                     // We accept multiple FF bytes followed by a 0 as meaning a single FF data byte.
-                    // This data pattern is not valid according to the standard.
+                    // even though it's considered 'invalid' according to the specs.
                     if (c != 0)
                     {
-                        this.Marker = (byte)c;
+                        // It's a trick so we won't read past actual marker
                         this.badData = true;
+                        this.Marker = (byte)c;
                         this.MarkerPosition = this.stream.Position - 2;
                     }
                 }
@@ -199,7 +201,6 @@ public bool FindNextMarker()
                     if (b != 0)
                     {
                         this.Marker = (byte)b;
-                        this.badData = true;
                         this.MarkerPosition = this.stream.Position - 2;
                         return true;
                     }

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

@@ -164,38 +164,38 @@ public JpegDecoderCore(Configuration configuration, IJpegDecoderOptions options)
         /// <summary>
         /// Finds the next file marker within the byte stream.
         /// </summary>
-        /// <param name="marker">The buffer to read file markers to.</param>
         /// <param name="stream">The input stream.</param>
-        /// <returns>The <see cref="JpegFileMarker"/></returns>
-        public static JpegFileMarker FindNextFileMarker(byte[] marker, BufferedReadStream stream)
+        /// <returns>The <see cref="JpegFileMarker"/>.</returns>
+        public static JpegFileMarker FindNextFileMarker(BufferedReadStream stream)
         {
-            int value = stream.Read(marker, 0, 2);
-
-            if (value == 0)
+            while (true)
             {
-                return new JpegFileMarker(JpegConstants.Markers.EOI, stream.Length - 2);
-            }
+                int b = stream.ReadByte();
+                if (b == -1)
+                {
+                    return new JpegFileMarker(JpegConstants.Markers.EOI, stream.Length - 2);
+                }
 
-            if (marker[0] == JpegConstants.Markers.XFF)
-            {
-                // According to Section B.1.1.2:
-                // "Any marker may optionally be preceded by any number of fill bytes, which are bytes assigned code 0xFF."
-                int m = marker[1];
-                while (m == JpegConstants.Markers.XFF)
+                // Found a marker.
+                if (b == JpegConstants.Markers.XFF)
                 {
-                    int suffix = stream.ReadByte();
-                    if (suffix == -1)
+                    while (b == JpegConstants.Markers.XFF)
                     {
-                        return new JpegFileMarker(JpegConstants.Markers.EOI, stream.Length - 2);
+                        // Loop here to discard any padding FF bytes on terminating marker.
+                        b = stream.ReadByte();
+                        if (b == -1)
+                        {
+                            return new JpegFileMarker(JpegConstants.Markers.EOI, stream.Length - 2);
+                        }
                     }
 
-                    m = suffix;
+                    // Found a valid marker. Exit loop
+                    if (b is not 0 and (< JpegConstants.Markers.RST0 or > JpegConstants.Markers.RST7))
+                    {
+                        return new JpegFileMarker((byte)(uint)b, stream.Position - 2);
+                    }
                 }
-
-                return new JpegFileMarker((byte)m, stream.Position - 2);
             }
-
-            return new JpegFileMarker(marker[1], stream.Position - 2, true);
         }
 
         /// <inheritdoc/>
@@ -331,15 +331,12 @@ internal void ParseStream(BufferedReadStream stream, SpectralConverter spectralC
                 JpegThrowHelper.ThrowInvalidImageContentException("Missing SOI marker.");
             }
 
-            stream.Read(this.markerBuffer, 0, 2);
-            byte marker = this.markerBuffer[1];
-            fileMarker = new JpegFileMarker(marker, (int)stream.Position - 2);
+            fileMarker = FindNextFileMarker(stream);
             this.QuantizationTables ??= new Block8x8F[4];
 
             // Break only when we discover a valid EOI marker.
             // https://github.com/SixLabors/ImageSharp/issues/695
-            while (fileMarker.Marker != JpegConstants.Markers.EOI
-                || (fileMarker.Marker == JpegConstants.Markers.EOI && fileMarker.Invalid))
+            while (fileMarker.Marker != JpegConstants.Markers.EOI)
             {
                 cancellationToken.ThrowIfCancellationRequested();
 
@@ -491,7 +488,7 @@ internal void ParseStream(BufferedReadStream stream, SpectralConverter spectralC
                 }
 
                 // Read on.
-                fileMarker = FindNextFileMarker(this.markerBuffer, stream);
+                fileMarker = FindNextFileMarker(stream);
             }
         }
 

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

@@ -220,7 +220,7 @@ public void Issue2057_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
 
         // https://github.com/SixLabors/ImageSharp/issues/2133
         [Theory]
-        [WithFile(TestImages.Jpeg.Issues.Issue2133DeduceColorSpace, PixelTypes.Rgba32)]
+        [WithFile(TestImages.Jpeg.Issues.Issue2133_DeduceColorSpace, PixelTypes.Rgba32)]
         public void Issue2133_DeduceColorSpace<TPixel>(TestImageProvider<TPixel> provider)
             where TPixel : unmanaged, IPixel<TPixel>
         {
@@ -231,6 +231,19 @@ public void Issue2133_DeduceColorSpace<TPixel>(TestImageProvider<TPixel> provide
             }
         }
 
+        // https://github.com/SixLabors/ImageSharp/issues/2133
+        [Theory]
+        [WithFile(TestImages.Jpeg.Issues.Issue2136_ScanMarkerExtraneousBytes, PixelTypes.Rgba32)]
+        public void Issue2136_DecodeWorks<TPixel>(TestImageProvider<TPixel> provider)
+            where TPixel : unmanaged, IPixel<TPixel>
+        {
+            using (Image<TPixel> image = provider.GetImage(JpegDecoder))
+            {
+                image.DebugSave(provider);
+                image.CompareToOriginal(provider);
+            }
+        }
+
         // DEBUG ONLY!
         // The PDF.js output should be saved by "tests\ImageSharp.Tests\Formats\Jpg\pdfjs\jpeg-converter.htm"
         // into "\tests\Images\ActualOutput\JpegDecoderTests\"

tests/ImageSharp.Tests/TestImages.cs

@@ -272,7 +272,8 @@ public static class Issues
                 public const string Issue2057App1Parsing = "Jpg/issues/Issue2057-App1Parsing.jpg";
                 public const string ExifNullArrayTag = "Jpg/issues/issue-2056-exif-null-array.jpg";
                 public const string ValidExifArgumentNullExceptionOnEncode = "Jpg/issues/Issue2087-exif-null-reference-on-encode.jpg";
-                public const string Issue2133DeduceColorSpace = "Jpg/issues/Issue2133.jpg";
+                public const string Issue2133_DeduceColorSpace = "Jpg/issues/Issue2133.jpg";
+                public const string Issue2136_ScanMarkerExtraneousBytes = "Jpg/issues/Issue2136-scan-segment-extraneous-bytes.jpg";
 
                 public static class Fuzz
                 {