From 04d760113f765da7f2a5971e43a498bb09d04227 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 19:51:58 +0100 Subject: [PATCH 01/18] tests --- .../ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs | 12 ++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 4 +++- .../00.png | 3 +++ .../00.png | 3 +++ .../01.png | 3 +++ tests/Images/Input/Gif/issues/issue_2859_A.gif | 3 +++ tests/Images/Input/Gif/issues/issue_2859_B.gif | 3 +++ 7 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png create mode 100644 tests/Images/Input/Gif/issues/issue_2859_A.gif create mode 100644 tests/Images/Input/Gif/issues/issue_2859_B.gif diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 9ecf8ad502..4e42722d2c 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -317,5 +317,17 @@ public void IssueTooLargeLzwBits(TestImageProvider provider) image.DebugSaveMultiFrame(provider); image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact); } + + // https://github.com/SixLabors/ImageSharp/issues/2859 + [Theory] + [WithFile(TestImages.Gif.Issues.Issue2859_A, PixelTypes.Rgba32)] + [WithFile(TestImages.Gif.Issues.Issue2859_B, PixelTypes.Rgba32)] + public void Issue2859_LZWPixelStackOverflow(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(); + image.DebugSaveMultiFrame(provider); + image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact); + } } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 1ba9ef2ef3..bb646800d0 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -464,7 +464,9 @@ public static class Issues 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 const string Issue2758 = "Gif/issues/issue_2758.gif"; + public const string Issue2758 = "Gif/issues/issue_2758.gif"; + public const string Issue2859_A = "Gif/issues/issue_2859_A.gif"; + public const string Issue2859_B = "Gif/issues/issue_2859_B.gif"; } public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 }; diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png new file mode 100644 index 0000000000..d8c8df1263 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:daa78347749c6ff49891e2e379a373599cd35c98b453af9bf8eac52f615f935c +size 12237 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png new file mode 100644 index 0000000000..36c3683187 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:731299281f942f277ce6803e0adda3b5dd0395eb79cae26cabc9d56905fae0fd +size 1833 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png new file mode 100644 index 0000000000..c03e5817f0 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:50ccac7739142578d99a76b6d39ba377099d4a7ac30cbb0a5aee44ef1e7c9c8c +size 1271 diff --git a/tests/Images/Input/Gif/issues/issue_2859_A.gif b/tests/Images/Input/Gif/issues/issue_2859_A.gif new file mode 100644 index 0000000000..f19a047525 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue_2859_A.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:50a1a4afc62a3a36ff83596f1eb55d91cdd184c64e0d1339bbea17205c23eee1 +size 3406142 diff --git a/tests/Images/Input/Gif/issues/issue_2859_B.gif b/tests/Images/Input/Gif/issues/issue_2859_B.gif new file mode 100644 index 0000000000..109b0f8797 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue_2859_B.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:db9b2992be772a4f0ac495e994a17c7c50fb6de9794cfb9afc4a3dc26ffdfae0 +size 4543 From e68d0b4d2ace28bd65caba1f2f580e9b876bec3f Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 19:58:04 +0100 Subject: [PATCH 02/18] backport LzwDecoder improvements --- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 324 ++++++++++++++--------- 1 file changed, 193 insertions(+), 131 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index aa4327a1a0..6ad88f95cf 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -38,22 +38,22 @@ internal sealed class LzwDecoder : IDisposable /// /// The prefix buffer. /// - private readonly IMemoryOwner prefix; + private readonly IMemoryOwner prefixOwner; /// /// The suffix buffer. /// - private readonly IMemoryOwner suffix; + private readonly IMemoryOwner suffixOwner; /// /// The scratch buffer for reading data blocks. /// - private readonly IMemoryOwner scratchBuffer; + private readonly IMemoryOwner bufferOwner; /// /// The pixel stack buffer. /// - private readonly IMemoryOwner pixelStack; + private readonly IMemoryOwner pixelStackOwner; private readonly int minCodeSize; private readonly int clearCode; private readonly int endCode; @@ -80,11 +80,12 @@ internal sealed class LzwDecoder : IDisposable public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream, int minCodeSize) { this.stream = stream ?? throw new ArgumentNullException(nameof(stream)); + Guard.IsTrue(IsValidMinCodeSize(minCodeSize), nameof(minCodeSize), "Invalid minimum code size."); - this.prefix = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); - this.suffix = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); - this.pixelStack = memoryAllocator.Allocate(MaxStackSize + 1, AllocationOptions.Clean); - this.scratchBuffer = memoryAllocator.Allocate(byte.MaxValue, AllocationOptions.None); + this.prefixOwner = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); + this.suffixOwner = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); + this.pixelStackOwner = memoryAllocator.Allocate(MaxStackSize + 1, AllocationOptions.Clean); + this.bufferOwner = memoryAllocator.Allocate(byte.MaxValue, AllocationOptions.None); this.minCodeSize = minCodeSize; // Calculate the clear code. The value of the clear code is 2 ^ minCodeSize @@ -94,11 +95,15 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream, in this.endCode = this.clearCode + 1; this.availableCode = this.clearCode + 2; - ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); - for (this.code = 0; this.code < this.clearCode; this.code++) + // Fill the suffix buffer with the initial values represented by the number of colors. + Span suffix = this.suffixOwner.GetSpan().Slice(0, this.clearCode); + int i; + for (i = 0; i < suffix.Length; i++) { - Unsafe.Add(ref suffixRef, this.code) = (byte)this.code; + suffix[i] = i; } + + this.code = i; } /// @@ -113,8 +118,7 @@ public static bool IsValidMinCodeSize(int 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 - int clearCode = 1 << minCodeSize; - if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize) + if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || 1 << minCodeSize > MaxStackSize) { // Don't attempt to decode the frame indices. // Theoretically we could determine a min code size from the length of the provided @@ -133,112 +137,139 @@ public void DecodePixelRow(Span indices) { indices.Clear(); - ref byte pixelsRowRef = ref MemoryMarshal.GetReference(indices); - ref int prefixRef = ref MemoryMarshal.GetReference(this.prefix.GetSpan()); - ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); - ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan()); - Span buffer = this.scratchBuffer.GetSpan(); - - int x = 0; - int xyz = 0; - while (xyz < indices.Length) + // Get span values from the owners. + Span prefix = this.prefixOwner.GetSpan(); + Span suffix = this.suffixOwner.GetSpan(); + Span pixelStack = this.pixelStackOwner.GetSpan(); + Span buffer = this.bufferOwner.GetSpan(); + + // Cache frequently accessed instance fields into locals. + // This helps avoid repeated field loads inside the tight loop. + BufferedReadStream stream = this.stream; + int top = this.top; + int bits = this.bits; + int codeSize = this.codeSize; + int codeMask = this.codeMask; + int minCodeSize = this.minCodeSize; + int availableCode = this.availableCode; + int oldCode = this.oldCode; + int first = this.first; + int data = this.data; + int count = this.count; + int bufferIndex = this.bufferIndex; + int code = this.code; + int clearCode = this.clearCode; + int endCode = this.endCode; + + int i = 0; + while (i < indices.Length) { - if (this.top == 0) + if (top == 0) { - if (this.bits < this.codeSize) + if (bits < codeSize) { // Load bytes until there are enough bits for a code. - if (this.count == 0) + if (count == 0) { // Read a new data block. - this.count = this.ReadBlock(buffer); - if (this.count == 0) + count = ReadBlock(stream, buffer); + if (count == 0) { break; } - this.bufferIndex = 0; + bufferIndex = 0; } - this.data += buffer[this.bufferIndex] << this.bits; - - this.bits += 8; - this.bufferIndex++; - this.count--; + data += buffer[bufferIndex] << bits; + bits += 8; + bufferIndex++; + count--; continue; } // Get the next code - this.code = this.data & this.codeMask; - this.data >>= this.codeSize; - this.bits -= this.codeSize; + code = data & codeMask; + data >>= codeSize; + bits -= codeSize; // Interpret the code - if (this.code > this.availableCode || this.code == this.endCode) + if (code > availableCode || code == endCode) { break; } - if (this.code == this.clearCode) + if (code == clearCode) { // Reset the decoder - this.codeSize = this.minCodeSize + 1; - this.codeMask = (1 << this.codeSize) - 1; - this.availableCode = this.clearCode + 2; - this.oldCode = NullCode; + codeSize = minCodeSize + 1; + codeMask = (1 << codeSize) - 1; + availableCode = clearCode + 2; + oldCode = NullCode; continue; } - if (this.oldCode == NullCode) + if (oldCode == NullCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.oldCode = this.code; - this.first = this.code; + pixelStack[top++] = suffix[code]; + oldCode = code; + first = code; continue; } - int inCode = this.code; - if (this.code == this.availableCode) + int inCode = code; + if (code == availableCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = (byte)this.first; - - this.code = this.oldCode; + pixelStack[top++] = first; + code = oldCode; } - while (this.code > this.clearCode) + while (code > clearCode && top < MaxStackSize) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.code = Unsafe.Add(ref prefixRef, this.code); + pixelStack[top++] = suffix[code]; + code = prefix[code]; } - int suffixCode = Unsafe.Add(ref suffixRef, this.code); - this.first = suffixCode; - Unsafe.Add(ref pixelStackRef, this.top++) = suffixCode; + int suffixCode = suffix[code]; + first = suffixCode; + pixelStack[top++] = suffixCode; - // Fix for Gifs that have "deferred clear code" as per here : + // Fix for GIFs that have "deferred clear code" as per: // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 - if (this.availableCode < MaxStackSize) + if (availableCode < MaxStackSize) { - Unsafe.Add(ref prefixRef, this.availableCode) = this.oldCode; - Unsafe.Add(ref suffixRef, this.availableCode) = this.first; - this.availableCode++; - if (this.availableCode == this.codeMask + 1 && this.availableCode < MaxStackSize) + prefix[availableCode] = oldCode; + suffix[availableCode] = first; + availableCode++; + if (availableCode == codeMask + 1 && availableCode < MaxStackSize) { - this.codeSize++; - this.codeMask = (1 << this.codeSize) - 1; + codeSize++; + codeMask = (1 << codeSize) - 1; } } - this.oldCode = inCode; + oldCode = inCode; } // Pop a pixel off the pixel stack. - this.top--; + top--; - // Clear missing pixels - xyz++; - Unsafe.Add(ref pixelsRowRef, x++) = (byte)Unsafe.Add(ref pixelStackRef, this.top); + // Clear missing pixels. + indices[i++] = (byte)pixelStack[top]; } + + // Write back the local values to the instance fields. + this.top = top; + this.bits = bits; + this.codeSize = codeSize; + this.codeMask = codeMask; + this.availableCode = availableCode; + this.oldCode = oldCode; + this.first = first; + this.data = data; + this.count = count; + this.bufferIndex = bufferIndex; + this.code = code; } /// @@ -247,130 +278,161 @@ public void DecodePixelRow(Span indices) /// The resulting index table length. public void SkipIndices(int length) { - ref int prefixRef = ref MemoryMarshal.GetReference(this.prefix.GetSpan()); - ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); - ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan()); - Span buffer = this.scratchBuffer.GetSpan(); - - int xyz = 0; - while (xyz < length) + // Get span values from the owners. + Span prefix = this.prefixOwner.GetSpan(); + Span suffix = this.suffixOwner.GetSpan(); + Span pixelStack = this.pixelStackOwner.GetSpan(); + Span buffer = this.bufferOwner.GetSpan(); + + // Cache frequently accessed instance fields into locals. + // This helps avoid repeated field loads inside the tight loop. + BufferedReadStream stream = this.stream; + int top = this.top; + int bits = this.bits; + int codeSize = this.codeSize; + int codeMask = this.codeMask; + int minCodeSize = this.minCodeSize; + int availableCode = this.availableCode; + int oldCode = this.oldCode; + int first = this.first; + int data = this.data; + int count = this.count; + int bufferIndex = this.bufferIndex; + int code = this.code; + int clearCode = this.clearCode; + int endCode = this.endCode; + + int i = 0; + while (i < length) { - if (this.top == 0) + if (top == 0) { - if (this.bits < this.codeSize) + if (bits < codeSize) { // Load bytes until there are enough bits for a code. - if (this.count == 0) + if (count == 0) { // Read a new data block. - this.count = this.ReadBlock(buffer); - if (this.count == 0) + count = ReadBlock(stream, buffer); + if (count == 0) { break; } - this.bufferIndex = 0; + bufferIndex = 0; } - this.data += buffer[this.bufferIndex] << this.bits; - - this.bits += 8; - this.bufferIndex++; - this.count--; + data += buffer[bufferIndex] << bits; + bits += 8; + bufferIndex++; + count--; continue; } // Get the next code - this.code = this.data & this.codeMask; - this.data >>= this.codeSize; - this.bits -= this.codeSize; + code = data & codeMask; + data >>= codeSize; + bits -= codeSize; // Interpret the code - if (this.code > this.availableCode || this.code == this.endCode) + if (code > availableCode || code == endCode) { break; } - if (this.code == this.clearCode) + if (code == clearCode) { // Reset the decoder - this.codeSize = this.minCodeSize + 1; - this.codeMask = (1 << this.codeSize) - 1; - this.availableCode = this.clearCode + 2; - this.oldCode = NullCode; + codeSize = minCodeSize + 1; + codeMask = (1 << codeSize) - 1; + availableCode = clearCode + 2; + oldCode = NullCode; continue; } - if (this.oldCode == NullCode) + if (oldCode == NullCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.oldCode = this.code; - this.first = this.code; + pixelStack[top++] = suffix[code]; + oldCode = code; + first = code; continue; } - int inCode = this.code; - if (this.code == this.availableCode) + int inCode = code; + if (code == availableCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = (byte)this.first; - - this.code = this.oldCode; + pixelStack[top++] = first; + code = oldCode; } - while (this.code > this.clearCode) + while (code > clearCode && top < MaxStackSize) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.code = Unsafe.Add(ref prefixRef, this.code); + pixelStack[top++] = suffix[code]; + code = prefix[code]; } - int suffixCode = Unsafe.Add(ref suffixRef, this.code); - this.first = suffixCode; - Unsafe.Add(ref pixelStackRef, this.top++) = suffixCode; + int suffixCode = suffix[code]; + first = suffixCode; + pixelStack[top++] = suffixCode; - // Fix for Gifs that have "deferred clear code" as per here : + // Fix for GIFs that have "deferred clear code" as per: // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 - if (this.availableCode < MaxStackSize) + if (availableCode < MaxStackSize) { - Unsafe.Add(ref prefixRef, this.availableCode) = this.oldCode; - Unsafe.Add(ref suffixRef, this.availableCode) = this.first; - this.availableCode++; - if (this.availableCode == this.codeMask + 1 && this.availableCode < MaxStackSize) + prefix[availableCode] = oldCode; + suffix[availableCode] = first; + availableCode++; + if (availableCode == codeMask + 1 && availableCode < MaxStackSize) { - this.codeSize++; - this.codeMask = (1 << this.codeSize) - 1; + codeSize++; + codeMask = (1 << codeSize) - 1; } } - this.oldCode = inCode; + oldCode = inCode; } // Pop a pixel off the pixel stack. - this.top--; + top--; - // Clear missing pixels - xyz++; + // Skip missing pixels. + i++; } + + // Write back the local values to the instance fields. + this.top = top; + this.bits = bits; + this.codeSize = codeSize; + this.codeMask = codeMask; + this.availableCode = availableCode; + this.oldCode = oldCode; + this.first = first; + this.data = data; + this.count = count; + this.bufferIndex = bufferIndex; + this.code = code; } /// /// Reads the next data block from the stream. A data block begins with a byte, /// which defines the size of the block, followed by the block itself. /// + /// The stream to read from. /// The buffer to store the block in. /// /// The . /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - private int ReadBlock(Span buffer) + private static int ReadBlock(BufferedReadStream stream, Span buffer) { - int bufferSize = this.stream.ReadByte(); + int bufferSize = stream.ReadByte(); if (bufferSize < 1) { return 0; } - int count = this.stream.Read(buffer, 0, bufferSize); + int count = stream.Read(buffer, 0, bufferSize); return count != bufferSize ? 0 : bufferSize; } @@ -378,10 +440,10 @@ private int ReadBlock(Span buffer) /// public void Dispose() { - this.prefix.Dispose(); - this.suffix.Dispose(); - this.pixelStack.Dispose(); - this.scratchBuffer.Dispose(); + this.prefixOwner.Dispose(); + this.suffixOwner.Dispose(); + this.pixelStackOwner.Dispose(); + this.bufferOwner.Dispose(); } } } From c287b32cad870a2276fd0b3769c77516c81f3a54 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 20:12:44 +0100 Subject: [PATCH 03/18] revert accidental push to release/2.1.x --- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 324 +++++++----------- .../Formats/Gif/GifDecoderTests.cs | 12 - tests/ImageSharp.Tests/TestImages.cs | 4 +- .../00.png | 3 - .../00.png | 3 - .../01.png | 3 - .../Images/Input/Gif/issues/issue_2859_A.gif | 3 - .../Images/Input/Gif/issues/issue_2859_B.gif | 3 - 8 files changed, 132 insertions(+), 223 deletions(-) delete mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png delete mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png delete mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png delete mode 100644 tests/Images/Input/Gif/issues/issue_2859_A.gif delete mode 100644 tests/Images/Input/Gif/issues/issue_2859_B.gif diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index 6ad88f95cf..aa4327a1a0 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -38,22 +38,22 @@ internal sealed class LzwDecoder : IDisposable /// /// The prefix buffer. /// - private readonly IMemoryOwner prefixOwner; + private readonly IMemoryOwner prefix; /// /// The suffix buffer. /// - private readonly IMemoryOwner suffixOwner; + private readonly IMemoryOwner suffix; /// /// The scratch buffer for reading data blocks. /// - private readonly IMemoryOwner bufferOwner; + private readonly IMemoryOwner scratchBuffer; /// /// The pixel stack buffer. /// - private readonly IMemoryOwner pixelStackOwner; + private readonly IMemoryOwner pixelStack; private readonly int minCodeSize; private readonly int clearCode; private readonly int endCode; @@ -80,12 +80,11 @@ internal sealed class LzwDecoder : IDisposable public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream, int minCodeSize) { this.stream = stream ?? throw new ArgumentNullException(nameof(stream)); - Guard.IsTrue(IsValidMinCodeSize(minCodeSize), nameof(minCodeSize), "Invalid minimum code size."); - this.prefixOwner = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); - this.suffixOwner = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); - this.pixelStackOwner = memoryAllocator.Allocate(MaxStackSize + 1, AllocationOptions.Clean); - this.bufferOwner = memoryAllocator.Allocate(byte.MaxValue, AllocationOptions.None); + this.prefix = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); + this.suffix = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); + this.pixelStack = memoryAllocator.Allocate(MaxStackSize + 1, AllocationOptions.Clean); + this.scratchBuffer = memoryAllocator.Allocate(byte.MaxValue, AllocationOptions.None); this.minCodeSize = minCodeSize; // Calculate the clear code. The value of the clear code is 2 ^ minCodeSize @@ -95,15 +94,11 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream, in this.endCode = this.clearCode + 1; this.availableCode = this.clearCode + 2; - // Fill the suffix buffer with the initial values represented by the number of colors. - Span suffix = this.suffixOwner.GetSpan().Slice(0, this.clearCode); - int i; - for (i = 0; i < suffix.Length; i++) + ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); + for (this.code = 0; this.code < this.clearCode; this.code++) { - suffix[i] = i; + Unsafe.Add(ref suffixRef, this.code) = (byte)this.code; } - - this.code = i; } /// @@ -118,7 +113,8 @@ public static bool IsValidMinCodeSize(int 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 || minCodeSize > MaximumLzwBits || 1 << minCodeSize > MaxStackSize) + int clearCode = 1 << minCodeSize; + if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize) { // Don't attempt to decode the frame indices. // Theoretically we could determine a min code size from the length of the provided @@ -137,139 +133,112 @@ public void DecodePixelRow(Span indices) { indices.Clear(); - // Get span values from the owners. - Span prefix = this.prefixOwner.GetSpan(); - Span suffix = this.suffixOwner.GetSpan(); - Span pixelStack = this.pixelStackOwner.GetSpan(); - Span buffer = this.bufferOwner.GetSpan(); - - // Cache frequently accessed instance fields into locals. - // This helps avoid repeated field loads inside the tight loop. - BufferedReadStream stream = this.stream; - int top = this.top; - int bits = this.bits; - int codeSize = this.codeSize; - int codeMask = this.codeMask; - int minCodeSize = this.minCodeSize; - int availableCode = this.availableCode; - int oldCode = this.oldCode; - int first = this.first; - int data = this.data; - int count = this.count; - int bufferIndex = this.bufferIndex; - int code = this.code; - int clearCode = this.clearCode; - int endCode = this.endCode; - - int i = 0; - while (i < indices.Length) + ref byte pixelsRowRef = ref MemoryMarshal.GetReference(indices); + ref int prefixRef = ref MemoryMarshal.GetReference(this.prefix.GetSpan()); + ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); + ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan()); + Span buffer = this.scratchBuffer.GetSpan(); + + int x = 0; + int xyz = 0; + while (xyz < indices.Length) { - if (top == 0) + if (this.top == 0) { - if (bits < codeSize) + if (this.bits < this.codeSize) { // Load bytes until there are enough bits for a code. - if (count == 0) + if (this.count == 0) { // Read a new data block. - count = ReadBlock(stream, buffer); - if (count == 0) + this.count = this.ReadBlock(buffer); + if (this.count == 0) { break; } - bufferIndex = 0; + this.bufferIndex = 0; } - data += buffer[bufferIndex] << bits; - bits += 8; - bufferIndex++; - count--; + this.data += buffer[this.bufferIndex] << this.bits; + + this.bits += 8; + this.bufferIndex++; + this.count--; continue; } // Get the next code - code = data & codeMask; - data >>= codeSize; - bits -= codeSize; + this.code = this.data & this.codeMask; + this.data >>= this.codeSize; + this.bits -= this.codeSize; // Interpret the code - if (code > availableCode || code == endCode) + if (this.code > this.availableCode || this.code == this.endCode) { break; } - if (code == clearCode) + if (this.code == this.clearCode) { // Reset the decoder - codeSize = minCodeSize + 1; - codeMask = (1 << codeSize) - 1; - availableCode = clearCode + 2; - oldCode = NullCode; + this.codeSize = this.minCodeSize + 1; + this.codeMask = (1 << this.codeSize) - 1; + this.availableCode = this.clearCode + 2; + this.oldCode = NullCode; continue; } - if (oldCode == NullCode) + if (this.oldCode == NullCode) { - pixelStack[top++] = suffix[code]; - oldCode = code; - first = code; + Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); + this.oldCode = this.code; + this.first = this.code; continue; } - int inCode = code; - if (code == availableCode) + int inCode = this.code; + if (this.code == this.availableCode) { - pixelStack[top++] = first; - code = oldCode; + Unsafe.Add(ref pixelStackRef, this.top++) = (byte)this.first; + + this.code = this.oldCode; } - while (code > clearCode && top < MaxStackSize) + while (this.code > this.clearCode) { - pixelStack[top++] = suffix[code]; - code = prefix[code]; + Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); + this.code = Unsafe.Add(ref prefixRef, this.code); } - int suffixCode = suffix[code]; - first = suffixCode; - pixelStack[top++] = suffixCode; + int suffixCode = Unsafe.Add(ref suffixRef, this.code); + this.first = suffixCode; + Unsafe.Add(ref pixelStackRef, this.top++) = suffixCode; - // Fix for GIFs that have "deferred clear code" as per: + // Fix for Gifs that have "deferred clear code" as per here : // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 - if (availableCode < MaxStackSize) + if (this.availableCode < MaxStackSize) { - prefix[availableCode] = oldCode; - suffix[availableCode] = first; - availableCode++; - if (availableCode == codeMask + 1 && availableCode < MaxStackSize) + Unsafe.Add(ref prefixRef, this.availableCode) = this.oldCode; + Unsafe.Add(ref suffixRef, this.availableCode) = this.first; + this.availableCode++; + if (this.availableCode == this.codeMask + 1 && this.availableCode < MaxStackSize) { - codeSize++; - codeMask = (1 << codeSize) - 1; + this.codeSize++; + this.codeMask = (1 << this.codeSize) - 1; } } - oldCode = inCode; + this.oldCode = inCode; } // Pop a pixel off the pixel stack. - top--; + this.top--; - // Clear missing pixels. - indices[i++] = (byte)pixelStack[top]; + // Clear missing pixels + xyz++; + Unsafe.Add(ref pixelsRowRef, x++) = (byte)Unsafe.Add(ref pixelStackRef, this.top); } - - // Write back the local values to the instance fields. - this.top = top; - this.bits = bits; - this.codeSize = codeSize; - this.codeMask = codeMask; - this.availableCode = availableCode; - this.oldCode = oldCode; - this.first = first; - this.data = data; - this.count = count; - this.bufferIndex = bufferIndex; - this.code = code; } /// @@ -278,161 +247,130 @@ public void DecodePixelRow(Span indices) /// The resulting index table length. public void SkipIndices(int length) { - // Get span values from the owners. - Span prefix = this.prefixOwner.GetSpan(); - Span suffix = this.suffixOwner.GetSpan(); - Span pixelStack = this.pixelStackOwner.GetSpan(); - Span buffer = this.bufferOwner.GetSpan(); - - // Cache frequently accessed instance fields into locals. - // This helps avoid repeated field loads inside the tight loop. - BufferedReadStream stream = this.stream; - int top = this.top; - int bits = this.bits; - int codeSize = this.codeSize; - int codeMask = this.codeMask; - int minCodeSize = this.minCodeSize; - int availableCode = this.availableCode; - int oldCode = this.oldCode; - int first = this.first; - int data = this.data; - int count = this.count; - int bufferIndex = this.bufferIndex; - int code = this.code; - int clearCode = this.clearCode; - int endCode = this.endCode; - - int i = 0; - while (i < length) + ref int prefixRef = ref MemoryMarshal.GetReference(this.prefix.GetSpan()); + ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); + ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan()); + Span buffer = this.scratchBuffer.GetSpan(); + + int xyz = 0; + while (xyz < length) { - if (top == 0) + if (this.top == 0) { - if (bits < codeSize) + if (this.bits < this.codeSize) { // Load bytes until there are enough bits for a code. - if (count == 0) + if (this.count == 0) { // Read a new data block. - count = ReadBlock(stream, buffer); - if (count == 0) + this.count = this.ReadBlock(buffer); + if (this.count == 0) { break; } - bufferIndex = 0; + this.bufferIndex = 0; } - data += buffer[bufferIndex] << bits; - bits += 8; - bufferIndex++; - count--; + this.data += buffer[this.bufferIndex] << this.bits; + + this.bits += 8; + this.bufferIndex++; + this.count--; continue; } // Get the next code - code = data & codeMask; - data >>= codeSize; - bits -= codeSize; + this.code = this.data & this.codeMask; + this.data >>= this.codeSize; + this.bits -= this.codeSize; // Interpret the code - if (code > availableCode || code == endCode) + if (this.code > this.availableCode || this.code == this.endCode) { break; } - if (code == clearCode) + if (this.code == this.clearCode) { // Reset the decoder - codeSize = minCodeSize + 1; - codeMask = (1 << codeSize) - 1; - availableCode = clearCode + 2; - oldCode = NullCode; + this.codeSize = this.minCodeSize + 1; + this.codeMask = (1 << this.codeSize) - 1; + this.availableCode = this.clearCode + 2; + this.oldCode = NullCode; continue; } - if (oldCode == NullCode) + if (this.oldCode == NullCode) { - pixelStack[top++] = suffix[code]; - oldCode = code; - first = code; + Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); + this.oldCode = this.code; + this.first = this.code; continue; } - int inCode = code; - if (code == availableCode) + int inCode = this.code; + if (this.code == this.availableCode) { - pixelStack[top++] = first; - code = oldCode; + Unsafe.Add(ref pixelStackRef, this.top++) = (byte)this.first; + + this.code = this.oldCode; } - while (code > clearCode && top < MaxStackSize) + while (this.code > this.clearCode) { - pixelStack[top++] = suffix[code]; - code = prefix[code]; + Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); + this.code = Unsafe.Add(ref prefixRef, this.code); } - int suffixCode = suffix[code]; - first = suffixCode; - pixelStack[top++] = suffixCode; + int suffixCode = Unsafe.Add(ref suffixRef, this.code); + this.first = suffixCode; + Unsafe.Add(ref pixelStackRef, this.top++) = suffixCode; - // Fix for GIFs that have "deferred clear code" as per: + // Fix for Gifs that have "deferred clear code" as per here : // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 - if (availableCode < MaxStackSize) + if (this.availableCode < MaxStackSize) { - prefix[availableCode] = oldCode; - suffix[availableCode] = first; - availableCode++; - if (availableCode == codeMask + 1 && availableCode < MaxStackSize) + Unsafe.Add(ref prefixRef, this.availableCode) = this.oldCode; + Unsafe.Add(ref suffixRef, this.availableCode) = this.first; + this.availableCode++; + if (this.availableCode == this.codeMask + 1 && this.availableCode < MaxStackSize) { - codeSize++; - codeMask = (1 << codeSize) - 1; + this.codeSize++; + this.codeMask = (1 << this.codeSize) - 1; } } - oldCode = inCode; + this.oldCode = inCode; } // Pop a pixel off the pixel stack. - top--; + this.top--; - // Skip missing pixels. - i++; + // Clear missing pixels + xyz++; } - - // Write back the local values to the instance fields. - this.top = top; - this.bits = bits; - this.codeSize = codeSize; - this.codeMask = codeMask; - this.availableCode = availableCode; - this.oldCode = oldCode; - this.first = first; - this.data = data; - this.count = count; - this.bufferIndex = bufferIndex; - this.code = code; } /// /// Reads the next data block from the stream. A data block begins with a byte, /// which defines the size of the block, followed by the block itself. /// - /// The stream to read from. /// The buffer to store the block in. /// /// The . /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static int ReadBlock(BufferedReadStream stream, Span buffer) + private int ReadBlock(Span buffer) { - int bufferSize = stream.ReadByte(); + int bufferSize = this.stream.ReadByte(); if (bufferSize < 1) { return 0; } - int count = stream.Read(buffer, 0, bufferSize); + int count = this.stream.Read(buffer, 0, bufferSize); return count != bufferSize ? 0 : bufferSize; } @@ -440,10 +378,10 @@ private static int ReadBlock(BufferedReadStream stream, Span buffer) /// public void Dispose() { - this.prefixOwner.Dispose(); - this.suffixOwner.Dispose(); - this.pixelStackOwner.Dispose(); - this.bufferOwner.Dispose(); + this.prefix.Dispose(); + this.suffix.Dispose(); + this.pixelStack.Dispose(); + this.scratchBuffer.Dispose(); } } } diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 4e42722d2c..9ecf8ad502 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -317,17 +317,5 @@ public void IssueTooLargeLzwBits(TestImageProvider provider) image.DebugSaveMultiFrame(provider); image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact); } - - // https://github.com/SixLabors/ImageSharp/issues/2859 - [Theory] - [WithFile(TestImages.Gif.Issues.Issue2859_A, PixelTypes.Rgba32)] - [WithFile(TestImages.Gif.Issues.Issue2859_B, PixelTypes.Rgba32)] - public void Issue2859_LZWPixelStackOverflow(TestImageProvider provider) - where TPixel : unmanaged, IPixel - { - using Image image = provider.GetImage(); - image.DebugSaveMultiFrame(provider); - image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact); - } } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index bb646800d0..1ba9ef2ef3 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -464,9 +464,7 @@ public static class Issues 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 const string Issue2758 = "Gif/issues/issue_2758.gif"; - public const string Issue2859_A = "Gif/issues/issue_2859_A.gif"; - public const string Issue2859_B = "Gif/issues/issue_2859_B.gif"; + public const string Issue2758 = "Gif/issues/issue_2758.gif"; } public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 }; diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png deleted file mode 100644 index d8c8df1263..0000000000 --- a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:daa78347749c6ff49891e2e379a373599cd35c98b453af9bf8eac52f615f935c -size 12237 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png deleted file mode 100644 index 36c3683187..0000000000 --- a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:731299281f942f277ce6803e0adda3b5dd0395eb79cae26cabc9d56905fae0fd -size 1833 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png deleted file mode 100644 index c03e5817f0..0000000000 --- a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:50ccac7739142578d99a76b6d39ba377099d4a7ac30cbb0a5aee44ef1e7c9c8c -size 1271 diff --git a/tests/Images/Input/Gif/issues/issue_2859_A.gif b/tests/Images/Input/Gif/issues/issue_2859_A.gif deleted file mode 100644 index f19a047525..0000000000 --- a/tests/Images/Input/Gif/issues/issue_2859_A.gif +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:50a1a4afc62a3a36ff83596f1eb55d91cdd184c64e0d1339bbea17205c23eee1 -size 3406142 diff --git a/tests/Images/Input/Gif/issues/issue_2859_B.gif b/tests/Images/Input/Gif/issues/issue_2859_B.gif deleted file mode 100644 index 109b0f8797..0000000000 --- a/tests/Images/Input/Gif/issues/issue_2859_B.gif +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:db9b2992be772a4f0ac495e994a17c7c50fb6de9794cfb9afc4a3dc26ffdfae0 -size 4543 From 367c077f59edc7bc1a1a8d2a5ab3ff2a72561432 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 20:14:53 +0100 Subject: [PATCH 04/18] Backport #2859 to release/2.1 --- src/ImageSharp/Formats/Gif/LzwDecoder.cs | 324 +++++++++++------- .../Formats/Gif/GifDecoderTests.cs | 12 + tests/ImageSharp.Tests/TestImages.cs | 4 +- .../00.png | 3 + .../00.png | 3 + .../01.png | 3 + .../Images/Input/Gif/issues/issue_2859_A.gif | 3 + .../Images/Input/Gif/issues/issue_2859_B.gif | 3 + 8 files changed, 223 insertions(+), 132 deletions(-) create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png create mode 100644 tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png create mode 100644 tests/Images/Input/Gif/issues/issue_2859_A.gif create mode 100644 tests/Images/Input/Gif/issues/issue_2859_B.gif diff --git a/src/ImageSharp/Formats/Gif/LzwDecoder.cs b/src/ImageSharp/Formats/Gif/LzwDecoder.cs index aa4327a1a0..6ad88f95cf 100644 --- a/src/ImageSharp/Formats/Gif/LzwDecoder.cs +++ b/src/ImageSharp/Formats/Gif/LzwDecoder.cs @@ -38,22 +38,22 @@ internal sealed class LzwDecoder : IDisposable /// /// The prefix buffer. /// - private readonly IMemoryOwner prefix; + private readonly IMemoryOwner prefixOwner; /// /// The suffix buffer. /// - private readonly IMemoryOwner suffix; + private readonly IMemoryOwner suffixOwner; /// /// The scratch buffer for reading data blocks. /// - private readonly IMemoryOwner scratchBuffer; + private readonly IMemoryOwner bufferOwner; /// /// The pixel stack buffer. /// - private readonly IMemoryOwner pixelStack; + private readonly IMemoryOwner pixelStackOwner; private readonly int minCodeSize; private readonly int clearCode; private readonly int endCode; @@ -80,11 +80,12 @@ internal sealed class LzwDecoder : IDisposable public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream, int minCodeSize) { this.stream = stream ?? throw new ArgumentNullException(nameof(stream)); + Guard.IsTrue(IsValidMinCodeSize(minCodeSize), nameof(minCodeSize), "Invalid minimum code size."); - this.prefix = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); - this.suffix = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); - this.pixelStack = memoryAllocator.Allocate(MaxStackSize + 1, AllocationOptions.Clean); - this.scratchBuffer = memoryAllocator.Allocate(byte.MaxValue, AllocationOptions.None); + this.prefixOwner = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); + this.suffixOwner = memoryAllocator.Allocate(MaxStackSize, AllocationOptions.Clean); + this.pixelStackOwner = memoryAllocator.Allocate(MaxStackSize + 1, AllocationOptions.Clean); + this.bufferOwner = memoryAllocator.Allocate(byte.MaxValue, AllocationOptions.None); this.minCodeSize = minCodeSize; // Calculate the clear code. The value of the clear code is 2 ^ minCodeSize @@ -94,11 +95,15 @@ public LzwDecoder(MemoryAllocator memoryAllocator, BufferedReadStream stream, in this.endCode = this.clearCode + 1; this.availableCode = this.clearCode + 2; - ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); - for (this.code = 0; this.code < this.clearCode; this.code++) + // Fill the suffix buffer with the initial values represented by the number of colors. + Span suffix = this.suffixOwner.GetSpan().Slice(0, this.clearCode); + int i; + for (i = 0; i < suffix.Length; i++) { - Unsafe.Add(ref suffixRef, this.code) = (byte)this.code; + suffix[i] = i; } + + this.code = i; } /// @@ -113,8 +118,7 @@ public static bool IsValidMinCodeSize(int 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 - int clearCode = 1 << minCodeSize; - if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize) + if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || 1 << minCodeSize > MaxStackSize) { // Don't attempt to decode the frame indices. // Theoretically we could determine a min code size from the length of the provided @@ -133,112 +137,139 @@ public void DecodePixelRow(Span indices) { indices.Clear(); - ref byte pixelsRowRef = ref MemoryMarshal.GetReference(indices); - ref int prefixRef = ref MemoryMarshal.GetReference(this.prefix.GetSpan()); - ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); - ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan()); - Span buffer = this.scratchBuffer.GetSpan(); - - int x = 0; - int xyz = 0; - while (xyz < indices.Length) + // Get span values from the owners. + Span prefix = this.prefixOwner.GetSpan(); + Span suffix = this.suffixOwner.GetSpan(); + Span pixelStack = this.pixelStackOwner.GetSpan(); + Span buffer = this.bufferOwner.GetSpan(); + + // Cache frequently accessed instance fields into locals. + // This helps avoid repeated field loads inside the tight loop. + BufferedReadStream stream = this.stream; + int top = this.top; + int bits = this.bits; + int codeSize = this.codeSize; + int codeMask = this.codeMask; + int minCodeSize = this.minCodeSize; + int availableCode = this.availableCode; + int oldCode = this.oldCode; + int first = this.first; + int data = this.data; + int count = this.count; + int bufferIndex = this.bufferIndex; + int code = this.code; + int clearCode = this.clearCode; + int endCode = this.endCode; + + int i = 0; + while (i < indices.Length) { - if (this.top == 0) + if (top == 0) { - if (this.bits < this.codeSize) + if (bits < codeSize) { // Load bytes until there are enough bits for a code. - if (this.count == 0) + if (count == 0) { // Read a new data block. - this.count = this.ReadBlock(buffer); - if (this.count == 0) + count = ReadBlock(stream, buffer); + if (count == 0) { break; } - this.bufferIndex = 0; + bufferIndex = 0; } - this.data += buffer[this.bufferIndex] << this.bits; - - this.bits += 8; - this.bufferIndex++; - this.count--; + data += buffer[bufferIndex] << bits; + bits += 8; + bufferIndex++; + count--; continue; } // Get the next code - this.code = this.data & this.codeMask; - this.data >>= this.codeSize; - this.bits -= this.codeSize; + code = data & codeMask; + data >>= codeSize; + bits -= codeSize; // Interpret the code - if (this.code > this.availableCode || this.code == this.endCode) + if (code > availableCode || code == endCode) { break; } - if (this.code == this.clearCode) + if (code == clearCode) { // Reset the decoder - this.codeSize = this.minCodeSize + 1; - this.codeMask = (1 << this.codeSize) - 1; - this.availableCode = this.clearCode + 2; - this.oldCode = NullCode; + codeSize = minCodeSize + 1; + codeMask = (1 << codeSize) - 1; + availableCode = clearCode + 2; + oldCode = NullCode; continue; } - if (this.oldCode == NullCode) + if (oldCode == NullCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.oldCode = this.code; - this.first = this.code; + pixelStack[top++] = suffix[code]; + oldCode = code; + first = code; continue; } - int inCode = this.code; - if (this.code == this.availableCode) + int inCode = code; + if (code == availableCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = (byte)this.first; - - this.code = this.oldCode; + pixelStack[top++] = first; + code = oldCode; } - while (this.code > this.clearCode) + while (code > clearCode && top < MaxStackSize) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.code = Unsafe.Add(ref prefixRef, this.code); + pixelStack[top++] = suffix[code]; + code = prefix[code]; } - int suffixCode = Unsafe.Add(ref suffixRef, this.code); - this.first = suffixCode; - Unsafe.Add(ref pixelStackRef, this.top++) = suffixCode; + int suffixCode = suffix[code]; + first = suffixCode; + pixelStack[top++] = suffixCode; - // Fix for Gifs that have "deferred clear code" as per here : + // Fix for GIFs that have "deferred clear code" as per: // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 - if (this.availableCode < MaxStackSize) + if (availableCode < MaxStackSize) { - Unsafe.Add(ref prefixRef, this.availableCode) = this.oldCode; - Unsafe.Add(ref suffixRef, this.availableCode) = this.first; - this.availableCode++; - if (this.availableCode == this.codeMask + 1 && this.availableCode < MaxStackSize) + prefix[availableCode] = oldCode; + suffix[availableCode] = first; + availableCode++; + if (availableCode == codeMask + 1 && availableCode < MaxStackSize) { - this.codeSize++; - this.codeMask = (1 << this.codeSize) - 1; + codeSize++; + codeMask = (1 << codeSize) - 1; } } - this.oldCode = inCode; + oldCode = inCode; } // Pop a pixel off the pixel stack. - this.top--; + top--; - // Clear missing pixels - xyz++; - Unsafe.Add(ref pixelsRowRef, x++) = (byte)Unsafe.Add(ref pixelStackRef, this.top); + // Clear missing pixels. + indices[i++] = (byte)pixelStack[top]; } + + // Write back the local values to the instance fields. + this.top = top; + this.bits = bits; + this.codeSize = codeSize; + this.codeMask = codeMask; + this.availableCode = availableCode; + this.oldCode = oldCode; + this.first = first; + this.data = data; + this.count = count; + this.bufferIndex = bufferIndex; + this.code = code; } /// @@ -247,130 +278,161 @@ public void DecodePixelRow(Span indices) /// The resulting index table length. public void SkipIndices(int length) { - ref int prefixRef = ref MemoryMarshal.GetReference(this.prefix.GetSpan()); - ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan()); - ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan()); - Span buffer = this.scratchBuffer.GetSpan(); - - int xyz = 0; - while (xyz < length) + // Get span values from the owners. + Span prefix = this.prefixOwner.GetSpan(); + Span suffix = this.suffixOwner.GetSpan(); + Span pixelStack = this.pixelStackOwner.GetSpan(); + Span buffer = this.bufferOwner.GetSpan(); + + // Cache frequently accessed instance fields into locals. + // This helps avoid repeated field loads inside the tight loop. + BufferedReadStream stream = this.stream; + int top = this.top; + int bits = this.bits; + int codeSize = this.codeSize; + int codeMask = this.codeMask; + int minCodeSize = this.minCodeSize; + int availableCode = this.availableCode; + int oldCode = this.oldCode; + int first = this.first; + int data = this.data; + int count = this.count; + int bufferIndex = this.bufferIndex; + int code = this.code; + int clearCode = this.clearCode; + int endCode = this.endCode; + + int i = 0; + while (i < length) { - if (this.top == 0) + if (top == 0) { - if (this.bits < this.codeSize) + if (bits < codeSize) { // Load bytes until there are enough bits for a code. - if (this.count == 0) + if (count == 0) { // Read a new data block. - this.count = this.ReadBlock(buffer); - if (this.count == 0) + count = ReadBlock(stream, buffer); + if (count == 0) { break; } - this.bufferIndex = 0; + bufferIndex = 0; } - this.data += buffer[this.bufferIndex] << this.bits; - - this.bits += 8; - this.bufferIndex++; - this.count--; + data += buffer[bufferIndex] << bits; + bits += 8; + bufferIndex++; + count--; continue; } // Get the next code - this.code = this.data & this.codeMask; - this.data >>= this.codeSize; - this.bits -= this.codeSize; + code = data & codeMask; + data >>= codeSize; + bits -= codeSize; // Interpret the code - if (this.code > this.availableCode || this.code == this.endCode) + if (code > availableCode || code == endCode) { break; } - if (this.code == this.clearCode) + if (code == clearCode) { // Reset the decoder - this.codeSize = this.minCodeSize + 1; - this.codeMask = (1 << this.codeSize) - 1; - this.availableCode = this.clearCode + 2; - this.oldCode = NullCode; + codeSize = minCodeSize + 1; + codeMask = (1 << codeSize) - 1; + availableCode = clearCode + 2; + oldCode = NullCode; continue; } - if (this.oldCode == NullCode) + if (oldCode == NullCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.oldCode = this.code; - this.first = this.code; + pixelStack[top++] = suffix[code]; + oldCode = code; + first = code; continue; } - int inCode = this.code; - if (this.code == this.availableCode) + int inCode = code; + if (code == availableCode) { - Unsafe.Add(ref pixelStackRef, this.top++) = (byte)this.first; - - this.code = this.oldCode; + pixelStack[top++] = first; + code = oldCode; } - while (this.code > this.clearCode) + while (code > clearCode && top < MaxStackSize) { - Unsafe.Add(ref pixelStackRef, this.top++) = Unsafe.Add(ref suffixRef, this.code); - this.code = Unsafe.Add(ref prefixRef, this.code); + pixelStack[top++] = suffix[code]; + code = prefix[code]; } - int suffixCode = Unsafe.Add(ref suffixRef, this.code); - this.first = suffixCode; - Unsafe.Add(ref pixelStackRef, this.top++) = suffixCode; + int suffixCode = suffix[code]; + first = suffixCode; + pixelStack[top++] = suffixCode; - // Fix for Gifs that have "deferred clear code" as per here : + // Fix for GIFs that have "deferred clear code" as per: // https://bugzilla.mozilla.org/show_bug.cgi?id=55918 - if (this.availableCode < MaxStackSize) + if (availableCode < MaxStackSize) { - Unsafe.Add(ref prefixRef, this.availableCode) = this.oldCode; - Unsafe.Add(ref suffixRef, this.availableCode) = this.first; - this.availableCode++; - if (this.availableCode == this.codeMask + 1 && this.availableCode < MaxStackSize) + prefix[availableCode] = oldCode; + suffix[availableCode] = first; + availableCode++; + if (availableCode == codeMask + 1 && availableCode < MaxStackSize) { - this.codeSize++; - this.codeMask = (1 << this.codeSize) - 1; + codeSize++; + codeMask = (1 << codeSize) - 1; } } - this.oldCode = inCode; + oldCode = inCode; } // Pop a pixel off the pixel stack. - this.top--; + top--; - // Clear missing pixels - xyz++; + // Skip missing pixels. + i++; } + + // Write back the local values to the instance fields. + this.top = top; + this.bits = bits; + this.codeSize = codeSize; + this.codeMask = codeMask; + this.availableCode = availableCode; + this.oldCode = oldCode; + this.first = first; + this.data = data; + this.count = count; + this.bufferIndex = bufferIndex; + this.code = code; } /// /// Reads the next data block from the stream. A data block begins with a byte, /// which defines the size of the block, followed by the block itself. /// + /// The stream to read from. /// The buffer to store the block in. /// /// The . /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - private int ReadBlock(Span buffer) + private static int ReadBlock(BufferedReadStream stream, Span buffer) { - int bufferSize = this.stream.ReadByte(); + int bufferSize = stream.ReadByte(); if (bufferSize < 1) { return 0; } - int count = this.stream.Read(buffer, 0, bufferSize); + int count = stream.Read(buffer, 0, bufferSize); return count != bufferSize ? 0 : bufferSize; } @@ -378,10 +440,10 @@ private int ReadBlock(Span buffer) /// public void Dispose() { - this.prefix.Dispose(); - this.suffix.Dispose(); - this.pixelStack.Dispose(); - this.scratchBuffer.Dispose(); + this.prefixOwner.Dispose(); + this.suffixOwner.Dispose(); + this.pixelStackOwner.Dispose(); + this.bufferOwner.Dispose(); } } } diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 9ecf8ad502..4e42722d2c 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -317,5 +317,17 @@ public void IssueTooLargeLzwBits(TestImageProvider provider) image.DebugSaveMultiFrame(provider); image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact); } + + // https://github.com/SixLabors/ImageSharp/issues/2859 + [Theory] + [WithFile(TestImages.Gif.Issues.Issue2859_A, PixelTypes.Rgba32)] + [WithFile(TestImages.Gif.Issues.Issue2859_B, PixelTypes.Rgba32)] + public void Issue2859_LZWPixelStackOverflow(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(); + image.DebugSaveMultiFrame(provider); + image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact); + } } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 1ba9ef2ef3..bb646800d0 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -464,7 +464,9 @@ public static class Issues 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 const string Issue2758 = "Gif/issues/issue_2758.gif"; + public const string Issue2758 = "Gif/issues/issue_2758.gif"; + public const string Issue2859_A = "Gif/issues/issue_2859_A.gif"; + public const string Issue2859_B = "Gif/issues/issue_2859_B.gif"; } public static readonly string[] All = { Rings, Giphy, Cheers, Trans, Kumin, Leo, Ratio4x1, Ratio1x4 }; diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png new file mode 100644 index 0000000000..d8c8df1263 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:daa78347749c6ff49891e2e379a373599cd35c98b453af9bf8eac52f615f935c +size 12237 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png new file mode 100644 index 0000000000..36c3683187 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:731299281f942f277ce6803e0adda3b5dd0395eb79cae26cabc9d56905fae0fd +size 1833 diff --git a/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png new file mode 100644 index 0000000000..c03e5817f0 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:50ccac7739142578d99a76b6d39ba377099d4a7ac30cbb0a5aee44ef1e7c9c8c +size 1271 diff --git a/tests/Images/Input/Gif/issues/issue_2859_A.gif b/tests/Images/Input/Gif/issues/issue_2859_A.gif new file mode 100644 index 0000000000..f19a047525 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue_2859_A.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:50a1a4afc62a3a36ff83596f1eb55d91cdd184c64e0d1339bbea17205c23eee1 +size 3406142 diff --git a/tests/Images/Input/Gif/issues/issue_2859_B.gif b/tests/Images/Input/Gif/issues/issue_2859_B.gif new file mode 100644 index 0000000000..109b0f8797 --- /dev/null +++ b/tests/Images/Input/Gif/issues/issue_2859_B.gif @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:db9b2992be772a4f0ac495e994a17c7c50fb6de9794cfb9afc4a3dc26ffdfae0 +size 4543 From f7b05d7134944492d58351eb055b4ef16654cbc6 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 20:25:23 +0100 Subject: [PATCH 05/18] uses: actions/upload-artifact@v4 --- .github/workflows/build-and-test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 5b3d294bc5..541ad3a623 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -151,7 +151,7 @@ jobs: XUNIT_PATH: .\tests\ImageSharp.Tests # Required for xunit - name: Export Failed Output - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 if: failure() with: name: actual_output_${{ runner.os }}_${{ matrix.options.framework }}${{ matrix.options.runtime }}.zip From 3346285165ede778f0f1ce5a8dce71e2129882e0 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 20:36:35 +0100 Subject: [PATCH 06/18] install prerequisites on ubuntu --- .github/workflows/build-and-test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 541ad3a623..a5b7f09afc 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -165,6 +165,9 @@ jobs: if: (github.event_name == 'push') steps: + - name: Install ubuntu prerequisites + if: ${{ contains(matrix.options.os, 'ubuntu') }} + run: sudo apt-get -y install libssl-dev libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev - name: Git Config shell: bash run: | From 01cbfc4090ca9915c52fe6606c54b2937637bd97 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 20:42:40 +0100 Subject: [PATCH 07/18] move the step to the right place --- .github/workflows/build-and-test.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index a5b7f09afc..f6c071d012 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -74,6 +74,10 @@ jobs: runs-on: ${{matrix.options.os}} steps: + - name: Install Ubuntu prerequisites + if: ${{ contains(matrix.options.os, 'ubuntu') }} + run: sudo apt-get -y install libssl-dev libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev + - name: Git Config shell: bash run: | @@ -165,9 +169,6 @@ jobs: if: (github.event_name == 'push') steps: - - name: Install ubuntu prerequisites - if: ${{ contains(matrix.options.os, 'ubuntu') }} - run: sudo apt-get -y install libssl-dev libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev - name: Git Config shell: bash run: | From cbb309453692b3289b8af95cfa0ccfa73efe1db2 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 20:56:17 +0100 Subject: [PATCH 08/18] yet another attempt to install libssl 1.x --- .github/workflows/build-and-test.yml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index f6c071d012..7abbd5c035 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -76,7 +76,18 @@ jobs: steps: - name: Install Ubuntu prerequisites if: ${{ contains(matrix.options.os, 'ubuntu') }} - run: sudo apt-get -y install libssl-dev libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev + run: | + # libssl 1.1 + wget http://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.16_amd64.deb + wget http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.16_amd64.deb + wget http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.16_amd64.deb + + sudo dpkg -i libssl1.1_1.1.1f-1ubuntu2.16_amd64.deb + sudo dpkg -i libssl-dev_1.1.1f-1ubuntu2.16_amd64.deb + sudo dpkg -i openssl_1.1.1f-1ubuntu2.16_amd64.deb + + # libgdiplus + sudo apt-get -y install libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev - name: Git Config shell: bash From 540374905a4204d6808e2a9d782c8d69445fdb58 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 20 Feb 2025 21:00:26 +0100 Subject: [PATCH 09/18] I'm the worst linux sysadmin ever --- .github/workflows/build-and-test.yml | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 7abbd5c035..7ccf2fc8ec 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -77,14 +77,9 @@ jobs: - name: Install Ubuntu prerequisites if: ${{ contains(matrix.options.os, 'ubuntu') }} run: | - # libssl 1.1 - wget http://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.16_amd64.deb - wget http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.16_amd64.deb - wget http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.16_amd64.deb - - sudo dpkg -i libssl1.1_1.1.1f-1ubuntu2.16_amd64.deb - sudo dpkg -i libssl-dev_1.1.1f-1ubuntu2.16_amd64.deb - sudo dpkg -i openssl_1.1.1f-1ubuntu2.16_amd64.deb + # libssl 1.1 (required by old .NET runtimes) + wget http://archive.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.0g-2ubuntu4_amd64.deb + sudo dpkg -i libssl1.1_1.1.0g-2ubuntu4_amd64.deb # libgdiplus sudo apt-get -y install libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev From e9e4688d387063cefc123e4138a93959e1201165 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 21 Feb 2025 19:41:39 +0100 Subject: [PATCH 10/18] setup .NET 8.0.x --- .github/workflows/build-and-test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 7ccf2fc8ec..5fc1a46b26 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -125,6 +125,7 @@ jobs: uses: actions/setup-dotnet@v1 with: dotnet-version: | + 8.0.x 6.0.x 5.0.x 3.1.x From efc3fc4ee15eec4e523c26f7130e786541b00df2 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 21 Feb 2025 20:53:57 +0100 Subject: [PATCH 11/18] Disable BmpDecoder_CanDecode_Os2BitmapArray on Linux --- tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs index 75a9800844..64307a4156 100644 --- a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs @@ -599,6 +599,7 @@ public void BmpDecoder_CanDecode_Os2v2Header(TestImageProvider p } [Theory] + [PlatformSpecific(~TestPlatforms.Linux)] // See discussion on https://github.com/SixLabors/ImageSharp/pull/2890 [WithFile(Os2BitmapArray, PixelTypes.Rgba32)] [WithFile(Os2BitmapArray9s, PixelTypes.Rgba32)] [WithFile(Os2BitmapArrayDiamond, PixelTypes.Rgba32)] From adb85d9e66aa3a588a86f4a4ef9a0539a8502117 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 21 Feb 2025 21:06:23 +0100 Subject: [PATCH 12/18] Another attempt for a Linux-specific skip --- tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs index 64307a4156..e5c7c70577 100644 --- a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs @@ -4,7 +4,7 @@ using System; using System.IO; using Microsoft.DotNet.RemoteExecutor; - +using Microsoft.DotNet.XUnitExtensions; using SixLabors.ImageSharp.Formats.Bmp; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.Metadata; @@ -598,8 +598,7 @@ public void BmpDecoder_CanDecode_Os2v2Header(TestImageProvider p } } - [Theory] - [PlatformSpecific(~TestPlatforms.Linux)] // See discussion on https://github.com/SixLabors/ImageSharp/pull/2890 + [ConditionalTheory] [WithFile(Os2BitmapArray, PixelTypes.Rgba32)] [WithFile(Os2BitmapArray9s, PixelTypes.Rgba32)] [WithFile(Os2BitmapArrayDiamond, PixelTypes.Rgba32)] @@ -612,6 +611,11 @@ public void BmpDecoder_CanDecode_Os2v2Header(TestImageProvider p public void BmpDecoder_CanDecode_Os2BitmapArray(TestImageProvider provider) where TPixel : unmanaged, IPixel { + if (TestEnvironment.IsLinux) + { + throw new SkipTestException("See discussion on https://github.com/SixLabors/ImageSharp/pull/2890"); + } + using (Image image = provider.GetImage(BmpDecoder)) { image.DebugSave(provider); From 44d294e06606111195152ead3006452357ef1bb9 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 21 Feb 2025 21:27:07 +0100 Subject: [PATCH 13/18] 8.0.x is not needed --- .github/workflows/build-and-test.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 5fc1a46b26..7ccf2fc8ec 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -125,7 +125,6 @@ jobs: uses: actions/setup-dotnet@v1 with: dotnet-version: | - 8.0.x 6.0.x 5.0.x 3.1.x From 67f7848d6e975e7956c8056823555de49a5fdf6d Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 21 Feb 2025 21:39:24 +0100 Subject: [PATCH 14/18] try to fix LFS for *.BMP --- .gitattributes | 1 + tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs | 7 +------ 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/.gitattributes b/.gitattributes index 355b64dce1..f97695f901 100644 --- a/.gitattributes +++ b/.gitattributes @@ -115,6 +115,7 @@ *.jpg filter=lfs diff=lfs merge=lfs -text *.jpeg filter=lfs diff=lfs merge=lfs -text *.bmp filter=lfs diff=lfs merge=lfs -text +*.BMP filter=lfs diff=lfs merge=lfs -text *.gif filter=lfs diff=lfs merge=lfs -text *.png filter=lfs diff=lfs merge=lfs -text *.tif filter=lfs diff=lfs merge=lfs -text diff --git a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs index e5c7c70577..acc4c201b7 100644 --- a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs @@ -598,7 +598,7 @@ public void BmpDecoder_CanDecode_Os2v2Header(TestImageProvider p } } - [ConditionalTheory] + [Theory] [WithFile(Os2BitmapArray, PixelTypes.Rgba32)] [WithFile(Os2BitmapArray9s, PixelTypes.Rgba32)] [WithFile(Os2BitmapArrayDiamond, PixelTypes.Rgba32)] @@ -611,11 +611,6 @@ public void BmpDecoder_CanDecode_Os2v2Header(TestImageProvider p public void BmpDecoder_CanDecode_Os2BitmapArray(TestImageProvider provider) where TPixel : unmanaged, IPixel { - if (TestEnvironment.IsLinux) - { - throw new SkipTestException("See discussion on https://github.com/SixLabors/ImageSharp/pull/2890"); - } - using (Image image = provider.GetImage(BmpDecoder)) { image.DebugSave(provider); From bb82f79db0197166271d4355b5fb5ceda370a906 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Mon, 24 Feb 2025 18:18:54 +0100 Subject: [PATCH 15/18] #2701 to 2.1.x [copy] --- .../Components/Decoder/HuffmanScanBuffer.cs | 17 +++++++++++++---- .../Formats/Jpg/JpegDecoderTests.cs | 13 +++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 3 ++- tests/Images/Input/Jpg/issues/Issue2638.jpg | 3 +++ 4 files changed, 31 insertions(+), 5 deletions(-) create mode 100644 tests/Images/Input/Jpg/issues/Issue2638.jpg diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs index 39e606fd39..c6fe34e22e 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs @@ -22,6 +22,9 @@ internal struct HuffmanScanBuffer // Whether there is no more good data to pull from the stream for the current mcu. private bool badData; + // How many times have we hit the eof. + private int eofHitCount; + public HuffmanScanBuffer(BufferedReadStream stream) { this.stream = stream; @@ -31,6 +34,7 @@ public HuffmanScanBuffer(BufferedReadStream stream) this.MarkerPosition = 0; this.badData = false; this.NoData = false; + this.eofHitCount = 0; } /// @@ -218,11 +222,16 @@ private int ReadStream() // we know we have hit the EOI and completed decoding the scan buffer. if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) { - // We've encountered the end of the file stream which means there's no EOI marker + // We've hit the end of the file stream more times than allowed which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. - this.badData = true; - this.NoData = true; - value = 0; + if (this.eofHitCount > JpegConstants.Huffman.FetchLoop) + { + this.badData = true; + this.NoData = true; + value = 0; + } + + this.eofHitCount++; } return value; diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index 708e859eed..284e4fa254 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -229,6 +229,19 @@ public void Issue2133_DeduceColorSpace(TestImageProvider provide } } + // https://github.com/SixLabors/ImageSharp/issues/2638 + [Theory] + [WithFile(TestImages.Jpeg.Issues.Issue2638, PixelTypes.Rgba32)] + public void Issue2638_DecodeWorks(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using (Image image = provider.GetImage(JpegDecoder)) + { + image.DebugSave(provider); + image.CompareToOriginal(provider); + } + } + // DEBUG ONLY! // The PDF.js output should be saved by "tests\ImageSharp.Tests\Formats\Jpg\pdfjs\jpeg-converter.htm" // into "\tests\Images\ActualOutput\JpegDecoderTests\" diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index bb646800d0..9ee63e5636 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -269,7 +269,8 @@ public static class Issues public const string ValidExifArgumentNullExceptionOnEncode = "Jpg/issues/Issue2087-exif-null-reference-on-encode.jpg"; public const string Issue2133DeduceColorSpace = "Jpg/issues/Issue2133.jpg"; public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg"; - public const string Issue2758 = "Jpg/issues/issue-2758.jpg"; + public const string Issue2758 = "Jpg/issues/issue-2758.jpg"; + public const string Issue2638 = "Jpg/issues/Issue2638.jpg"; public static class Fuzz { diff --git a/tests/Images/Input/Jpg/issues/Issue2638.jpg b/tests/Images/Input/Jpg/issues/Issue2638.jpg new file mode 100644 index 0000000000..f42d67b0e8 --- /dev/null +++ b/tests/Images/Input/Jpg/issues/Issue2638.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:208d5b0b727bbef120a7e090e020a48f99c9e264c2d3939ba749f8620853c1fe +size 70876 From 4d3a85112b03c89d2cb8616a5b747684b6e73730 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 6 Mar 2025 17:02:17 +1000 Subject: [PATCH 16/18] Use latest cache action --- .github/workflows/build-and-test.yml | 2 +- .github/workflows/code-coverage.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 7ccf2fc8ec..357b919930 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -191,7 +191,7 @@ jobs: uses: NuGet/setup-nuget@v1 - name: NuGet Setup Cache - uses: actions/cache@v2 + uses: actions/cache@v4 id: nuget-cache with: path: ~/.nuget diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index 2b14f2a4b7..bb3d8c6e84 100644 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -47,7 +47,7 @@ jobs: uses: NuGet/setup-nuget@v1 - name: NuGet Setup Cache - uses: actions/cache@v2 + uses: actions/cache@v4 id: nuget-cache with: path: ~/.nuget From 5dfe5a800367581239de442cc18de659da6e9b1d Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 6 Mar 2025 17:06:53 +1000 Subject: [PATCH 17/18] Missed cache action update --- .github/workflows/build-and-test.yml | 4 ++-- .github/workflows/code-coverage.yml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 357b919930..45769c2163 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -101,7 +101,7 @@ jobs: run: git lfs ls-files -l | cut -d' ' -f1 | sort > .lfs-assets-id - name: Git Setup LFS Cache - uses: actions/cache@v2 + uses: actions/cache@v4 id: lfs-cache with: path: .git/lfs @@ -114,7 +114,7 @@ jobs: uses: NuGet/setup-nuget@v1 - name: NuGet Setup Cache - uses: actions/cache@v2 + uses: actions/cache@v4 id: nuget-cache with: path: ~/.nuget diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index bb3d8c6e84..45b856a15c 100644 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -34,7 +34,7 @@ jobs: run: git lfs ls-files -l | cut -d' ' -f1 | sort > .lfs-assets-id - name: Git Setup LFS Cache - uses: actions/cache@v2 + uses: actions/cache@v4 id: lfs-cache with: path: .git/lfs From d133ef99e8becfc3b924b0bb4315e63b8681d307 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Thu, 6 Mar 2025 17:14:27 +1000 Subject: [PATCH 18/18] Set lang version --- Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index 26b3cc5afc..b46c0f19f2 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -23,7 +23,7 @@ - preview + 10.0