Merge pull request #1055 from SixLabors/js/fix-1047

Fix reading of pngs with corrupt IEND chunks.

src/ImageSharp/Formats/Png/PngChunk.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 SixLabors.Memory;
@@ -10,12 +10,11 @@ namespace SixLabors.ImageSharp.Formats.Png
     /// </summary>
     internal readonly struct PngChunk
     {
-        public PngChunk(int length, PngChunkType type, IManagedByteBuffer data = null, uint crc = 0)
+        public PngChunk(int length, PngChunkType type, IManagedByteBuffer data = null)
         {
             this.Length = length;
             this.Type = type;
             this.Data = data;
-            this.Crc = crc;
         }
 
         /// <summary>
@@ -38,20 +37,12 @@ namespace SixLabors.ImageSharp.Formats.Png
         /// </summary>
         public IManagedByteBuffer Data { get; }
 
-        /// <summary>
-        /// Gets a CRC (Cyclic Redundancy Check) calculated on the preceding bytes in the chunk,
-        /// including the chunk type code and chunk data fields, but not including the length field.
-        /// The CRC is always present, even for chunks containing no data
-        /// </summary>
-        public uint Crc { get; }
-
         /// <summary>
         /// Gets a value indicating whether the given chunk is critical to decoding
         /// </summary>
         public bool IsCritical =>
             this.Type == PngChunkType.Header ||
             this.Type == PngChunkType.Palette ||
-            this.Type == PngChunkType.Data ||
-            this.Type == PngChunkType.End;
+            this.Type == PngChunkType.Data;
     }
-}
+}

src/ImageSharp/Formats/Png/PngDecoderCore.cs

@@ -221,7 +221,7 @@ namespace SixLabors.ImageSharp.Formats.Png
 
                 if (image is null)
                 {
-                    throw new ImageFormatException("PNG Image does not contain a data chunk");
+                    PngThrowHelper.ThrowNoData();
                 }
 
                 return image;
@@ -285,7 +285,7 @@ namespace SixLabors.ImageSharp.Formats.Png
 
             if (this.header.Width == 0 && this.header.Height == 0)
             {
-                throw new ImageFormatException("PNG Image does not contain a header chunk");
+                PngThrowHelper.ThrowNoHeader();
             }
 
             return new ImageInfo(new PixelTypeInfo(this.CalculateBitsPerPixel()), this.header.Width, this.header.Height, metadata);
@@ -407,7 +407,8 @@ namespace SixLabors.ImageSharp.Formats.Png
                 case PngColorType.RgbWithAlpha:
                     return this.header.BitDepth * 4;
                 default:
-                    throw new NotSupportedException("Unsupported PNG color type");
+                    PngThrowHelper.ThrowNotSupportedColor();
+                    return -1;
             }
         }
 
@@ -528,7 +529,8 @@ namespace SixLabors.ImageSharp.Formats.Png
                         break;
 
                     default:
-                        throw new ImageFormatException("Unknown filter type.");
+                        PngThrowHelper.ThrowUnknownFilter();
+                        break;
                 }
 
                 this.ProcessDefilteredScanline(scanlineSpan, image, pngMetadata);
@@ -601,7 +603,8 @@ namespace SixLabors.ImageSharp.Formats.Png
                             break;
 
                         default:
-                            throw new ImageFormatException("Unknown filter type.");
+                            PngThrowHelper.ThrowUnknownFilter();
+                            break;
                     }
 
                     Span<TPixel> rowSpan = image.GetPixelRowSpan(this.currentRow);
@@ -1119,13 +1122,9 @@ namespace SixLabors.ImageSharp.Formats.Png
             chunk = new PngChunk(
                 length: length,
                 type: type,
-                data: this.ReadChunkData(length),
-                crc: this.ReadChunkCrc());
+                data: this.ReadChunkData(length));
 
-            if (chunk.IsCritical)
-            {
-                this.ValidateChunk(chunk);
-            }
+            this.ValidateChunk(chunk);
 
             return true;
         }
@@ -1136,6 +1135,11 @@ namespace SixLabors.ImageSharp.Formats.Png
         /// <param name="chunk">The <see cref="PngChunk"/>.</param>
         private void ValidateChunk(in PngChunk chunk)
         {
+            if (!chunk.IsCritical)
+            {
+                return;
+            }
+
             Span<byte> chunkType = stackalloc byte[4];
 
             BinaryPrimitives.WriteUInt32BigEndian(chunkType, (uint)chunk.Type);
@@ -1144,31 +1148,34 @@ namespace SixLabors.ImageSharp.Formats.Png
             this.crc.Update(chunkType);
             this.crc.Update(chunk.Data.GetSpan());
 
-            if (this.crc.Value != chunk.Crc)
+            uint crc = this.ReadChunkCrc();
+            if (this.crc.Value != crc)
             {
                 string chunkTypeName = Encoding.ASCII.GetString(chunkType);
-
-                throw new ImageFormatException($"CRC Error. PNG {chunkTypeName} chunk is corrupt!");
+                PngThrowHelper.ThrowInvalidChunkCrc(chunkTypeName);
             }
         }
 
         /// <summary>
         /// Reads the cycle redundancy chunk from the data.
         /// </summary>
-        /// <exception cref="ImageFormatException">
-        /// Thrown if the input stream is not valid or corrupt.
-        /// </exception>
+        [MethodImpl(InliningOptions.ShortMethod)]
         private uint ReadChunkCrc()
         {
-            return this.currentStream.Read(this.buffer, 0, 4) == 4
-                ? BinaryPrimitives.ReadUInt32BigEndian(this.buffer)
-                : throw new ImageFormatException("Image stream is not valid!");
+            uint crc = 0;
+            if (this.currentStream.Read(this.buffer, 0, 4) == 4)
+            {
+                crc = BinaryPrimitives.ReadUInt32BigEndian(this.buffer);
+            }
+
+            return crc;
         }
 
         /// <summary>
         /// Skips the chunk data and the cycle redundancy chunk read from the data.
         /// </summary>
         /// <param name="chunk">The image format chunk.</param>
+        [MethodImpl(InliningOptions.ShortMethod)]
         private void SkipChunkDataAndCrc(in PngChunk chunk)
         {
             this.currentStream.Skip(chunk.Length);
@@ -1179,6 +1186,7 @@ namespace SixLabors.ImageSharp.Formats.Png
         /// Reads the chunk data from the stream.
         /// </summary>
         /// <param name="length">The length of the chunk data to read.</param>
+        [MethodImpl(InliningOptions.ShortMethod)]
         private IManagedByteBuffer ReadChunkData(int length)
         {
             // We rent the buffer here to return it afterwards in Decode()
@@ -1195,11 +1203,20 @@ namespace SixLabors.ImageSharp.Formats.Png
         /// <exception cref="ImageFormatException">
         /// Thrown if the input stream is not valid.
         /// </exception>
+        [MethodImpl(InliningOptions.ShortMethod)]
         private PngChunkType ReadChunkType()
         {
-            return this.currentStream.Read(this.buffer, 0, 4) == 4
-                ? (PngChunkType)BinaryPrimitives.ReadUInt32BigEndian(this.buffer)
-                : throw new ImageFormatException("Invalid PNG data.");
+            if (this.currentStream.Read(this.buffer, 0, 4) == 4)
+            {
+                return (PngChunkType)BinaryPrimitives.ReadUInt32BigEndian(this.buffer);
+            }
+            else
+            {
+                PngThrowHelper.ThrowInvalidChunkType();
+
+                // The IDE cannot detect the throw here.
+                return default;
+            }
         }
 
         /// <summary>
@@ -1208,6 +1225,7 @@ namespace SixLabors.ImageSharp.Formats.Png
         /// <returns>
         /// Whether the length was read.
         /// </returns>
+        [MethodImpl(InliningOptions.ShortMethod)]
         private bool TryReadChunkLength(out int result)
         {
             if (this.currentStream.Read(this.buffer, 0, 4) == 4)

src/ImageSharp/Formats/Png/PngThrowHelper.cs

@@ -0,0 +1,33 @@
+// Copyright (c) Six Labors and contributors.
+// Licensed under the Apache License, Version 2.0.
+
+using System;
+using System.Diagnostics.CodeAnalysis;
+using System.Runtime.CompilerServices;
+
+namespace SixLabors.ImageSharp.Formats.Png
+{
+    /// <summary>
+    /// Cold path optimizations for throwing png format based exceptions.
+    /// </summary>
+    internal static class PngThrowHelper
+    {
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowNoHeader() => throw new ImageFormatException("PNG Image does not contain a header chunk");
+
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowNoData() => throw new ImageFormatException("PNG Image does not contain a data chunk");
+
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowInvalidChunkType() => throw new ImageFormatException("Invalid PNG data.");
+
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowInvalidChunkCrc(string chunkTypeName) => throw new ImageFormatException($"CRC Error. PNG {chunkTypeName} chunk is corrupt!");
+
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowNotSupportedColor() => new NotSupportedException("Unsupported PNG color type");
+
+        [MethodImpl(InliningOptions.ColdPath)]
+        public static void ThrowUnknownFilter() => throw new ImageFormatException("Unknown filter type.");
+    }
+}

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.Chunks.cs

@@ -54,7 +54,6 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png
         [InlineData((uint)PngChunkType.Header)] // IHDR
         [InlineData((uint)PngChunkType.Palette)] // PLTE
         // [InlineData(PngChunkTypes.Data)] //TODO: Figure out how to test this
-        [InlineData((uint)PngChunkType.End)] // IEND
         public void Decode_IncorrectCRCForCriticalChunk_ExceptionIsThrown(uint chunkType)
         {
             string chunkName = GetChunkTypeName(chunkType);
@@ -74,26 +73,6 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png
             }
         }
 
-        [Theory]
-        [InlineData((uint)PngChunkType.Gamma)] // gAMA
-        [InlineData((uint)PngChunkType.Transparency)] // tRNS
-        [InlineData((uint)PngChunkType.Physical)] // pHYs: It's ok to test physical as we don't throw for duplicate chunks.
-        //[InlineData(PngChunkTypes.Text)] //TODO: Figure out how to test this
-        public void Decode_IncorrectCRCForNonCriticalChunk_ExceptionIsThrown(uint chunkType)
-        {
-            string chunkName = GetChunkTypeName(chunkType);
-
-            using (var memStream = new MemoryStream())
-            {
-                WriteHeaderChunk(memStream);
-                WriteChunk(memStream, chunkName);
-                WriteDataChunk(memStream);
-
-                var decoder = new PngDecoder();
-                decoder.Decode<Rgb24>(null, memStream);
-            }
-        }
-
         private static string GetChunkTypeName(uint value)
         {
             var data = new byte[4];

tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs

@@ -4,11 +4,11 @@
 // ReSharper disable InconsistentNaming
 
 using System.IO;
-
+using SixLabors.ImageSharp.Formats;
 using SixLabors.ImageSharp.Formats.Png;
 using SixLabors.ImageSharp.PixelFormats;
 using SixLabors.ImageSharp.Tests.TestUtilities.ImageComparison;
-
+using SixLabors.ImageSharp.Tests.TestUtilities.ReferenceCodecs;
 using Xunit;
 
 namespace SixLabors.ImageSharp.Tests.Formats.Png
@@ -42,6 +42,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png
             TestImages.Png.Bad.ZlibOverflow,
             TestImages.Png.Bad.ZlibOverflow2,
             TestImages.Png.Bad.ZlibZtxtBadHeader,
+            TestImages.Png.Bad.Issue1047_BadEndChunk
         };
 
         public static readonly string[] TestImages48Bpp =
@@ -90,7 +91,19 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png
             using (Image<TPixel> image = provider.GetImage(new PngDecoder()))
             {
                 image.DebugSave(provider);
-                image.CompareToOriginal(provider, ImageComparer.Exact);
+
+                // We don't have another x-plat reference decoder that can be compared for this image.
+                if (provider.Utility.SourceFileOrDescription == TestImages.Png.Bad.Issue1047_BadEndChunk)
+                {
+                    if (TestEnvironment.IsWindows)
+                    {
+                        image.CompareToOriginal(provider, ImageComparer.Exact, (IImageDecoder)SystemDrawingReferenceDecoder.Instance);
+                    }
+                }
+                else
+                {
+                    image.CompareToOriginal(provider, ImageComparer.Exact);
+                }
             }
         }
 

tests/ImageSharp.Tests/TestImages.cs

@@ -99,6 +99,7 @@ namespace SixLabors.ImageSharp.Tests
                 public const string ZlibOverflow = "Png/zlib-overflow.png";
                 public const string ZlibOverflow2 = "Png/zlib-overflow2.png";
                 public const string ZlibZtxtBadHeader = "Png/zlib-ztxt-bad-header.png";
+                public const string Issue1047_BadEndChunk = "Png/issues/Issue_1047.png";
             }
 
             public static readonly string[] All =