From 72b80132416a88873f5f836b32d0ef718cb1a4f9 Mon Sep 17 00:00:00 2001 From: Sven Claesson Date: Fri, 10 Nov 2023 13:42:25 +0100 Subject: [PATCH 1/5] refactor PngDecoder to SpecializedImageDecoder --- src/ImageSharp/Formats/Png/PngDecoder.cs | 17 ++++++---- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 32 ++++++++++++------- .../Formats/Png/PngDecoderOptions.cs | 19 +++++++++++ .../Formats/Png/PngDecoderTests.cs | 11 +++++++ .../ReferenceCodecs/MagickReferenceDecoder.cs | 5 +++ 5 files changed, 66 insertions(+), 18 deletions(-) create mode 100644 src/ImageSharp/Formats/Png/PngDecoderOptions.cs diff --git a/src/ImageSharp/Formats/Png/PngDecoder.cs b/src/ImageSharp/Formats/Png/PngDecoder.cs index d226451389..4a7ba3f961 100644 --- a/src/ImageSharp/Formats/Png/PngDecoder.cs +++ b/src/ImageSharp/Formats/Png/PngDecoder.cs @@ -8,7 +8,7 @@ namespace SixLabors.ImageSharp.Formats.Png; /// /// Decoder for generating an image out of a png encoded stream. /// -public sealed class PngDecoder : ImageDecoder +public sealed class PngDecoder : SpecializedImageDecoder { private PngDecoder() { @@ -25,31 +25,31 @@ protected override ImageInfo Identify(DecoderOptions options, Stream stream, Can 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); } /// - 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); - Image image = decoder.Decode(options.Configuration, stream, cancellationToken); + Image image = decoder.Decode(options.GeneralOptions.Configuration, stream, cancellationToken); - ScaleToTargetSize(options, image); + ScaleToTargetSize(options.GeneralOptions, image); return image; } /// - 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 @@ protected override Image Decode(DecoderOptions options, Stream stream, Cancellat return this.Decode(options, stream, cancellationToken); } } + + /// + protected override PngDecoderOptions CreateDefaultSpecializedOptions(DecoderOptions options) => new PngDecoderOptions() { GeneralOptions = options }; } diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index d8305a3f57..0957a7bd53 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -113,27 +113,34 @@ internal sealed class PngDecoderCore : IImageDecoderInternals /// private PngChunk? nextChunk; + /// + /// If true, ADLER32 checksum in the IDAT chunk as well as the chunk CRCs will be ignored. + /// + private bool ignoreCrcErrors; + /// /// Initializes a new instance of the class. /// /// The decoder options. - 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.ignoreCrcErrors = options.IgnoreCrcCheck; } - 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.ignoreCrcErrors = options.IgnoreCrcCheck; } /// @@ -1789,8 +1796,11 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) type: type, data: this.ReadChunkData(length)); - this.ValidateChunk(chunk, buffer); - + if (!this.ignoreCrcErrors) + { + this.ValidateChunk(chunk, buffer); + } + // Restore the stream position for IDAT and fdAT chunks, because it will be decoded later and // was only read to verifying the CRC is correct. if (type is PngChunkType.Data or PngChunkType.FrameData) diff --git a/src/ImageSharp/Formats/Png/PngDecoderOptions.cs b/src/ImageSharp/Formats/Png/PngDecoderOptions.cs new file mode 100644 index 0000000000..c952a18ef2 --- /dev/null +++ b/src/ImageSharp/Formats/Png/PngDecoderOptions.cs @@ -0,0 +1,19 @@ +// Copyright (c) Six Labors. +// Licensed under the Six Labors Split License. + +namespace SixLabors.ImageSharp.Formats.Png; + +/// +/// Configuration options for decoding png images. +/// +public sealed class PngDecoderOptions : ISpecializedDecoderOptions +{ + /// + public DecoderOptions GeneralOptions { get; init; } = new DecoderOptions(); + + /// + /// If true, ADLER32 checksum in the IDAT chunk as well as the chunk CRCs will be ignored. + /// Similar to PNG_CRC_QUIET_USE in libpng. + /// + public bool IgnoreCrcCheck { get; init; } +} diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index 2e11093db6..a2925d254e 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -470,6 +470,17 @@ public void Decode_InvalidDataChunkCrc_ThrowsException(TestImageProvider Assert.Contains("CRC Error. PNG IDAT chunk is corrupt!", ex.Message); } + [Theory] + [WithFile(TestImages.Png.Bad.WrongCrcDataChunk, PixelTypes.Rgba32)] + public void Decode_InvalidDataChunkCrc_IgnoreCrcErrors(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(PngDecoder.Instance, new PngDecoderOptions() { IgnoreCrcCheck = true }); + + image.DebugSave(provider); + image.CompareToOriginal(provider, new MagickReferenceDecoder(false)); + } + // https://github.com/SixLabors/ImageSharp/issues/1014 [Theory] [WithFileCollection(nameof(TestImagesIssue1014), PixelTypes.Rgba32)] diff --git a/tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs b/tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs index ae09c4f3f2..80b5536ebd 100644 --- a/tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs +++ b/tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs @@ -31,12 +31,17 @@ protected override Image Decode(DecoderOptions options, Stream s { 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> framesList = new(); From 0c30a08b8152c2509cc79e86aa6e0606ac864490 Mon Sep 17 00:00:00 2001 From: Sven Claesson Date: Fri, 10 Nov 2023 14:12:24 +0100 Subject: [PATCH 2/5] fix old copy paste issue --- tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs b/tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs index 74c7fc1d09..324bd4783a 100644 --- a/tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs +++ b/tests/ImageSharp.Tests/Formats/ImageFormatManagerTests.cs @@ -43,7 +43,7 @@ public void IfAutoLoadWellKnownFormatsIsTrueAllFormatsAreLoaded() Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); - Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); + Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); Assert.Equal(1, this.DefaultFormatsManager.ImageDecoders.Select(item => item.Value).OfType().Count()); From c99c83ae61a7aa1fdeabfa6fb18b49fe4af77a97 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 29 Nov 2023 19:44:10 +1000 Subject: [PATCH 3/5] Create fine-grained options for CRC handling. --- src/ImageSharp/Formats/Png/PngChunk.cs | 14 +++++---- .../Formats/Png/PngCrcChunkHandling.cs | 30 +++++++++++++++++++ src/ImageSharp/Formats/Png/PngDecoderCore.cs | 18 +++++------ .../Formats/Png/PngDecoderOptions.cs | 5 ++-- .../Formats/Png/PngDecoderTests.cs | 2 +- 5 files changed, 49 insertions(+), 20 deletions(-) create mode 100644 src/ImageSharp/Formats/Png/PngCrcChunkHandling.cs diff --git a/src/ImageSharp/Formats/Png/PngChunk.cs b/src/ImageSharp/Formats/Png/PngChunk.cs index e5fa5fbb72..6ec0df9ad6 100644 --- a/src/ImageSharp/Formats/Png/PngChunk.cs +++ b/src/ImageSharp/Formats/Png/PngChunk.cs @@ -41,9 +41,13 @@ public PngChunk(int length, PngChunkType type, IMemoryOwner data = null) /// /// Gets a value indicating whether the given chunk is critical to decoding /// - public bool IsCritical => - this.Type is PngChunkType.Header or - PngChunkType.Palette or - PngChunkType.Data or - PngChunkType.FrameData; + /// The chunk CRC handling behavior. + 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, + }; } diff --git a/src/ImageSharp/Formats/Png/PngCrcChunkHandling.cs b/src/ImageSharp/Formats/Png/PngCrcChunkHandling.cs new file mode 100644 index 0000000000..264d737fdc --- /dev/null +++ b/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; + +/// +/// Specifies how to handle validation of any CRC (Cyclic Redundancy Check) data within the encoded PNG. +/// +public enum PngCrcChunkHandling +{ + /// + /// Do not ignore any CRC chunk errors. + /// + IgnoreNone, + + /// + /// Ignore CRC errors in non critical chunks. + /// + IgnoreNonCritical, + + /// + /// Ignore CRC errors in data chunks. + /// + IgnoreData, + + /// + /// Ignore CRC errors in all chunks. + /// + IgnoreAll +} diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 355a0bfcf7..4edbe50c78 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -115,9 +115,9 @@ internal sealed class PngDecoderCore : IImageDecoderInternals private PngChunk? nextChunk; /// - /// If true, ADLER32 checksum in the IDAT chunk as well as the chunk CRCs will be ignored. + /// How to handle CRC errors. /// - private bool ignoreCrcErrors; + private readonly PngCrcChunkHandling pngCrcChunkHandling; /// /// Initializes a new instance of the class. @@ -130,7 +130,7 @@ public PngDecoderCore(PngDecoderOptions options) this.maxFrames = options.GeneralOptions.MaxFrames; this.skipMetadata = options.GeneralOptions.SkipMetadata; this.memoryAllocator = this.configuration.MemoryAllocator; - this.ignoreCrcErrors = options.IgnoreCrcCheck; + this.pngCrcChunkHandling = options.PngCrcChunkHandling; } internal PngDecoderCore(PngDecoderOptions options, bool colorMetadataOnly) @@ -141,7 +141,7 @@ internal PngDecoderCore(PngDecoderOptions options, bool colorMetadataOnly) this.skipMetadata = true; this.configuration = options.GeneralOptions.Configuration; this.memoryAllocator = this.configuration.MemoryAllocator; - this.ignoreCrcErrors = options.IgnoreCrcCheck; + this.pngCrcChunkHandling = options.PngCrcChunkHandling; } /// @@ -1797,11 +1797,8 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) type: type, data: this.ReadChunkData(length)); - if (!this.ignoreCrcErrors) - { - this.ValidateChunk(chunk, buffer); - } - + this.ValidateChunk(chunk, buffer); + // Restore the stream position for IDAT and fdAT chunks, because it will be decoded later and // was only read to verifying the CRC is correct. if (type is PngChunkType.Data or PngChunkType.FrameData) @@ -1820,8 +1817,7 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) private void ValidateChunk(in PngChunk chunk, Span buffer) { uint inputCrc = this.ReadChunkCrc(buffer); - - if (chunk.IsCritical) + if (chunk.IsCritical(this.pngCrcChunkHandling)) { Span chunkType = stackalloc byte[4]; BinaryPrimitives.WriteUInt32BigEndian(chunkType, (uint)chunk.Type); diff --git a/src/ImageSharp/Formats/Png/PngDecoderOptions.cs b/src/ImageSharp/Formats/Png/PngDecoderOptions.cs index c952a18ef2..ab6ba4770e 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderOptions.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderOptions.cs @@ -12,8 +12,7 @@ public sealed class PngDecoderOptions : ISpecializedDecoderOptions public DecoderOptions GeneralOptions { get; init; } = new DecoderOptions(); /// - /// If true, ADLER32 checksum in the IDAT chunk as well as the chunk CRCs will be ignored. - /// Similar to PNG_CRC_QUIET_USE in libpng. + /// Gets a value indicating how to handle validation of any CRC (Cyclic Redundancy Check) data within the encoded PNG. /// - public bool IgnoreCrcCheck { get; init; } + public PngCrcChunkHandling PngCrcChunkHandling { get; init; } = PngCrcChunkHandling.IgnoreNonCritical; } diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index 5351af4fcc..cbb625bdf4 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -475,7 +475,7 @@ public void Decode_InvalidDataChunkCrc_ThrowsException(TestImageProvider public void Decode_InvalidDataChunkCrc_IgnoreCrcErrors(TestImageProvider provider) where TPixel : unmanaged, IPixel { - using Image image = provider.GetImage(PngDecoder.Instance, new PngDecoderOptions() { IgnoreCrcCheck = true }); + using Image image = provider.GetImage(PngDecoder.Instance, new PngDecoderOptions() { PngCrcChunkHandling = PngCrcChunkHandling.IgnoreData }); image.DebugSave(provider); image.CompareToOriginal(provider, new MagickReferenceDecoder(false)); From 3d9ceaa8fad18a755ae682bec2dbe74cfc6c59d7 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 29 Nov 2023 21:31:48 +1000 Subject: [PATCH 4/5] Allow decoding early EOF --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 50 +++++++++++++++---- .../Formats/Png/PngDecoderTests.cs | 19 +++++-- tests/ImageSharp.Tests/TestImages.cs | 1 + .../TestUtilities/EofHitCounter.cs | 15 ++++++ tests/Images/Input/Png/issues/Issue_2589.png | 3 ++ 5 files changed, 72 insertions(+), 16 deletions(-) create mode 100644 tests/Images/Input/Png/issues/Issue_2589.png diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 4edbe50c78..64f1e00fff 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -583,11 +583,23 @@ private static void ReadGammaChunk(PngMetadata pngMetadata, ReadOnlySpan d private void InitializeImage(ImageMetadata metadata, FrameControl frameControl, out Image image) where TPixel : unmanaged, IPixel { - image = Image.CreateUninitialized( - 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( + this.configuration, + this.header.Width, + this.header.Height, + metadata); + } + else + { + image = Image.CreateUninitialized( + this.configuration, + this.header.Width, + this.header.Height, + metadata); + } PngFrameMetadata frameMetadata = image.Frames.RootFrame.Metadata.GetPngFrameMetadata(); frameMetadata.FromChunk(in frameControl); @@ -808,6 +820,11 @@ private void DecodePixelData( break; default: + if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll) + { + goto EXIT; + } + PngThrowHelper.ThrowUnknownFilter(); break; } @@ -817,6 +834,7 @@ private void DecodePixelData( currentRow++; } + EXIT: blendMemory?.Dispose(); } @@ -908,6 +926,11 @@ private void DecodeInterlacedPixelData( break; default: + if (this.pngCrcChunkHandling is PngCrcChunkHandling.IgnoreData or PngCrcChunkHandling.IgnoreAll) + { + goto EXIT; + } + PngThrowHelper.ThrowUnknownFilter(); break; } @@ -942,6 +965,7 @@ private void DecodeInterlacedPixelData( } } + EXIT: blendMemory?.Dispose(); } @@ -1761,21 +1785,25 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) return true; } - if (!this.TryReadChunkLength(buffer, out int length)) + if (this.currentStream.Position == this.currentStream.Length) { 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)) { chunk = default; - return false; } } @@ -1791,9 +1819,9 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) return true; } - long pos = this.currentStream.Position; + 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)); @@ -1803,7 +1831,7 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) // 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; diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index cbb625bdf4..ee6e15d7dc 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -470,15 +470,21 @@ public void Decode_InvalidDataChunkCrc_ThrowsException(TestImageProvider 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)] - public void Decode_InvalidDataChunkCrc_IgnoreCrcErrors(TestImageProvider provider) + [WithFile(TestImages.Png.Bad.WrongCrcDataChunk, PixelTypes.Rgba32, true)] + [WithFile(TestImages.Png.Bad.Issue2589, PixelTypes.Rgba32, false)] + public void Decode_InvalidDataChunkCrc_IgnoreCrcErrors(TestImageProvider provider, bool compare) where TPixel : unmanaged, IPixel { using Image image = provider.GetImage(PngDecoder.Instance, new PngDecoderOptions() { PngCrcChunkHandling = PngCrcChunkHandling.IgnoreData }); image.DebugSave(provider); - image.CompareToOriginal(provider, new MagickReferenceDecoder(false)); + if (compare) + { + // Magick cannot actually decode this image to compare. + image.CompareToOriginal(provider, new MagickReferenceDecoder(false)); + } } // https://github.com/SixLabors/ImageSharp/issues/1014 @@ -651,9 +657,12 @@ static void RunTest(string providerDump, string nonContiguousBuffersStr) [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: Undo this. + Assert.True(eofHitCounter.EofHitCount <= 6); Assert.Equal(new Size(200, 120), eofHitCounter.Image.Size); } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 0d9fc55007..ad764628e7 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -157,6 +157,7 @@ public static class Bad 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"; diff --git a/tests/ImageSharp.Tests/TestUtilities/EofHitCounter.cs b/tests/ImageSharp.Tests/TestUtilities/EofHitCounter.cs index d949ce0f2d..c5ffdd6489 100644 --- a/tests/ImageSharp.Tests/TestUtilities/EofHitCounter.cs +++ b/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 @@ public EofHitCounter(BufferedReadStream stream, Image image) public static EofHitCounter RunDecoder(string testImage) => RunDecoder(TestFile.Create(testImage).Bytes); + public static EofHitCounter RunDecoder(string testImage, T decoder, TO options) + where T : SpecializedImageDecoder + 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 @@ public static EofHitCounter RunDecoder(byte[] imageData) return new EofHitCounter(stream, image); } + public static EofHitCounter RunDecoder(byte[] imageData, T decoder, TO options) + where T : SpecializedImageDecoder + 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(); diff --git a/tests/Images/Input/Png/issues/Issue_2589.png b/tests/Images/Input/Png/issues/Issue_2589.png new file mode 100644 index 0000000000..f2f159ea70 --- /dev/null +++ b/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 From 091cb1eabc1275dc81789b7b2b7024de8758aa5f Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 29 Nov 2023 21:49:19 +1000 Subject: [PATCH 5/5] More early returns and renaming --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 33 ++++++++++++------- .../Formats/Png/PngDecoderTests.cs | 4 +-- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 7fb96241a3..84eea9a5a5 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -1395,7 +1395,7 @@ private void ReadCompressedTextChunk(ImageMetadata baseMetadata, PngMetadata met ReadOnlySpan 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)); @@ -1539,19 +1539,19 @@ private void ReadColorProfileChunk(ImageMetadata metadata, ReadOnlySpan da ReadOnlySpan compressedData = data[(zeroIndex + 2)..]; - if (this.TryUncompressZlibData(compressedData, out byte[] iccpProfileBytes)) + if (this.TryDecompressZlibData(compressedData, out byte[] iccpProfileBytes)) { metadata.IccProfile = new IccProfile(iccpProfileBytes); } } /// - /// Tries to un-compress zlib compressed data. + /// Tries to decompress zlib compressed data. /// /// The compressed data. /// The uncompressed bytes array. /// True, if de-compressing was successful. - private unsafe bool TryUncompressZlibData(ReadOnlySpan compressedData, out byte[] uncompressedBytesArray) + private unsafe bool TryDecompressZlibData(ReadOnlySpan compressedData, out byte[] uncompressedBytesArray) { fixed (byte* compressedDataBase = compressedData) { @@ -1688,7 +1688,7 @@ private void ReadInternationalTextChunk(ImageMetadata metadata, ReadOnlySpan 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)); } @@ -1711,9 +1711,9 @@ private void ReadInternationalTextChunk(ImageMetadata metadata, ReadOnlySpanThe string encoding to use. /// The uncompressed value. /// The . - private bool TryUncompressTextData(ReadOnlySpan compressedData, Encoding encoding, [NotNullWhen(true)] out string? value) + private bool TryDecompressTextData(ReadOnlySpan 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; @@ -1736,7 +1736,11 @@ private int ReadNextDataChunk() Span 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)) { @@ -1765,7 +1769,11 @@ private int ReadNextFrameDataChunk() Span 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)) { @@ -1802,8 +1810,9 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) return true; } - if (this.currentStream.Position == this.currentStream.Length) + if (this.currentStream.Position >= this.currentStream.Length - 1) { + // IEND chunk = default; return false; } @@ -1820,6 +1829,7 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) // Not a valid chunk so try again until we reach a known chunk. if (!this.TryReadChunkLength(buffer, out length)) { + // IEND chunk = default; return false; } @@ -1832,10 +1842,11 @@ private bool TryReadChunk(Span buffer, out PngChunk chunk) if (this.colorMetadataOnly && type != PngChunkType.Header && type != PngChunkType.Transparency && type != PngChunkType.Palette) { chunk = new PngChunk(length, type); - return true; } + // 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: (int)Math.Min(length, this.currentStream.Length - position), diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index ee6e15d7dc..bc277bf485 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -661,8 +661,8 @@ public void Binary_PrematureEof() PngDecoderOptions options = new() { PngCrcChunkHandling = PngCrcChunkHandling.IgnoreData }; using EofHitCounter eofHitCounter = EofHitCounter.RunDecoder(TestImages.Png.Bad.FlagOfGermany0000016446, decoder, options); - // TODO: Undo this. - Assert.True(eofHitCounter.EofHitCount <= 6); + // TODO: Try to reduce this to 1. + Assert.True(eofHitCounter.EofHitCount <= 3); Assert.Equal(new Size(200, 120), eofHitCounter.Image.Size); } }