From 9ffcc51ed6b72f557d804f6393b0a610c673c703 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 15 Feb 2022 02:19:35 +0100 Subject: [PATCH 01/19] Respect PreferContiguousImageBuffers in decoders --- .../Decoder/SpectralConverter{TPixel}.cs | 5 ++- src/ImageSharp/Image.Decode.cs | 6 ++- .../Image/LargeImageIntegrationTests.cs | 37 ++++++++++++++++++- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs index 40411ef28..430adeb21 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs @@ -111,7 +111,10 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder this.pixelRowsPerStep = majorVerticalSamplingFactor * blockPixelHeight; // pixel buffer for resulting image - this.pixelBuffer = allocator.Allocate2D(frame.PixelWidth, frame.PixelHeight); + this.pixelBuffer = allocator.Allocate2D( + frame.PixelWidth, + frame.PixelHeight, + this.configuration.PreferContiguousImageBuffers); this.paddedProxyPixelRow = allocator.Allocate(frame.PixelWidth + 3); // component processors from spectral to Rgba32 diff --git a/src/ImageSharp/Image.Decode.cs b/src/ImageSharp/Image.Decode.cs index ee340bf86..c892b82ba 100644 --- a/src/ImageSharp/Image.Decode.cs +++ b/src/ImageSharp/Image.Decode.cs @@ -37,8 +37,10 @@ namespace SixLabors.ImageSharp ImageMetadata metadata) where TPixel : unmanaged, IPixel { - Buffer2D uninitializedMemoryBuffer = - configuration.MemoryAllocator.Allocate2D(width, height); + Buffer2D uninitializedMemoryBuffer = configuration.MemoryAllocator.Allocate2D( + width, + height, + configuration.PreferContiguousImageBuffers); return new Image(configuration, uninitializedMemoryBuffer.FastMemoryGroup, width, height, metadata); } diff --git a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs index b2ee9d673..00714c816 100644 --- a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs +++ b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs @@ -2,7 +2,10 @@ // Licensed under the Apache License, Version 2.0. using System; +using System.IO; using Microsoft.DotNet.RemoteExecutor; +using SixLabors.ImageSharp.Advanced; +using SixLabors.ImageSharp.Formats; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; using SixLabors.ImageSharp.Processing; @@ -32,12 +35,44 @@ namespace SixLabors.ImageSharp.Tests Configuration configuration = Configuration.Default.Clone(); configuration.PreferContiguousImageBuffers = true; - using var image = new Image(configuration, 8192, 4096); + using var image = new Image(configuration, 2048, 2048); Assert.True(image.DangerousTryGetSinglePixelMemory(out Memory mem)); Assert.Equal(8192 * 4096, mem.Length); } } + [Theory] + [InlineData("bmp")] + [InlineData("png")] + [InlineData("jpeg")] + [InlineData("gif")] + [InlineData("tiff")] + [InlineData("webp")] + public void PreferContiguousImageBuffers_LoadImage_BufferIsContiguous(string formatOuter) + { + // Run remotely to avoid large allocation in the test process: + // RemoteExecutor.Invoke(RunTest).Dispose(); + RunTest(formatOuter); + + static void RunTest(string formatInner) + { + Configuration configuration = Configuration.Default.Clone(); + configuration.PreferContiguousImageBuffers = true; + IImageEncoder encoder = configuration.ImageFormatsManager.FindEncoder( + configuration.ImageFormatsManager.FindFormatByFileExtension(formatInner)); + string dir = TestEnvironment.CreateOutputDirectory(".Temp"); + string path = Path.Combine(dir, $"{Guid.NewGuid().ToString()}.{formatInner}"); + using (Image temp = new(2048, 2048)) + { + temp.Save(path, encoder); + } + + using var image = Image.Load(configuration, path); + File.Delete(path); + Assert.Equal(1, image.GetPixelMemoryGroup().Count); + } + } + [Theory] [WithBasicTestPatternImages(width: 10, height: 10, PixelTypes.Rgba32)] public void DangerousTryGetSinglePixelMemory_WhenImageTooLarge_ReturnsFalse(TestImageProvider provider) From 1e410719c0b095de8361183184af043529f2384c Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 15 Feb 2022 02:22:25 +0100 Subject: [PATCH 02/19] fix test --- tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs index 00714c816..f44f124b0 100644 --- a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs +++ b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs @@ -51,8 +51,7 @@ namespace SixLabors.ImageSharp.Tests public void PreferContiguousImageBuffers_LoadImage_BufferIsContiguous(string formatOuter) { // Run remotely to avoid large allocation in the test process: - // RemoteExecutor.Invoke(RunTest).Dispose(); - RunTest(formatOuter); + RemoteExecutor.Invoke(RunTest, formatOuter).Dispose(); static void RunTest(string formatInner) { From f387d4a1615d9a4eef11b9b5d49380c4091803e2 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 15 Feb 2022 03:34:35 +0100 Subject: [PATCH 03/19] Fix test --- tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs index f44f124b0..357d02be4 100644 --- a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs +++ b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs @@ -37,7 +37,7 @@ namespace SixLabors.ImageSharp.Tests using var image = new Image(configuration, 2048, 2048); Assert.True(image.DangerousTryGetSinglePixelMemory(out Memory mem)); - Assert.Equal(8192 * 4096, mem.Length); + Assert.Equal(2048 * 2048, mem.Length); } } From 3ea84859896a45d85a633163e9a350ed95006ac6 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Tue, 15 Feb 2022 06:42:52 +0300 Subject: [PATCH 04/19] Removed redundant CommitConversion() call --- .../Components/Decoder/HuffmanScanDecoder.cs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs index cf2fd0290..e1faf93f4 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs @@ -31,7 +31,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder /// /// Number of component in the current scan. /// - private int componentsCount; + private int scanComponentCount; /// /// The reset interval determined by RST markers. @@ -112,11 +112,12 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder /// /// Decodes the entropy coded data. /// - public void ParseEntropyCodedData(int componentCount) + /// Component count in the current scan. + public void ParseEntropyCodedData(int scanComponentCount) { this.cancellationToken.ThrowIfCancellationRequested(); - this.componentsCount = componentCount; + this.scanComponentCount = scanComponentCount; this.scanBuffer = new HuffmanScanBuffer(this.stream); @@ -148,7 +149,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder private void ParseBaselineData() { - if (this.componentsCount != 1) + if (this.scanComponentCount != 1) { this.ParseBaselineDataInterleaved(); this.spectralConverter.CommitConversion(); @@ -180,7 +181,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder { // Scan an interleaved mcu... process components in order int mcuCol = mcu % mcusPerLine; - for (int k = 0; k < this.componentsCount; k++) + for (int k = 0; k < this.scanComponentCount; k++) { int order = this.frame.ComponentOrder[k]; JpegComponent component = this.components[order]; @@ -228,9 +229,6 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder // Convert from spectral to actual pixels via given converter this.spectralConverter.ConvertStrideBaseline(); } - - // Stride conversion must be sealed for stride conversion approach - this.spectralConverter.CommitConversion(); } private void ParseBaselineDataNonInterleaved() @@ -336,7 +334,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder } // AC scans may have only one component. - if (this.componentsCount != 1) + if (this.scanComponentCount != 1) { invalid = true; } @@ -368,7 +366,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder { this.CheckProgressiveData(); - if (this.componentsCount == 1) + if (this.scanComponentCount == 1) { this.ParseProgressiveDataNonInterleaved(); } @@ -393,7 +391,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder // Scan an interleaved mcu... process components in order int mcuRow = mcu / mcusPerLine; int mcuCol = mcu % mcusPerLine; - for (int k = 0; k < this.componentsCount; k++) + for (int k = 0; k < this.scanComponentCount; k++) { int order = this.frame.ComponentOrder[k]; JpegComponent component = this.components[order]; From 63792362b45498b215f6d22bdfcccc7a0b9a4b55 Mon Sep 17 00:00:00 2001 From: Dirk Lemstra Date: Fri, 18 Feb 2022 12:12:27 +0100 Subject: [PATCH 05/19] Restored checkboxes. --- .github/ISSUE_TEMPLATE/oss-bug-report.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/ISSUE_TEMPLATE/oss-bug-report.yml b/.github/ISSUE_TEMPLATE/oss-bug-report.yml index 00138d511..a4e5619d4 100644 --- a/.github/ISSUE_TEMPLATE/oss-bug-report.yml +++ b/.github/ISSUE_TEMPLATE/oss-bug-report.yml @@ -2,6 +2,18 @@ name: "OSS : Bug Report" description: Create a report to help us improve the project. OSS Issues are not guaranteed to be triaged. labels: ["needs triage"] body: +- type: checkboxes + attributes: + label: Prerequisites + options: + - label: I have written a descriptive issue title + required: true + - label: I have verified that I am running the latest version of ImageSharp + required: true + - label: I have verified if the problem exist in both `DEBUG` and `RELEASE` mode + required: true + - label: I have searched [open](https://github.com/SixLabors/ImageSharp/issues) and [closed](https://github.com/SixLabors/ImageSharp/issues?q=is%3Aissue+is%3Aclosed) issues to ensure it has not already been reported + required: true - type: input attributes: label: ImageSharp version From 92da254bb8203ea5eb2fc8f58f849f487921466e Mon Sep 17 00:00:00 2001 From: Dirk Lemstra Date: Fri, 18 Feb 2022 12:14:19 +0100 Subject: [PATCH 06/19] Restored checkboxes. --- .github/ISSUE_TEMPLATE/commercial-bug-report.yml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.github/ISSUE_TEMPLATE/commercial-bug-report.yml b/.github/ISSUE_TEMPLATE/commercial-bug-report.yml index e96677b24..6b4d914d7 100644 --- a/.github/ISSUE_TEMPLATE/commercial-bug-report.yml +++ b/.github/ISSUE_TEMPLATE/commercial-bug-report.yml @@ -4,6 +4,20 @@ description: | Please contact help@sixlabors.com for issues requiring private support. labels: ["commercial", "needs triage"] body: +- type: checkboxes + attributes: + label: Prerequisites + options: + - label: I have bought a Commercial License + required: true + - label: I have written a descriptive issue title + required: true + - label: I have verified that I am running the latest version of ImageSharp + required: true + - label: I have verified if the problem exist in both `DEBUG` and `RELEASE` mode + required: true + - label: I have searched [open](https://github.com/SixLabors/ImageSharp/issues) and [closed](https://github.com/SixLabors/ImageSharp/issues?q=is%3Aissue+is%3Aclosed) issues to ensure it has not already been reported + required: true - type: input attributes: label: ImageSharp version From 8db482834d6b158f944b08f2f64bc20ba205edee Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 19 Feb 2022 02:38:01 +1100 Subject: [PATCH 07/19] 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 1193eccee..8746989b3 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 72e3ed144..0a5eba789 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -265,10 +265,14 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 a21b050a8..e8bb13612 100644 --- a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs @@ -123,7 +123,8 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 @@ namespace SixLabors.ImageSharp.Formats.Gif // 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 68227db53..1db05bb83 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -68,7 +68,7 @@ namespace SixLabors.ImageSharp.Formats.Gif /// 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 236508fe9..00bf4b7a2 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 89592f776..4760fa141 100644 --- a/src/ImageSharp/Metadata/ImageMetadata.cs +++ b/src/ImageSharp/Metadata/ImageMetadata.cs @@ -68,6 +68,7 @@ namespace SixLabors.ImageSharp.Metadata this.ExifProfile = other.ExifProfile?.DeepClone(); this.IccProfile = other.IccProfile?.DeepClone(); this.IptcProfile = other.IptcProfile?.DeepClone(); + this.XmpProfile = other.XmpProfile?.DeepClone(); } /// @@ -175,7 +176,7 @@ namespace SixLabors.ImageSharp.Metadata } /// - 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 c8ecdb717..7ca64bea7 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -259,5 +259,17 @@ namespace SixLabors.ImageSharp.Tests.Formats.Gif 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 7fba86b4f..eaa7fb564 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 @@ namespace SixLabors.ImageSharp.Tests.Formats.Webp [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 81dad699a..a3c15d0b3 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 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Xmp [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 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Xmp [WithFile(TestImages.Jpeg.Baseline.Lake, PixelTypes.Rgba32)] [WithFile(TestImages.Jpeg.Baseline.Metadata, PixelTypes.Rgba32)] [WithFile(TestImages.Jpeg.Baseline.ExtendedXmp, PixelTypes.Rgba32)] - public async void ReadXmpMetadata_FromJpg_Works(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 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Xmp [Theory] [WithFile(TestImages.Png.XmpColorPalette, PixelTypes.Rgba32)] - public async void ReadXmpMetadata_FromPng_Works(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 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Xmp [Theory] [WithFile(TestImages.Tiff.SampleMetadata, PixelTypes.Rgba32)] - public async void ReadXmpMetadata_FromTiff_Works(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 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Xmp [Theory] [WithFile(TestImages.Webp.Lossy.WithXmp, PixelTypes.Rgba32)] - public async void ReadXmpMetadata_FromWebp_Works(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 @@ namespace SixLabors.ImageSharp.Tests.Metadata.Profiles.Xmp } [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 8b943194a..ffdd6f8f2 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -452,6 +452,7 @@ namespace SixLabors.ImageSharp.Tests 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 000000000..c646eb860 --- /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 000000000..90f0e0f1c --- /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 08/19] 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 e8bb13612..da5b1cb23 100644 --- a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs @@ -428,26 +428,31 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 801849c9b..847633694 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifGraphicControlExtension.cs @@ -71,13 +71,11 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 2c7bed611..c9e8033db 100644 --- a/src/ImageSharp/Formats/Gif/Sections/GifNetscapeLoopingApplicationExtension.cs +++ b/src/ImageSharp/Formats/Gif/Sections/GifNetscapeLoopingApplicationExtension.cs @@ -40,7 +40,7 @@ namespace SixLabors.ImageSharp.Formats.Gif // 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 00bf4b7a2..8c396e7fb 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 @@ namespace SixLabors.ImageSharp.Formats.Gif buffer[bytesWritten++] = 0x00; - return totalSize; + return this.ContentLength; } private static byte[] ReadXmpData(Stream stream, MemoryAllocator allocator) @@ -100,8 +94,5 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 09/19] 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 0a5eba789..98e012658 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -378,8 +378,8 @@ namespace SixLabors.ImageSharp.Formats.Gif } 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 1db05bb83..8bb52b637 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -68,16 +68,20 @@ namespace SixLabors.ImageSharp.Formats.Gif /// 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 7ca64bea7..289bf6a4f 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -271,5 +271,17 @@ namespace SixLabors.ImageSharp.Tests.Formats.Gif 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 ffdd6f8f2..6ff116225 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -453,6 +453,7 @@ namespace SixLabors.ImageSharp.Tests 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 000000000..5e23f5528 --- /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 000000000..803d68487 --- /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 892dbe318f03b4b21ecbf71d03989b2985e0db54 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Sat, 19 Feb 2022 18:12:14 +0100 Subject: [PATCH 10/19] Throw exception, if palette chunk is missing --- src/ImageSharp/Formats/Png/PngScanlineProcessor.cs | 5 +++++ src/ImageSharp/Formats/Png/PngThrowHelper.cs | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs b/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs index 58fa5aca8..26bc566d6 100644 --- a/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs +++ b/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs @@ -240,6 +240,11 @@ namespace SixLabors.ImageSharp.Formats.Png byte[] paletteAlpha) where TPixel : unmanaged, IPixel { + if (palette.IsEmpty) + { + PngThrowHelper.ThrowMissingPalette(); + } + TPixel pixel = default; ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan); ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan); diff --git a/src/ImageSharp/Formats/Png/PngThrowHelper.cs b/src/ImageSharp/Formats/Png/PngThrowHelper.cs index 8700438bd..ad733f010 100644 --- a/src/ImageSharp/Formats/Png/PngThrowHelper.cs +++ b/src/ImageSharp/Formats/Png/PngThrowHelper.cs @@ -21,6 +21,9 @@ namespace SixLabors.ImageSharp.Formats.Png [MethodImpl(InliningOptions.ColdPath)] public static void ThrowNoData() => throw new InvalidImageContentException("PNG Image does not contain a data chunk"); + [MethodImpl(InliningOptions.ColdPath)] + public static void ThrowMissingPalette() => throw new InvalidImageContentException("PNG Image does not contain palette chunk"); + [MethodImpl(InliningOptions.ColdPath)] public static void ThrowInvalidChunkType() => throw new InvalidImageContentException("Invalid PNG data."); From d98171fa54180dec22db9e0e207e803edc4b6c8a Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Sat, 19 Feb 2022 18:23:45 +0100 Subject: [PATCH 11/19] Add tests for missing palette chunk --- .../Formats/Png/PngDecoderTests.cs | 16 ++++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 2 ++ tests/Images/Input/Png/missing_plte.png | 3 +++ tests/Images/Input/Png/missing_plte_2.png | 3 +++ 4 files changed, 24 insertions(+) create mode 100644 tests/Images/Input/Png/missing_plte.png create mode 100644 tests/Images/Input/Png/missing_plte_2.png diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index c29f8c589..f81a77ca1 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -265,6 +265,22 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png Assert.Contains("PNG Image does not contain a data chunk", ex.Message); } + [Theory] + [WithFile(TestImages.Png.Bad.MissingPaletteChunk1, PixelTypes.Rgba32)] + [WithFile(TestImages.Png.Bad.MissingPaletteChunk2, PixelTypes.Rgba32)] + public void Decode_MissingPaletteChunk_ThrowsException(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + Exception ex = Record.Exception( + () => + { + using Image image = provider.GetImage(PngDecoder); + image.DebugSave(provider); + }); + Assert.NotNull(ex); + Assert.Contains("PNG Image does not contain palette chunk", ex.Message); + } + [Theory] [WithFile(TestImages.Png.Bad.BitDepthZero, PixelTypes.Rgba32)] [WithFile(TestImages.Png.Bad.BitDepthThree, PixelTypes.Rgba32)] diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 8b943194a..d03a60183 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -127,6 +127,8 @@ namespace SixLabors.ImageSharp.Tests public const string MissingDataChunk = "Png/xdtn0g01.png"; public const string WrongCrcDataChunk = "Png/xcsn0g01.png"; public const string CorruptedChunk = "Png/big-corrupted-chunk.png"; + public const string MissingPaletteChunk1 = "Png/missing_plte.png"; + public const string MissingPaletteChunk2 = "Png/missing_plte_2.png"; // Zlib errors. public const string ZlibOverflow = "Png/zlib-overflow.png"; diff --git a/tests/Images/Input/Png/missing_plte.png b/tests/Images/Input/Png/missing_plte.png new file mode 100644 index 000000000..0c24883fb --- /dev/null +++ b/tests/Images/Input/Png/missing_plte.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:73fd17a394f8258f4767986bc427c0160277819349c937f18cb29044e7549bc8 +size 506 diff --git a/tests/Images/Input/Png/missing_plte_2.png b/tests/Images/Input/Png/missing_plte_2.png new file mode 100644 index 000000000..8fc6580e5 --- /dev/null +++ b/tests/Images/Input/Png/missing_plte_2.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:797844db61a937c6f31ecb392c8416fbf106017413ba55c6576e0b1fcfc1cf9c +size 597 From 29ddc6053e1a27ab095e87d8baa015f7428c53ef Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Sat, 19 Feb 2022 20:31:07 +0100 Subject: [PATCH 12/19] Change error message to "...a palette chunk" --- src/ImageSharp/Formats/Png/PngThrowHelper.cs | 2 +- tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngThrowHelper.cs b/src/ImageSharp/Formats/Png/PngThrowHelper.cs index ad733f010..ae7d16ec7 100644 --- a/src/ImageSharp/Formats/Png/PngThrowHelper.cs +++ b/src/ImageSharp/Formats/Png/PngThrowHelper.cs @@ -22,7 +22,7 @@ namespace SixLabors.ImageSharp.Formats.Png public static void ThrowNoData() => throw new InvalidImageContentException("PNG Image does not contain a data chunk"); [MethodImpl(InliningOptions.ColdPath)] - public static void ThrowMissingPalette() => throw new InvalidImageContentException("PNG Image does not contain palette chunk"); + public static void ThrowMissingPalette() => throw new InvalidImageContentException("PNG Image does not contain a palette chunk"); [MethodImpl(InliningOptions.ColdPath)] public static void ThrowInvalidChunkType() => throw new InvalidImageContentException("Invalid PNG data."); diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index f81a77ca1..a92856c73 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -278,7 +278,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png image.DebugSave(provider); }); Assert.NotNull(ex); - Assert.Contains("PNG Image does not contain palette chunk", ex.Message); + Assert.Contains("PNG Image does not contain a palette chunk", ex.Message); } [Theory] From 78e5b6c19d45b3083b8160cde0b90cd072bead06 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 21 Feb 2022 18:39:29 +1100 Subject: [PATCH 13/19] 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 17d293b43..5428f3394 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 98e012658..16dca3324 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -410,9 +410,9 @@ namespace SixLabors.ImageSharp.Formats.Gif [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 8bb52b637..470929c4a 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -64,17 +64,22 @@ namespace SixLabors.ImageSharp.Formats.Gif /// /// 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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 289bf6a4f..30bcb7255 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -277,6 +277,23 @@ namespace SixLabors.ImageSharp.Tests.Formats.Gif [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 6ff116225..6fbf584d8 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -448,6 +448,7 @@ namespace SixLabors.ImageSharp.Tests 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 5e23f5528..000000000 --- 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 000000000..b5769c2c4 --- /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 000000000..929ea67c3 --- /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 14/19] 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 470929c4a..2a0720001 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -79,7 +79,7 @@ namespace SixLabors.ImageSharp.Formats.Gif // 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 @@ namespace SixLabors.ImageSharp.Formats.Gif 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."); } } From 0fa6085611e69e5e4eb2827e8293eb747585a034 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Mon, 21 Feb 2022 12:10:23 +0100 Subject: [PATCH 15/19] Throw exception, if gamma chunk does not contain enough data --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 10 ++++++++-- src/ImageSharp/Formats/Png/PngThrowHelper.cs | 3 +++ .../Formats/Png/PngDecoderTests.cs | 15 +++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + tests/Images/Input/Png/length_gama.png | 3 +++ 5 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 tests/Images/Input/Png/length_gama.png diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index f5fc86ee4..090e1e2b0 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -429,10 +429,16 @@ namespace SixLabors.ImageSharp.Formats.Png /// The metadata to read to. /// The data containing physical data. private void ReadGammaChunk(PngMetadata pngMetadata, ReadOnlySpan data) + { + if (data.Length < 4) + { + PngThrowHelper.ThrowInvalidGamma(); + } - // The value is encoded as a 4-byte unsigned integer, representing gamma times 100000. // For example, a gamma of 1/2.2 would be stored as 45455. - => pngMetadata.Gamma = BinaryPrimitives.ReadUInt32BigEndian(data) * 1e-5F; + // The value is encoded as a 4-byte unsigned integer, representing gamma times 100000. + pngMetadata.Gamma = BinaryPrimitives.ReadUInt32BigEndian(data) * 1e-5F; + } /// /// Initializes the image and various buffers needed for processing diff --git a/src/ImageSharp/Formats/Png/PngThrowHelper.cs b/src/ImageSharp/Formats/Png/PngThrowHelper.cs index ae7d16ec7..07372dae2 100644 --- a/src/ImageSharp/Formats/Png/PngThrowHelper.cs +++ b/src/ImageSharp/Formats/Png/PngThrowHelper.cs @@ -24,6 +24,9 @@ namespace SixLabors.ImageSharp.Formats.Png [MethodImpl(InliningOptions.ColdPath)] public static void ThrowMissingPalette() => throw new InvalidImageContentException("PNG Image does not contain a palette chunk"); + [MethodImpl(InliningOptions.ColdPath)] + public static void ThrowInvalidGamma() => throw new InvalidImageContentException("PNG Image does not contain enough data for the gamma chunk"); + [MethodImpl(InliningOptions.ColdPath)] public static void ThrowInvalidChunkType() => throw new InvalidImageContentException("Invalid PNG data."); diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index a92856c73..7fd87258b 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -281,6 +281,21 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png Assert.Contains("PNG Image does not contain a palette chunk", ex.Message); } + [Theory] + [WithFile(TestImages.Png.Bad.InvalidGammaChunk, PixelTypes.Rgba32)] + public void Decode_InvalidGammaChunk_ThrowsException(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + Exception ex = Record.Exception( + () => + { + using Image image = provider.GetImage(PngDecoder); + image.DebugSave(provider); + }); + Assert.NotNull(ex); + Assert.Contains("PNG Image does not contain enough data for the gamma chunk", ex.Message); + } + [Theory] [WithFile(TestImages.Png.Bad.BitDepthZero, PixelTypes.Rgba32)] [WithFile(TestImages.Png.Bad.BitDepthThree, PixelTypes.Rgba32)] diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index c6bd20a7d..5bce99ce1 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -129,6 +129,7 @@ namespace SixLabors.ImageSharp.Tests public const string CorruptedChunk = "Png/big-corrupted-chunk.png"; 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"; // Zlib errors. public const string ZlibOverflow = "Png/zlib-overflow.png"; diff --git a/tests/Images/Input/Png/length_gama.png b/tests/Images/Input/Png/length_gama.png new file mode 100644 index 000000000..caf0fb01d --- /dev/null +++ b/tests/Images/Input/Png/length_gama.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:824766b34739727c722e88611d7b55401452c2970cd433f56e5f9f1b36d6950d +size 1285 From d76c40a43a85f3ff82bdbc05f577fe7236095939 Mon Sep 17 00:00:00 2001 From: Brian Popow Date: Mon, 21 Feb 2022 14:39:10 +0100 Subject: [PATCH 16/19] Ignore invalid gamma chunks --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 3 ++- src/ImageSharp/Formats/Png/PngThrowHelper.cs | 3 --- tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | 5 ++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 090e1e2b0..60437e015 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -432,7 +432,8 @@ namespace SixLabors.ImageSharp.Formats.Png { if (data.Length < 4) { - PngThrowHelper.ThrowInvalidGamma(); + // Ignore invalid gamma chunks. + return; } // For example, a gamma of 1/2.2 would be stored as 45455. diff --git a/src/ImageSharp/Formats/Png/PngThrowHelper.cs b/src/ImageSharp/Formats/Png/PngThrowHelper.cs index 07372dae2..ae7d16ec7 100644 --- a/src/ImageSharp/Formats/Png/PngThrowHelper.cs +++ b/src/ImageSharp/Formats/Png/PngThrowHelper.cs @@ -24,9 +24,6 @@ namespace SixLabors.ImageSharp.Formats.Png [MethodImpl(InliningOptions.ColdPath)] public static void ThrowMissingPalette() => throw new InvalidImageContentException("PNG Image does not contain a palette chunk"); - [MethodImpl(InliningOptions.ColdPath)] - public static void ThrowInvalidGamma() => throw new InvalidImageContentException("PNG Image does not contain enough data for the gamma chunk"); - [MethodImpl(InliningOptions.ColdPath)] public static void ThrowInvalidChunkType() => throw new InvalidImageContentException("Invalid PNG data."); diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index 7fd87258b..0af5d9995 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -283,7 +283,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png [Theory] [WithFile(TestImages.Png.Bad.InvalidGammaChunk, PixelTypes.Rgba32)] - public void Decode_InvalidGammaChunk_ThrowsException(TestImageProvider provider) + public void Decode_InvalidGammaChunk_Ignored(TestImageProvider provider) where TPixel : unmanaged, IPixel { Exception ex = Record.Exception( @@ -292,8 +292,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png using Image image = provider.GetImage(PngDecoder); image.DebugSave(provider); }); - Assert.NotNull(ex); - Assert.Contains("PNG Image does not contain enough data for the gamma chunk", ex.Message); + Assert.Null(ex); } [Theory] From 854ea5d0962921a8f1831376f77b1bef8a57b75a Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Mon, 21 Feb 2022 23:25:22 +0100 Subject: [PATCH 17/19] workaround for #2001 / https://github.com/dotnet/runtime/issues/65466 --- .../UniformUnmanagedMemoryPoolMemoryAllocator.cs | 13 +++++++++++-- .../UniformUnmanagedPoolMemoryAllocatorTests.cs | 13 +++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs index 16a3cb73d..5f591a9bb 100644 --- a/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs @@ -74,6 +74,10 @@ namespace SixLabors.ImageSharp.Memory this.nonPoolAllocator = new UnmanagedMemoryAllocator(unmanagedBufferSizeInBytes); } + // This delegate allows overriding the method returning the available system memory, + // so we can test our workaround for https://github.com/dotnet/runtime/issues/65466 + internal static Func GetTotalAvailableMemoryBytes { get; set; } = () => GC.GetGCMemoryInfo().TotalAvailableMemoryBytes; + /// protected internal override int GetBufferCapacityInBytes() => this.poolBufferSizeInBytes; @@ -152,8 +156,13 @@ namespace SixLabors.ImageSharp.Memory // https://github.com/dotnet/runtime/issues/55126#issuecomment-876779327 if (Environment.Is64BitProcess || !RuntimeInformation.FrameworkDescription.StartsWith(".NET 5.0")) { - GCMemoryInfo info = GC.GetGCMemoryInfo(); - return info.TotalAvailableMemoryBytes / 8; + long total = GetTotalAvailableMemoryBytes(); + + // Workaround for https://github.com/dotnet/runtime/issues/65466 + if (total > 0) + { + return total / 8; + } } #endif diff --git a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs index 45a7cc278..0c59e334c 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs @@ -379,5 +379,18 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators g1.GetSpan()[0] = 42; } } + + [Fact] + public void Issue2001_NegativeMemoryReportedByGc() + { + RemoteExecutor.Invoke(RunTest).Dispose(); + + static void RunTest() + { + // Emulate GC.GetGCMemoryInfo() issue https://github.com/dotnet/runtime/issues/65466 + UniformUnmanagedMemoryPoolMemoryAllocator.GetTotalAvailableMemoryBytes = () => -402354176; + _ = MemoryAllocator.Create(); + } + } } } From 5b82c57ce773566da96ef176c0fa1426e70c91fe Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 22 Feb 2022 12:42:36 +1100 Subject: [PATCH 18/19] Add compiler directives --- .../UniformUnmanagedMemoryPoolMemoryAllocator.cs | 7 ++++--- .../Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs index 5f591a9bb..0da4ff9f8 100644 --- a/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs @@ -1,11 +1,10 @@ -// Copyright (c) Six Labors. +// Copyright (c) Six Labors. // Licensed under the Apache License, Version 2.0. using System; using System.Buffers; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; -using System.Threading; using SixLabors.ImageSharp.Memory.Internals; namespace SixLabors.ImageSharp.Memory @@ -22,7 +21,7 @@ namespace SixLabors.ImageSharp.Memory private readonly int poolCapacity; private readonly UniformUnmanagedMemoryPool.TrimSettings trimSettings; - private UniformUnmanagedMemoryPool pool; + private readonly UniformUnmanagedMemoryPool pool; private readonly UnmanagedMemoryAllocator nonPoolAllocator; public UniformUnmanagedMemoryPoolMemoryAllocator(int? maxPoolSizeMegabytes) @@ -74,9 +73,11 @@ namespace SixLabors.ImageSharp.Memory this.nonPoolAllocator = new UnmanagedMemoryAllocator(unmanagedBufferSizeInBytes); } +#if NETCOREAPP3_1_OR_GREATER // This delegate allows overriding the method returning the available system memory, // so we can test our workaround for https://github.com/dotnet/runtime/issues/65466 internal static Func GetTotalAvailableMemoryBytes { get; set; } = () => GC.GetGCMemoryInfo().TotalAvailableMemoryBytes; +#endif /// protected internal override int GetBufferCapacityInBytes() => this.poolBufferSizeInBytes; diff --git a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs index 0c59e334c..414f991f7 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs @@ -380,6 +380,7 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators } } +#if NETCOREAPP3_1_OR_GREATER [Fact] public void Issue2001_NegativeMemoryReportedByGc() { @@ -392,5 +393,6 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators _ = MemoryAllocator.Create(); } } +#endif } } From de5f661177f4c902a98fd091a406abea92e2ba36 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Tue, 22 Feb 2022 02:53:43 +0100 Subject: [PATCH 19/19] make GetTotalAvailableMemoryBytes conditional --- .../Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs | 2 ++ .../Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs b/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs index 5f591a9bb..5b72a5804 100644 --- a/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs +++ b/src/ImageSharp/Memory/Allocators/UniformUnmanagedMemoryPoolMemoryAllocator.cs @@ -74,9 +74,11 @@ namespace SixLabors.ImageSharp.Memory this.nonPoolAllocator = new UnmanagedMemoryAllocator(unmanagedBufferSizeInBytes); } +#if NETCOREAPP3_1_OR_GREATER // This delegate allows overriding the method returning the available system memory, // so we can test our workaround for https://github.com/dotnet/runtime/issues/65466 internal static Func GetTotalAvailableMemoryBytes { get; set; } = () => GC.GetGCMemoryInfo().TotalAvailableMemoryBytes; +#endif /// protected internal override int GetBufferCapacityInBytes() => this.poolBufferSizeInBytes; diff --git a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs index 0c59e334c..414f991f7 100644 --- a/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs +++ b/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs @@ -380,6 +380,7 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators } } +#if NETCOREAPP3_1_OR_GREATER [Fact] public void Issue2001_NegativeMemoryReportedByGc() { @@ -392,5 +393,6 @@ namespace SixLabors.ImageSharp.Tests.Memory.Allocators _ = MemoryAllocator.Create(); } } +#endif } }