Merge pull request #2589 from svenclaesson/better_handle_corrupt_png

Relaxed handle of corrupt png files

src/ImageSharp/Formats/Png/PngChunk.cs

@@ -41,9 +41,13 @@ internal readonly struct PngChunk
     /// <summary>
     /// Gets a value indicating whether the given chunk is critical to decoding
     /// </summary>
-    public bool IsCritical =>
-        this.Type is PngChunkType.Header or
-        PngChunkType.Palette or
-        PngChunkType.Data or
-        PngChunkType.FrameData;
+    /// <param name="handling">The chunk CRC handling behavior.</param>
+    public bool IsCritical(PngCrcChunkHandling handling)
+        => handling switch
+        {
+            PngCrcChunkHandling.IgnoreNone => true,
+            PngCrcChunkHandling.IgnoreNonCritical => this.Type is PngChunkType.Header or PngChunkType.Palette or PngChunkType.Data or PngChunkType.FrameData,
+            PngCrcChunkHandling.IgnoreData => this.Type is PngChunkType.Header or PngChunkType.Palette,
+            _ => false,
+        };
 }

src/ImageSharp/Formats/Png/PngCrcChunkHandling.cs

@@ -0,0 +1,30 @@
+// Copyright (c) Six Labors.
+// Licensed under the Six Labors Split License.
+
+namespace SixLabors.ImageSharp.Formats.Png;
+
+/// <summary>
+/// Specifies how to handle validation of any CRC (Cyclic Redundancy Check) data within the encoded PNG.
+/// </summary>
+public enum PngCrcChunkHandling
+{
+    /// <summary>
+    /// Do not ignore any CRC chunk errors.
+    /// </summary>
+    IgnoreNone,
+
+    /// <summary>
+    /// Ignore CRC errors in non critical chunks.
+    /// </summary>
+    IgnoreNonCritical,
+
+    /// <summary>
+    /// Ignore CRC errors in data chunks.
+    /// </summary>
+    IgnoreData,
+
+    /// <summary>
+    /// Ignore CRC errors in all chunks.
+    /// </summary>
+    IgnoreAll
+}

src/ImageSharp/Formats/Png/PngDecoder.cs

@@ -8,7 +8,7 @@ namespace SixLabors.ImageSharp.Formats.Png;
 /// <summary>
 /// Decoder for generating an image out of a png encoded stream.
 /// </summary>
-public sealed class PngDecoder : ImageDecoder
+public sealed class PngDecoder : SpecializedImageDecoder<PngDecoderOptions>
 {
     private PngDecoder()
     {
@@ -25,31 +25,31 @@ public sealed class PngDecoder : ImageDecoder
         Guard.NotNull(options, nameof(options));
         Guard.NotNull(stream, nameof(stream));
 
-        return new PngDecoderCore(options).Identify(options.Configuration, stream, cancellationToken);
+        return new PngDecoderCore(new PngDecoderOptions() { GeneralOptions = options }).Identify(options.Configuration, stream, cancellationToken);
     }
 
     /// <inheritdoc/>
-    protected override Image<TPixel> Decode<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
+    protected override Image<TPixel> Decode<TPixel>(PngDecoderOptions options, Stream stream, CancellationToken cancellationToken)
     {
         Guard.NotNull(options, nameof(options));
         Guard.NotNull(stream, nameof(stream));
 
         PngDecoderCore decoder = new(options);
-        Image<TPixel> image = decoder.Decode<TPixel>(options.Configuration, stream, cancellationToken);
+        Image<TPixel> image = decoder.Decode<TPixel>(options.GeneralOptions.Configuration, stream, cancellationToken);
 
-        ScaleToTargetSize(options, image);
+        ScaleToTargetSize(options.GeneralOptions, image);
 
         return image;
     }
 
     /// <inheritdoc/>
-    protected override Image Decode(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
+    protected override Image Decode(PngDecoderOptions options, Stream stream, CancellationToken cancellationToken)
     {
         Guard.NotNull(options, nameof(options));
         Guard.NotNull(stream, nameof(stream));
 
         PngDecoderCore decoder = new(options, true);
-        ImageInfo info = decoder.Identify(options.Configuration, stream, cancellationToken);
+        ImageInfo info = decoder.Identify(options.GeneralOptions.Configuration, stream, cancellationToken);
         stream.Position = 0;
 
         PngMetadata meta = info.Metadata.GetPngMetadata();
@@ -99,4 +99,7 @@ public sealed class PngDecoder : ImageDecoder
                 return this.Decode<Rgba32>(options, stream, cancellationToken);
         }
     }
+
+    /// <inheritdoc/>
+    protected override PngDecoderOptions CreateDefaultSpecializedOptions(DecoderOptions options) => new PngDecoderOptions() { GeneralOptions = options };
 }

src/ImageSharp/Formats/Png/PngDecoderCore.cs

@@ -114,27 +114,34 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     /// </summary>
     private PngChunk? nextChunk;
 
+    /// <summary>
+    /// How to handle CRC errors.
+    /// </summary>
+    private readonly PngCrcChunkHandling pngCrcChunkHandling;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="PngDecoderCore"/> class.
     /// </summary>
     /// <param name="options">The decoder options.</param>
-    public PngDecoderCore(DecoderOptions options)
+    public PngDecoderCore(PngDecoderOptions options)
     {
-        this.Options = options;
-        this.configuration = options.Configuration;
-        this.maxFrames = options.MaxFrames;
-        this.skipMetadata = options.SkipMetadata;
+        this.Options = options.GeneralOptions;
+        this.configuration = options.GeneralOptions.Configuration;
+        this.maxFrames = options.GeneralOptions.MaxFrames;
+        this.skipMetadata = options.GeneralOptions.SkipMetadata;
         this.memoryAllocator = this.configuration.MemoryAllocator;
+        this.pngCrcChunkHandling = options.PngCrcChunkHandling;
     }
 
-    internal PngDecoderCore(DecoderOptions options, bool colorMetadataOnly)
+    internal PngDecoderCore(PngDecoderOptions options, bool colorMetadataOnly)
     {
-        this.Options = options;
+        this.Options = options.GeneralOptions;
         this.colorMetadataOnly = colorMetadataOnly;
-        this.maxFrames = options.MaxFrames;
+        this.maxFrames = options.GeneralOptions.MaxFrames;
         this.skipMetadata = true;
-        this.configuration = options.Configuration;
+        this.configuration = options.GeneralOptions.Configuration;
         this.memoryAllocator = this.configuration.MemoryAllocator;
+        this.pngCrcChunkHandling = options.PngCrcChunkHandling;
     }
 
     /// <inheritdoc/>
@@ -576,11 +583,23 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     private void InitializeImage<TPixel>(ImageMetadata metadata, FrameControl frameControl, out Image<TPixel> image)
         where TPixel : unmanaged, IPixel<TPixel>
     {
-        image = Image.CreateUninitialized<TPixel>(
-            this.configuration,
-            this.header.Width,
-            this.header.Height,
-            metadata);
+        // When ignoring data CRCs, we can't use the image constructor that leaves the buffer uncleared.
+        if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll)
+        {
+            image = new Image<TPixel>(
+                this.configuration,
+                this.header.Width,
+                this.header.Height,
+                metadata);
+        }
+        else
+        {
+            image = Image.CreateUninitialized<TPixel>(
+                this.configuration,
+                this.header.Width,
+                this.header.Height,
+                metadata);
+        }
 
         PngFrameMetadata frameMetadata = image.Frames.RootFrame.Metadata.GetPngMetadata();
         frameMetadata.FromChunk(in frameControl);
@@ -803,6 +822,11 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
                     break;
 
                 default:
+                    if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll)
+                    {
+                        goto EXIT;
+                    }
+
                     PngThrowHelper.ThrowUnknownFilter();
                     break;
             }
@@ -812,6 +836,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
             currentRow++;
         }
 
+        EXIT:
         blendMemory?.Dispose();
     }
 
@@ -903,6 +928,11 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
                         break;
 
                     default:
+                        if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll)
+                        {
+                            goto EXIT;
+                        }
+
                         PngThrowHelper.ThrowUnknownFilter();
                         break;
                 }
@@ -937,6 +967,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
             }
         }
 
+        EXIT:
         blendMemory?.Dispose();
     }
 
@@ -1364,7 +1395,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
 
         ReadOnlySpan<byte> compressedData = data[(zeroIndex + 2)..];
 
-        if (this.TryUncompressTextData(compressedData, PngConstants.Encoding, out string? uncompressed)
+        if (this.TryDecompressTextData(compressedData, PngConstants.Encoding, out string? uncompressed)
             && !TryReadTextChunkMetadata(baseMetadata, name, uncompressed))
         {
             metadata.TextData.Add(new PngTextData(name, uncompressed, string.Empty, string.Empty));
@@ -1508,19 +1539,19 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
 
         ReadOnlySpan<byte> compressedData = data[(zeroIndex + 2)..];
 
-        if (this.TryUncompressZlibData(compressedData, out byte[] iccpProfileBytes))
+        if (this.TryDecompressZlibData(compressedData, out byte[] iccpProfileBytes))
         {
             metadata.IccProfile = new IccProfile(iccpProfileBytes);
         }
     }
 
     /// <summary>
-    /// Tries to un-compress zlib compressed data.
+    /// Tries to decompress zlib compressed data.
     /// </summary>
     /// <param name="compressedData">The compressed data.</param>
     /// <param name="uncompressedBytesArray">The uncompressed bytes array.</param>
     /// <returns>True, if de-compressing was successful.</returns>
-    private unsafe bool TryUncompressZlibData(ReadOnlySpan<byte> compressedData, out byte[] uncompressedBytesArray)
+    private unsafe bool TryDecompressZlibData(ReadOnlySpan<byte> compressedData, out byte[] uncompressedBytesArray)
     {
         fixed (byte* compressedDataBase = compressedData)
         {
@@ -1657,7 +1688,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
         {
             ReadOnlySpan<byte> compressedData = data[dataStartIdx..];
 
-            if (this.TryUncompressTextData(compressedData, PngConstants.TranslatedEncoding, out string? uncompressed))
+            if (this.TryDecompressTextData(compressedData, PngConstants.TranslatedEncoding, out string? uncompressed))
             {
                 pngMetadata.TextData.Add(new PngTextData(keyword, uncompressed, language, translatedKeyword));
             }
@@ -1680,9 +1711,9 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     /// <param name="encoding">The string encoding to use.</param>
     /// <param name="value">The uncompressed value.</param>
     /// <returns>The <see cref="bool"/>.</returns>
-    private bool TryUncompressTextData(ReadOnlySpan<byte> compressedData, Encoding encoding, [NotNullWhen(true)] out string? value)
+    private bool TryDecompressTextData(ReadOnlySpan<byte> compressedData, Encoding encoding, [NotNullWhen(true)] out string? value)
     {
-        if (this.TryUncompressZlibData(compressedData, out byte[] uncompressedData))
+        if (this.TryDecompressZlibData(compressedData, out byte[] uncompressedData))
         {
             value = encoding.GetString(uncompressedData);
             return true;
@@ -1705,7 +1736,11 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
 
         Span<byte> buffer = stackalloc byte[20];
 
-        _ = this.currentStream.Read(buffer, 0, 4);
+        int length = this.currentStream.Read(buffer, 0, 4);
+        if (length == 0)
+        {
+            return 0;
+        }
 
         if (this.TryReadChunk(buffer, out PngChunk chunk))
         {
@@ -1734,7 +1769,11 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
 
         Span<byte> buffer = stackalloc byte[20];
 
-        _ = this.currentStream.Read(buffer, 0, 4);
+        int length = this.currentStream.Read(buffer, 0, 4);
+        if (length == 0)
+        {
+            return 0;
+        }
 
         if (this.TryReadChunk(buffer, out PngChunk chunk))
         {
@@ -1771,21 +1810,27 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
             return true;
         }
 
-        if (!this.TryReadChunkLength(buffer, out int length))
+        if (this.currentStream.Position >= this.currentStream.Length - 1)
         {
+            // IEND
             chunk = default;
+            return false;
+        }
 
+        if (!this.TryReadChunkLength(buffer, out int length))
+        {
             // IEND
+            chunk = default;
             return false;
         }
 
-        while (length < 0 || length > (this.currentStream.Length - this.currentStream.Position))
+        while (length < 0)
         {
             // Not a valid chunk so try again until we reach a known chunk.
             if (!this.TryReadChunkLength(buffer, out length))
             {
+                // IEND
                 chunk = default;
-
                 return false;
             }
         }
@@ -1797,13 +1842,14 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
         if (this.colorMetadataOnly && type != PngChunkType.Header && type != PngChunkType.Transparency && type != PngChunkType.Palette)
         {
             chunk = new PngChunk(length, type);
-
             return true;
         }
 
-        long pos = this.currentStream.Position;
+        // A chunk might report a length that exceeds the length of the stream.
+        // Take the minimum of the two values to ensure we don't read past the end of the stream.
+        long position = this.currentStream.Position;
         chunk = new PngChunk(
-            length: length,
+            length: (int)Math.Min(length, this.currentStream.Length - position),
             type: type,
             data: this.ReadChunkData(length));
 
@@ -1813,7 +1859,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
         // was only read to verifying the CRC is correct.
         if (type is PngChunkType.Data or PngChunkType.FrameData)
         {
-            this.currentStream.Position = pos;
+            this.currentStream.Position = position;
         }
 
         return true;
@@ -1827,8 +1873,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     private void ValidateChunk(in PngChunk chunk, Span<byte> buffer)
     {
         uint inputCrc = this.ReadChunkCrc(buffer);
-
-        if (chunk.IsCritical)
+        if (chunk.IsCritical(this.pngCrcChunkHandling))
         {
             Span<byte> chunkType = stackalloc byte[4];
             BinaryPrimitives.WriteUInt32BigEndian(chunkType, (uint)chunk.Type);

src/ImageSharp/Formats/Png/PngDecoderOptions.cs

@@ -0,0 +1,18 @@
+// Copyright (c) Six Labors.
+// Licensed under the Six Labors Split License.
+
+namespace SixLabors.ImageSharp.Formats.Png;
+
+/// <summary>
+/// Configuration options for decoding png images.
+/// </summary>
+public sealed class PngDecoderOptions : ISpecializedDecoderOptions
+{
+    /// <inheritdoc/>
+    public DecoderOptions GeneralOptions { get; init; } = new DecoderOptions();
+
+    /// <summary>
+    /// Gets a value indicating how to handle validation of any CRC (Cyclic Redundancy Check) data within the encoded PNG.
+    /// </summary>
+    public PngCrcChunkHandling PngCrcChunkHandling { get; init; } = PngCrcChunkHandling.IgnoreNonCritical;
+}

tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs

@@ -43,7 +43,7 @@ public class ImageFormatManagerTests
         Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<PngDecoder>().Count());
         Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<BmpDecoder>().Count());
         Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<JpegDecoder>().Count());
-        Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<BmpDecoder>().Count());
+        Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<GifDecoder>().Count());
         Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<TgaDecoder>().Count());
         Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<TiffDecoder>().Count());
         Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType<WebpDecoder>().Count());

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

@@ -470,6 +470,23 @@ public partial class PngDecoderTests
         Assert.Contains("CRC Error. PNG IDAT chunk is corrupt!", ex.Message);
     }
 
+    // https://github.com/SixLabors/ImageSharp/pull/2589
+    [Theory]
+    [WithFile(TestImages.Png.Bad.WrongCrcDataChunk, PixelTypes.Rgba32, true)]
+    [WithFile(TestImages.Png.Bad.Issue2589, PixelTypes.Rgba32, false)]
+    public void Decode_InvalidDataChunkCrc_IgnoreCrcErrors<TPixel>(TestImageProvider<TPixel> provider, bool compare)
+        where TPixel : unmanaged, IPixel<TPixel>
+    {
+        using Image<TPixel> image = provider.GetImage(PngDecoder.Instance, new PngDecoderOptions() { PngCrcChunkHandling = PngCrcChunkHandling.IgnoreData });
+
+        image.DebugSave(provider);
+        if (compare)
+        {
+            // Magick cannot actually decode this image to compare.
+            image.CompareToOriginal(provider, new MagickReferenceDecoder(false));
+        }
+    }
+
     // https://github.com/SixLabors/ImageSharp/issues/1014
     [Theory]
     [WithFileCollection(nameof(TestImagesIssue1014), PixelTypes.Rgba32)]
@@ -640,9 +657,12 @@ public partial class PngDecoderTests
     [Fact]
     public void Binary_PrematureEof()
     {
-        using EofHitCounter eofHitCounter = EofHitCounter.RunDecoder(TestImages.Png.Bad.FlagOfGermany0000016446);
+        PngDecoder decoder = PngDecoder.Instance;
+        PngDecoderOptions options = new() { PngCrcChunkHandling = PngCrcChunkHandling.IgnoreData };
+        using EofHitCounter eofHitCounter = EofHitCounter.RunDecoder(TestImages.Png.Bad.FlagOfGermany0000016446, decoder, options);
 
-        Assert.True(eofHitCounter.EofHitCount <= 2);
+        // TODO: Try to reduce this to 1.
+        Assert.True(eofHitCounter.EofHitCount <= 3);
         Assert.Equal(new Size(200, 120), eofHitCounter.Image.Size);
     }
 }

tests/ImageSharp.Tests/TestImages.cs

@@ -157,6 +157,7 @@ public static class TestImages
             public const string MissingPaletteChunk1 = "Png/missing_plte.png";
             public const string MissingPaletteChunk2 = "Png/missing_plte_2.png";
             public const string InvalidGammaChunk = "Png/length_gama.png";
+            public const string Issue2589 = "Png/issues/Issue_2589.png";
 
             // Zlib errors.
             public const string ZlibOverflow = "Png/zlib-overflow.png";

tests/ImageSharp.Tests/TestUtilities/EofHitCounter.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Six Labors.
 // Licensed under the Six Labors Split License.
 
+using SixLabors.ImageSharp.Formats;
 using SixLabors.ImageSharp.IO;
 
 namespace SixLabors.ImageSharp.Tests.TestUtilities;
@@ -21,6 +22,11 @@ internal class EofHitCounter : IDisposable
 
     public static EofHitCounter RunDecoder(string testImage) => RunDecoder(TestFile.Create(testImage).Bytes);
 
+    public static EofHitCounter RunDecoder<T, TO>(string testImage, T decoder, TO options)
+        where T : SpecializedImageDecoder<TO>
+        where TO : ISpecializedDecoderOptions
+        => RunDecoder(TestFile.Create(testImage).Bytes, decoder, options);
+
     public static EofHitCounter RunDecoder(byte[] imageData)
     {
         BufferedReadStream stream = new(Configuration.Default, new MemoryStream(imageData));
@@ -28,6 +34,15 @@ internal class EofHitCounter : IDisposable
         return new EofHitCounter(stream, image);
     }
 
+    public static EofHitCounter RunDecoder<T, TO>(byte[] imageData, T decoder, TO options)
+        where T : SpecializedImageDecoder<TO>
+        where TO : ISpecializedDecoderOptions
+    {
+        BufferedReadStream stream = new(options.GeneralOptions.Configuration, new MemoryStream(imageData));
+        Image image = decoder.Decode(options, stream);
+        return new EofHitCounter(stream, image);
+    }
+
     public void Dispose()
     {
         this.stream.Dispose();

tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs

@@ -31,12 +31,17 @@ public class MagickReferenceDecoder : ImageDecoder
         {
             IgnoreFileSize = !this.validate
         };
+        PngReadDefines pngReadDefines = new()
+        {
+            IgnoreCrc = !this.validate
+        };
 
         MagickReadSettings settings = new()
         {
             FrameCount = (int)options.MaxFrames
         };
         settings.SetDefines(bmpReadDefines);
+        settings.SetDefines(pngReadDefines);
 
         using MagickImageCollection magickImageCollection = new(stream, settings);
         List<ImageFrame<TPixel>> framesList = new();

tests/Images/Input/Png/issues/Issue_2589.png

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:f7e87701298da4ba0a5cc14e9c04d810125f4958aa338255b14fd19dec15b677
+size 62662