From 8db482834d6b158f944b08f2f64bc20ba205edee Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 19 Feb 2022 02:38:01 +1100 Subject: [PATCH 1/5] Handle empty XMP profiles. Fix #2012 --- .../Common/Extensions/StreamExtensions.cs | 2 +- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 10 ++- src/ImageSharp/Formats/Gif/GifEncoderCore.cs | 4 +- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 2 +- .../Sections/GifXmpApplicationExtension.cs | 72 +++++++++++-------- src/ImageSharp/Metadata/ImageMetadata.cs | 3 +- .../Formats/Gif/GifDecoderTests.cs | 12 ++++ .../Formats/WebP/WebpMetaDataTests.cs | 3 +- .../Metadata/Profiles/XMP/XmpProfileTests.cs | 13 ++-- tests/ImageSharp.Tests/TestImages.cs | 1 + ...2012_Stronghold-Crusader-Extreme-Cover.png | 3 + ...2012_Stronghold-Crusader-Extreme-Cover.gif | 3 + 12 files changed, 82 insertions(+), 46 deletions(-) create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012EmptyXmp_Rgba32_issue2012_Stronghold-Crusader-Extreme-Cover.png create mode 100644 tests/Images/Input/Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif diff --git a/src/ImageSharp/Common/Extensions/StreamExtensions.cs b/src/ImageSharp/Common/Extensions/StreamExtensions.cs index 1193eccee3..8746989b38 100644 --- a/src/ImageSharp/Common/Extensions/StreamExtensions.cs +++ b/src/ImageSharp/Common/Extensions/StreamExtensions.cs @@ -13,7 +13,7 @@ namespace SixLabors.ImageSharp internal static class StreamExtensions { /// - /// Writes data from a stream into the provided buffer. + /// Writes data from a stream from the provided buffer. /// /// The stream. /// The buffer. diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index 72e3ed144c..0a5eba7895 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -265,10 +265,14 @@ private void ReadApplicationExtension() this.stream.Read(this.buffer, 0, GifConstants.ApplicationBlockSize); bool isXmp = this.buffer.AsSpan().StartsWith(GifConstants.XmpApplicationIdentificationBytes); - if (isXmp) + if (isXmp && !this.IgnoreMetadata) { - var extension = GifXmpApplicationExtension.Read(this.stream); - this.metadata.XmpProfile = new XmpProfile(extension.Data); + var extension = GifXmpApplicationExtension.Read(this.stream, this.MemoryAllocator); + if (extension.Data.Length > 0) + { + this.metadata.XmpProfile = new XmpProfile(extension.Data); + } + return; } else diff --git a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs index a21b050a81..e8bb136122 100644 --- a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs @@ -123,7 +123,8 @@ public void Encode(Image image, Stream stream, CancellationToken this.WriteComments(gifMetadata, stream); // Write application extensions. - this.WriteApplicationExtensions(stream, image.Frames.Count, gifMetadata.RepeatCount, metadata.XmpProfile); + XmpProfile xmpProfile = image.Metadata.XmpProfile ?? image.Frames.RootFrame.Metadata.XmpProfile; + this.WriteApplicationExtensions(stream, image.Frames.Count, gifMetadata.RepeatCount, xmpProfile); if (useGlobalTable) { @@ -137,7 +138,6 @@ public void Encode(Image image, Stream stream, CancellationToken // Clean up. quantized.Dispose(); - // TODO: Write extension etc stream.WriteByte(GifConstants.EndIntroducer); } diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index 68227db53d..1db05bb835 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -68,7 +68,7 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream) /// The pixel array to decode to. public void DecodePixels(int dataSize, Buffer2D pixels) { - Guard.MustBeLessThan(dataSize, int.MaxValue, nameof(dataSize)); + Guard.MustBeLessThanOrEqualTo(1 << dataSize, MaxStackSize, nameof(dataSize)); // The resulting index table length. int width = pixels.Width; diff --git a/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs b/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs index 236508fe99..00bf4b7a2b 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs @@ -2,9 +2,9 @@ // Licensed under the Apache License, Version 2.0. using System; -using System.Collections.Generic; using System.IO; -using SixLabors.ImageSharp.Metadata.Profiles.Xmp; +using SixLabors.ImageSharp.IO; +using SixLabors.ImageSharp.Memory; namespace SixLabors.ImageSharp.Formats.Gif { @@ -14,7 +14,10 @@ namespace SixLabors.ImageSharp.Formats.Gif public byte Label => GifConstants.ApplicationExtensionLabel; - public int ContentLength => this.Data.Length + 269; // 12 + Data Length + 1 + 256 + // size : 1 + // identifier : 11 + // magic trailer : 257 + public int ContentLength => this.Data.Length + 269; /// /// Gets the raw Data. @@ -25,40 +28,23 @@ namespace SixLabors.ImageSharp.Formats.Gif /// Reads the XMP metadata from the specified stream. /// /// The stream to read from. + /// The memory allocator. /// The XMP metadata /// Thrown if the XMP block is not properly terminated. - public static GifXmpApplicationExtension Read(Stream stream) + public static GifXmpApplicationExtension Read(Stream stream, MemoryAllocator allocator) { - // Read data in blocks, until an \0 character is encountered. - // We overshoot, indicated by the terminatorIndex variable. - const int bufferSize = 256; - var list = new List(); - int terminationIndex = -1; - while (terminationIndex < 0) - { - byte[] temp = new byte[bufferSize]; - int bytesRead = stream.Read(temp); - list.Add(temp); - terminationIndex = Array.IndexOf(temp, (byte)1); - } + byte[] xmpBytes = ReadXmpData(stream, allocator); - // Pack all the blocks (except magic trailer) into one single array again. - int dataSize = ((list.Count - 1) * bufferSize) + terminationIndex; - byte[] buffer = new byte[dataSize]; - Span bufferSpan = buffer; - int pos = 0; - for (int j = 0; j < list.Count - 1; j++) + // Exclude the "magic trailer", see XMP Specification Part 3, 1.1.2 GIF + int xmpLength = xmpBytes.Length - 256; // 257 - unread 0x0 + byte[] buffer = Array.Empty(); + if (xmpLength > 0) { - list[j].CopyTo(bufferSpan.Slice(pos)); - pos += bufferSize; + buffer = new byte[xmpLength]; + xmpBytes.AsSpan(0, xmpLength).CopyTo(buffer); + stream.Skip(1); // Skip the terminator. } - // Last one only needs the portion until terminationIndex copied over. - Span lastBytes = list[list.Count - 1]; - lastBytes.Slice(0, terminationIndex).CopyTo(bufferSpan.Slice(pos)); - - // Skip the remainder of the magic trailer. - stream.Skip(258 - (bufferSize - terminationIndex)); return new GifXmpApplicationExtension(buffer); } @@ -67,7 +53,7 @@ public int WriteTo(Span buffer) int totalSize = this.ContentLength; if (buffer.Length < totalSize) { - throw new InsufficientMemoryException("Unable to write XMP metadata to GIF image"); + ThrowInsufficientMemory(); } int bytesWritten = 0; @@ -93,5 +79,29 @@ public int WriteTo(Span buffer) return totalSize; } + + private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator) + { + using ChunkedMemoryStream bytes = new(allocator); + + // XMP data doesn't have a fixed length nor is there an indicator of the length. + // So we simply read one byte at a time until we hit the 0x0 value at the end + // of the magic trailer or the end of the stream. + // Using ChunkedMemoryStream reduces the array resize allocation normally associated + // with writing from a non fixed-size buffer. + while (true) + { + int b = stream.ReadByte(); + if (b <= 0) + { + return bytes.ToArray(); + } + + bytes.WriteByte((byte)b); + } + } + + private static void ThrowInsufficientMemory() + => throw new InsufficientMemoryException("Unable to write XMP metadata to GIF image"); } } diff --git a/src/ImageSharp/Metadata/ImageMetadata.cs b/src/ImageSharp/Metadata/ImageMetadata.cs index 89592f776c..4760fa141e 100644 --- a/src/ImageSharp/Metadata/ImageMetadata.cs +++ b/src/ImageSharp/Metadata/ImageMetadata.cs @@ -68,6 +68,7 @@ private ImageMetadata(ImageMetadata other) this.ExifProfile = other.ExifProfile?.DeepClone(); this.IccProfile = other.IccProfile?.DeepClone(); this.IptcProfile = other.IptcProfile?.DeepClone(); + this.XmpProfile = other.XmpProfile?.DeepClone(); } /// @@ -175,7 +176,7 @@ public TFormatMetadata GetFormatMetadata(IImageFormat - public ImageMetadata DeepClone() => new ImageMetadata(this); + public ImageMetadata DeepClone() => new(this); /// /// Synchronizes the profiles with the current metadata. diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index c8ecdb717b..7ca64bea76 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -259,5 +259,17 @@ public void Issue1962(TestImageProvider provider) image.CompareFirstFrameToReferenceOutput(ImageComparer.Exact, provider); } + + // https://github.com/SixLabors/ImageSharp/issues/2012 + [Theory] + [WithFile(TestImages.Gif.Issues.Issue2012EmptyXmp, PixelTypes.Rgba32)] + public void Issue2012EmptyXmp(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(); + + image.DebugSave(provider); + image.CompareFirstFrameToReferenceOutput(ImageComparer.Exact, provider); + } } } diff --git a/tests/ImageSharp.Tests/Formats/WebP/WebpMetaDataTests.cs b/tests/ImageSharp.Tests/Formats/WebP/WebpMetaDataTests.cs index 7fba86b4fe..eaa7fb5646 100644 --- a/tests/ImageSharp.Tests/Formats/WebP/WebpMetaDataTests.cs +++ b/tests/ImageSharp.Tests/Formats/WebP/WebpMetaDataTests.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. using System.IO; +using System.Threading.Tasks; using SixLabors.ImageSharp.Formats.Webp; using SixLabors.ImageSharp.Metadata.Profiles.Exif; using SixLabors.ImageSharp.PixelFormats; @@ -66,7 +67,7 @@ public void IgnoreMetadata_ControlsWhetherIccpIsParsed(TestImageProvider [Theory] [WithFile(TestImages.Webp.Lossy.WithXmp, PixelTypes.Rgba32, false)] [WithFile(TestImages.Webp.Lossy.WithXmp, PixelTypes.Rgba32, true)] - public async void IgnoreMetadata_ControlsWhetherXmpIsParsed(TestImageProvider provider, bool ignoreMetadata) + public async Task IgnoreMetadata_ControlsWhetherXmpIsParsed(TestImageProvider provider, bool ignoreMetadata) where TPixel : unmanaged, IPixel { var decoder = new WebpDecoder { IgnoreMetadata = ignoreMetadata }; diff --git a/tests/ImageSharp.Tests/Metadata/Profiles/XMP/XmpProfileTests.cs b/tests/ImageSharp.Tests/Metadata/Profiles/XMP/XmpProfileTests.cs index 81dad699a1..a3c15d0b3f 100644 --- a/tests/ImageSharp.Tests/Metadata/Profiles/XMP/XmpProfileTests.cs +++ b/tests/ImageSharp.Tests/Metadata/Profiles/XMP/XmpProfileTests.cs @@ -4,6 +4,7 @@ using System; using System.IO; using System.Text; +using System.Threading.Tasks; using System.Xml.Linq; using SixLabors.ImageSharp.Formats; using SixLabors.ImageSharp.Formats.Gif; @@ -31,7 +32,7 @@ public class XmpProfileTests [Theory] [WithFile(TestImages.Gif.Receipt, PixelTypes.Rgba32)] - public async void ReadXmpMetadata_FromGif_Works(TestImageProvider provider) + public async Task ReadXmpMetadata_FromGif_Works(TestImageProvider provider) where TPixel : unmanaged, IPixel { using (Image image = await provider.GetImageAsync(GifDecoder)) @@ -45,7 +46,7 @@ public async void ReadXmpMetadata_FromGif_Works(TestImageProvider(TestImageProvider provider) + public async Task ReadXmpMetadata_FromJpg_Works(TestImageProvider provider) where TPixel : unmanaged, IPixel { using (Image image = await provider.GetImageAsync(JpegDecoder)) @@ -57,7 +58,7 @@ public async void ReadXmpMetadata_FromJpg_Works(TestImageProvider(TestImageProvider provider) + public async Task ReadXmpMetadata_FromPng_Works(TestImageProvider provider) where TPixel : unmanaged, IPixel { using (Image image = await provider.GetImageAsync(PngDecoder)) @@ -69,7 +70,7 @@ public async void ReadXmpMetadata_FromPng_Works(TestImageProvider(TestImageProvider provider) + public async Task ReadXmpMetadata_FromTiff_Works(TestImageProvider provider) where TPixel : unmanaged, IPixel { using (Image image = await provider.GetImageAsync(TiffDecoder)) @@ -81,7 +82,7 @@ public async void ReadXmpMetadata_FromTiff_Works(TestImageProvider(TestImageProvider provider) + public async Task ReadXmpMetadata_FromWebp_Works(TestImageProvider provider) where TPixel : unmanaged, IPixel { using (Image image = await provider.GetImageAsync(WebpDecoder)) @@ -157,7 +158,7 @@ public void WritingJpeg_PreservesXmpProfile() } [Fact] - public async void WritingJpeg_PreservesExtendedXmpProfile() + public async Task WritingJpeg_PreservesExtendedXmpProfile() { // arrange var provider = TestImageProvider.File(TestImages.Jpeg.Baseline.ExtendedXmp); diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 8b943194a5..ffdd6f8f2e 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -452,6 +452,7 @@ public static class Issues public const string Issue1530 = "Gif/issues/issue1530.gif"; public const string InvalidColorIndex = "Gif/issues/issue1668_invalidcolorindex.gif"; public const string Issue1962NoColorTable = "Gif/issues/issue1962_tiniest_gif_1st.gif"; + public const string Issue2012EmptyXmp = "Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif"; } public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 }; diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012EmptyXmp_Rgba32_issue2012_Stronghold-Crusader-Extreme-Cover.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012EmptyXmp_Rgba32_issue2012_Stronghold-Crusader-Extreme-Cover.png new file mode 100644 index 0000000000..c646eb8605 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012EmptyXmp_Rgba32_issue2012_Stronghold-Crusader-Extreme-Cover.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:3ba8295d8a4b087d6c19fbad7e97cef7b5ce1a69b9c4c4f79cee6bc77e41f236 +size 62778 diff --git a/tests/Images/Input/Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif b/tests/Images/Input/Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif new file mode 100644 index 0000000000..90f0e0f1c5 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:97f8fdbabfbd9663bf9940dc33f81edf330b62789d1aa573ae85a520903723e5 +size 77498 From c64078cc17369e92ea83df3ec8687e4d9c41daf3 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 19 Feb 2022 16:58:09 +1100 Subject: [PATCH 2/5] Remove unnecessary throw and optimize write. --- src/ImageSharp/Formats/Gif/GifEncoderCore.cs | 23 +++++++++++-------- .../Sections/GifGraphicControlExtension.cs | 6 ++--- .../GifNetscapeLoopingApplicationExtension.cs | 2 +- .../Sections/GifXmpApplicationExtension.cs | 13 ++--------- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs index e8bb136122..da5b1cb236 100644 --- a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs @@ -428,26 +428,31 @@ private void WriteExtension(TGifExtension extension, Stream strea where TGifExtension : struct, IGifExtension { IMemoryOwner owner = null; - Span buffer; + Span extensionBuffer; int extensionSize = extension.ContentLength; - if (extensionSize > this.buffer.Length - 3) + + if (extensionSize == 0) + { + return; + } + else if (extensionSize > this.buffer.Length - 3) { owner = this.memoryAllocator.Allocate(extensionSize + 3); - buffer = owner.GetSpan(); + extensionBuffer = owner.GetSpan(); } else { - buffer = this.buffer; + extensionBuffer = this.buffer; } - buffer[0] = GifConstants.ExtensionIntroducer; - buffer[1] = extension.Label; + extensionBuffer[0] = GifConstants.ExtensionIntroducer; + extensionBuffer[1] = extension.Label; - extension.WriteTo(buffer.Slice(2)); + extension.WriteTo(extensionBuffer.Slice(2)); - buffer[extensionSize + 2] = GifConstants.Terminator; + extensionBuffer[extensionSize + 2] = GifConstants.Terminator; - stream.Write(buffer, 0, extensionSize + 3); + stream.Write(extensionBuffer, 0, extensionSize + 3); owner?.Dispose(); } diff --git a/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs b/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs index 801849c9b8..8476336942 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs @@ -71,13 +71,11 @@ public int WriteTo(Span buffer) dest = this; - return 5; + return ((IGifExtension)this).ContentLength; } public static GifGraphicControlExtension Parse(ReadOnlySpan buffer) - { - return MemoryMarshal.Cast(buffer)[0]; - } + => MemoryMarshal.Cast(buffer)[0]; public static byte GetPackedValue(GifDisposalMethod disposalMethod, bool userInputFlag = false, bool transparencyFlag = false) { diff --git a/src/ImageSharp/Formats/Gif/Sections/GifNetscapeLoopingApplicationExtension.cs b/src/ImageSharp/Formats/Gif/Sections/GifNetscapeLoopingApplicationExtension.cs index 2c7bed6115..c9e8033dbd 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifNetscapeLoopingApplicationExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifNetscapeLoopingApplicationExtension.cs @@ -40,7 +40,7 @@ public int WriteTo(Span buffer) // 0 means loop indefinitely. Count is set as play n + 1 times. BinaryPrimitives.WriteUInt16LittleEndian(buffer.Slice(14, 2), this.RepeatCount); - return 16; // Length - Introducer + Label + Terminator. + return this.ContentLength; // Length - Introducer + Label + Terminator. } } } diff --git a/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs b/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs index 00bf4b7a2b..8c396e7fb3 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifXmpApplicationExtension.cs @@ -17,7 +17,7 @@ namespace SixLabors.ImageSharp.Formats.Gif // size : 1 // identifier : 11 // magic trailer : 257 - public int ContentLength => this.Data.Length + 269; + public int ContentLength => (this.Data.Length > 0) ? this.Data.Length + 269 : 0; /// /// Gets the raw Data. @@ -50,12 +50,6 @@ public static GifXmpApplicationExtension Read(Stream stream, MemoryAllocator all public int WriteTo(Span buffer) { - int totalSize = this.ContentLength; - if (buffer.Length < totalSize) - { - ThrowInsufficientMemory(); - } - int bytesWritten = 0; buffer[bytesWritten++] = GifConstants.ApplicationBlockSize; @@ -77,7 +71,7 @@ public int WriteTo(Span buffer) buffer[bytesWritten++] = 0x00; - return totalSize; + return this.ContentLength; } private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator) @@ -100,8 +94,5 @@ private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator) bytes.WriteByte((byte)b); } } - - private static void ThrowInsufficientMemory() - => throw new InsufficientMemoryException("Unable to write XMP metadata to GIF image"); } } From c129278cae336b997d1de0f90047326e527d9fe2 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 19 Feb 2022 21:10:15 +1100 Subject: [PATCH 3/5] Don't throw for bad min code, just don't decode indices. --- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 2 +- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 12 ++++++++---- .../ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs | 12 ++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + .../Issue2012BadMinCode_Rgba32_issue2012_drona1.png | 3 +++ tests/Images/Input/Gif/issues/issue2012_drona1.gif | 3 +++ 6 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png create mode 100644 tests/Images/Input/Gif/issues/issue2012_drona1.gif diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index 0a5eba7895..98e012658e 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -378,8 +378,8 @@ private void ReadFrame(ref Image image, ref ImageFrame p } indices = this.Configuration.MemoryAllocator.Allocate2D(this.imageDescriptor.Width, this.imageDescriptor.Height, AllocationOptions.Clean); - this.ReadFrameIndices(indices); + Span rawColorTable = default; if (localColorTable != null) { diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index 1db05bb835..8bb52b6371 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -68,16 +68,20 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream) /// The pixel array to decode to. public void DecodePixels(int dataSize, Buffer2D pixels) { - Guard.MustBeLessThanOrEqualTo(1 << dataSize, MaxStackSize, nameof(dataSize)); + // Calculate the clear code. The value of the clear code is 2 ^ dataSize + int clearCode = 1 << dataSize; + if (clearCode > MaxStackSize) + { + // Don't attempt to decode the frame indices. + // The image is most likely corrupted. + return; + } // The resulting index table length. int width = pixels.Width; int height = pixels.Height; int length = width * height; - // Calculate the clear code. The value of the clear code is 2 ^ dataSize - int clearCode = 1 << dataSize; - int codeSize = dataSize + 1; // Calculate the end code diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 7ca64bea76..289bf6a4fb 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -271,5 +271,17 @@ public void Issue2012EmptyXmp(TestImageProvider provider) image.DebugSave(provider); image.CompareFirstFrameToReferenceOutput(ImageComparer.Exact, provider); } + + // https://github.com/SixLabors/ImageSharp/issues/2012 + [Theory] + [WithFile(TestImages.Gif.Issues.Issue2012BadMinCode, PixelTypes.Rgba32)] + public void Issue2012BadMinCode(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(); + + image.DebugSave(provider); + image.CompareFirstFrameToReferenceOutput(ImageComparer.Exact, provider); + } } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index ffdd6f8f2e..6ff1162256 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -453,6 +453,7 @@ public static class Issues public const string InvalidColorIndex = "Gif/issues/issue1668_invalidcolorindex.gif"; public const string Issue1962NoColorTable = "Gif/issues/issue1962_tiniest_gif_1st.gif"; public const string Issue2012EmptyXmp = "Gif/issues/issue2012_Stronghold-Crusader-Extreme-Cover.gif"; + public const string Issue2012BadMinCode = "Gif/issues/issue2012_drona1.gif"; } public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 }; diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png new file mode 100644 index 0000000000..5e23f55284 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:cf4bc93479140b05b25066eb10c33fa9f82c617828a56526d47f5a8c72035ef0 +size 1871 diff --git a/tests/Images/Input/Gif/issues/issue2012_drona1.gif b/tests/Images/Input/Gif/issues/issue2012_drona1.gif new file mode 100644 index 0000000000..803d684874 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue2012_drona1.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:cbb23b2a19e314969c6da99374ae133d834d76c3f0ab9df4a7edc9334bb065e6 +size 10508 From 78e5b6c19d45b3083b8160cde0b90cd072bead06 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 21 Feb 2022 18:39:29 +1100 Subject: [PATCH 4/5] Throw for corrupt LZW min code. Add test for deferred clear code --- ImageSharp.sln | 7 +++++ src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 4 +-- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 26 ++++++++++++------- .../Formats/Gif/GifDecoderTests.cs | 17 ++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + ...2012BadMinCode_Rgba32_issue2012_drona1.png | 3 --- ...eferredClearCode_Rgba32_bugzilla-55918.png | 3 +++ .../Input/Gif/issues/bugzilla-55918.gif | 3 +++ 8 files changed, 50 insertions(+), 14 deletions(-) delete mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/IssueDeferredClearCode_Rgba32_bugzilla-55918.png create mode 100644 tests/Images/Input/Gif/issues/bugzilla-55918.gif diff --git a/ImageSharp.sln b/ImageSharp.sln index 17d293b434..5428f3394d 100644 --- a/ImageSharp.sln +++ b/ImageSharp.sln @@ -142,6 +142,13 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Gif", "Gif", "{EE3FB0B3-1C3 EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "issues", "issues", "{BF8DFDC1-CEE5-4A37-B216-D3085360C776}" ProjectSection(SolutionItems) = preProject + tests\Images\Input\Gif\issues\bugzilla-55918.gif = tests\Images\Input\Gif\issues\bugzilla-55918.gif + tests\Images\Input\Gif\issues\issue1505_argumentoutofrange.png = tests\Images\Input\Gif\issues\issue1505_argumentoutofrange.png + tests\Images\Input\Gif\issues\issue1530.gif = tests\Images\Input\Gif\issues\issue1530.gif + tests\Images\Input\Gif\issues\issue1668_invalidcolorindex.gif = tests\Images\Input\Gif\issues\issue1668_invalidcolorindex.gif + tests\Images\Input\Gif\issues\issue1962_tiniest_gif_1st.gif = tests\Images\Input\Gif\issues\issue1962_tiniest_gif_1st.gif + tests\Images\Input\Gif\issues\issue2012_drona1.gif = tests\Images\Input\Gif\issues\issue2012_drona1.gif + tests\Images\Input\Gif\issues\issue2012_Stronghold-Crusader-Extreme-Cover.gif = tests\Images\Input\Gif\issues\issue2012_Stronghold-Crusader-Extreme-Cover.gif tests\Images\Input\Gif\issues\issue403_baddescriptorwidth.gif = tests\Images\Input\Gif\issues\issue403_baddescriptorwidth.gif tests\Images\Input\Gif\issues\issue405_badappextlength252-2.gif = tests\Images\Input\Gif\issues\issue405_badappextlength252-2.gif tests\Images\Input\Gif\issues\issue405_badappextlength252.gif = tests\Images\Input\Gif\issues\issue405_badappextlength252.gif diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index 98e012658e..16dca3324f 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -410,9 +410,9 @@ private void ReadFrame(ref Image image, ref ImageFrame p [MethodImpl(MethodImplOptions.AggressiveInlining)] private void ReadFrameIndices(Buffer2D indices) { - int dataSize = this.stream.ReadByte(); + int minCodeSize = this.stream.ReadByte(); using var lzwDecoder = new LzwDecoder(this.Configuration.MemoryAllocator, this.stream); - lzwDecoder.DecodePixels(dataSize, indices); + lzwDecoder.DecodePixels(minCodeSize, indices); } /// diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index 8bb52b6371..470929c4a1 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -64,17 +64,22 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream) /// /// Decodes and decompresses all pixel indices from the stream. /// - /// Size of the data. + /// Minimum code size of the data. /// The pixel array to decode to. - public void DecodePixels(int dataSize, Buffer2D pixels) + public void DecodePixels(int minCodeSize, Buffer2D pixels) { - // Calculate the clear code. The value of the clear code is 2 ^ dataSize - int clearCode = 1 << dataSize; - if (clearCode > MaxStackSize) + // Calculate the clear code. The value of the clear code is 2 ^ minCodeSize + int clearCode = 1 << minCodeSize; + + // It is possible to specify a larger LZW minimum code size than the palette length in bits + // which may leave a gap in the codes where no colors are assigned. + // http://www.matthewflickinger.com/lab/whatsinagif/lzw_image_data.asp#lzw_compression + if (minCodeSize < 2 || clearCode > MaxStackSize) { // Don't attempt to decode the frame indices. - // The image is most likely corrupted. - return; + // Theoretically we could determine a min code size from the length of the provided + // color palette but we won't bother since the image is most likely corrupted. + ThrowBadMinimumCode(); } // The resulting index table length. @@ -82,7 +87,7 @@ public void DecodePixels(int dataSize, Buffer2D pixels) int height = pixels.Height; int length = width * height; - int codeSize = dataSize + 1; + int codeSize = minCodeSize + 1; // Calculate the end code int endCode = clearCode + 1; @@ -169,7 +174,7 @@ public void DecodePixels(int dataSize, Buffer2D pixels) if (code == clearCode) { // Reset the decoder - codeSize = dataSize + 1; + codeSize = minCodeSize + 1; codeMask = (1 << codeSize) - 1; availableCode = clearCode + 2; oldCode = NullCode; @@ -258,5 +263,8 @@ public void Dispose() this.suffix.Dispose(); this.pixelStack.Dispose(); } + + [MethodImpl(InliningOptions.ColdPath)] + public static void ThrowBadMinimumCode() => throw new InvalidImageContentException("Gif Image does not contain a valid LZW minimum code."); } } diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 289bf6a4fb..30bcb72554 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -277,6 +277,23 @@ public void Issue2012EmptyXmp(TestImageProvider provider) [WithFile(TestImages.Gif.Issues.Issue2012BadMinCode, PixelTypes.Rgba32)] public void Issue2012BadMinCode(TestImageProvider provider) where TPixel : unmanaged, IPixel + { + Exception ex = Record.Exception( + () => + { + using Image image = provider.GetImage(); + image.DebugSave(provider); + }); + + Assert.NotNull(ex); + Assert.Contains("Gif Image does not contain a valid LZW minimum code.", ex.Message); + } + + // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 + [Theory] + [WithFile(TestImages.Gif.Issues.DeferredClearCode, PixelTypes.Rgba32)] + public void IssueDeferredClearCode(TestImageProvider provider) + where TPixel : unmanaged, IPixel { using Image image = provider.GetImage(); diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 6ff1162256..6fbf584d8d 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -448,6 +448,7 @@ public static class Issues public const string BadAppExtLength = "Gif/issues/issue405_badappextlength252.gif"; public const string BadAppExtLength_2 = "Gif/issues/issue405_badappextlength252-2.gif"; public const string BadDescriptorWidth = "Gif/issues/issue403_baddescriptorwidth.gif"; + public const string DeferredClearCode = "Gif/issues/bugzilla-55918.gif"; public const string Issue1505 = "Gif/issues/issue1505_argumentoutofrange.png"; public const string Issue1530 = "Gif/issues/issue1530.gif"; public const string InvalidColorIndex = "Gif/issues/issue1668_invalidcolorindex.gif"; diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png deleted file mode 100644 index 5e23f55284..0000000000 --- a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:cf4bc93479140b05b25066eb10c33fa9f82c617828a56526d47f5a8c72035ef0 -size 1871 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/IssueDeferredClearCode_Rgba32_bugzilla-55918.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/IssueDeferredClearCode_Rgba32_bugzilla-55918.png new file mode 100644 index 0000000000..b5769c2c42 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/IssueDeferredClearCode_Rgba32_bugzilla-55918.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:6b33733518b855b25c5e9a1b2f5c93cacf0699a40a459dde795b0ed91a978909 +size 12776 diff --git a/tests/Images/Input/Gif/issues/bugzilla-55918.gif b/tests/Images/Input/Gif/issues/bugzilla-55918.gif new file mode 100644 index 0000000000..929ea67c35 --- /dev/null +++ b/tests/Images/Input/Gif/issues/bugzilla-55918.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d11148669a093c2e39be62bc3482c5863362d28c03c7f26c5a2386d5de28c339 +size 14551 From 85a0ac644d1e51c87b510428c882e51b26aad3e4 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 21 Feb 2022 21:29:13 +1100 Subject: [PATCH 5/5] Use GifThrowHelper --- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index 470929c4a1..2a07200016 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -79,7 +79,7 @@ public void DecodePixels(int minCodeSize, Buffer2D pixels) // Don't attempt to decode the frame indices. // Theoretically we could determine a min code size from the length of the provided // color palette but we won't bother since the image is most likely corrupted. - ThrowBadMinimumCode(); + GifThrowHelper.ThrowInvalidImageContentException("Gif Image does not contain a valid LZW minimum code."); } // The resulting index table length. @@ -263,8 +263,5 @@ public void Dispose() this.suffix.Dispose(); this.pixelStack.Dispose(); } - - [MethodImpl(InliningOptions.ColdPath)] - public static void ThrowBadMinimumCode() => throw new InvalidImageContentException("Gif Image does not contain a valid LZW minimum code."); } }