Skip to content

V3: Gracefully handle LZW overflows #2880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JimBobSquarePants merged 5 commits into from
Feb 10, 2025
Merged

V3: Gracefully handle LZW overflows #2880

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b99bf32
Gracefully handle LZW overflows
JimBobSquarePants Feb 4, 2025
1709797
Add reference output
JimBobSquarePants Feb 4, 2025
9cb022b
Update build-and-test.yml
JimBobSquarePants Feb 4, 2025
ca07754
Use safe code.
JimBobSquarePants Feb 5, 2025
20e5596
Clean up and optimize
JimBobSquarePants Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/ImageSharp/Formats/Gif/LzwDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ public void DecodePixelRow(Span<byte> indices)
ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan());
ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan());
Span<byte> buffer = this.scratchBuffer.GetSpan();
int maxTop = this.pixelStack.Length() - 1;

int x = 0;
int xyz = 0;
Expand Down Expand Up @@ -204,7 +205,7 @@ public void DecodePixelRow(Span<byte> indices)
this.code = this.oldCode;
}

while (this.code > this.clearCode)
while (this.code > this.clearCode && this.top < maxTop)

@antonfirsov antonfirsov Feb 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are here, can we also un-Unsafe the methods touched?

@JimBobSquarePants JimBobSquarePants Feb 4, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we can get the same performance out. We're hitting two separate spans inside a loop which is will definitely incur bounds checks costs.

@antonfirsov antonfirsov Feb 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can benchmark that. IMO it was a big mistake to start using Unsafe as default way of indexing things, since it opens us up to security risks and it should be our goal to significantly reduce the amount of Unsafe usage across the codebase. A gain of few percents is not worth the risk, it's better to find algorithmic optimizations.

@JimBobSquarePants JimBobSquarePants Feb 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov I've updated the LzwDecoder with the best I could do performance-wise using safe code.

I'm OK with the performance hit given the much-improved security. Given that System.Drawing doesn't actually fully decode the image I'm still happy with the the performance of the decoder.

Target Branch

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.26100
11th Gen Intel Core i7-11370H 3.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=9.0.200-preview.0.25057.12
  [Host]     : .NET 7.0.20 (7.0.2024.26716), X64 RyuJIT
  Job-AUXYJW : .NET 6.0.36 (6.0.3624.51421), X64 RyuJIT

Runtime=.NET 6.0  Arguments=/p:DebugType=portable  IterationCount=3
LaunchCount=1  WarmupCount=3
Method TestImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing Gif' Gif/rings.gif 294.7 us 100.98 us 5.53 us 1.00 0.00 - - - 168 B
'ImageSharp Gif' Gif/rings.gif 410.6 us 87.95 us 4.82 us 1.39 0.03 0.9766 - - 7,557 B

This PR

Method TestImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing Gif' Gif/rings.gif 299.0 us 189.8 us 10.41 us 1.00 0.00 - - - 168 B
'ImageSharp Gif' Gif/rings.gif 486.9 us 153.3 us 8.40 us 1.63 0.06 0.9766 - - 7,715 B

{
Unsafe.Add(ref pixelStackRef, (uint)this.top++) = Unsafe.Add(ref suffixRef, (uint)this.code);
this.code = Unsafe.Add(ref prefixRef, (uint)this.code);
Expand Down Expand Up @@ -250,6 +251,7 @@ public void SkipIndices(int length)
ref int suffixRef = ref MemoryMarshal.GetReference(this.suffix.GetSpan());
ref int pixelStackRef = ref MemoryMarshal.GetReference(this.pixelStack.GetSpan());
Span<byte> buffer = this.scratchBuffer.GetSpan();
int maxTop = this.pixelStack.Length() - 1;

int xyz = 0;
while (xyz < length)
Expand Down Expand Up @@ -316,7 +318,7 @@ public void SkipIndices(int length)
this.code = this.oldCode;
}

while (this.code > this.clearCode)
while (this.code > this.clearCode && this.top < maxTop)
{
Unsafe.Add(ref pixelStackRef, (uint)this.top++) = Unsafe.Add(ref suffixRef, (uint)this.code);
this.code = Unsafe.Add(ref prefixRef, (uint)this.code);
Expand Down
12 changes: 12 additions & 0 deletions tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,4 +334,16 @@ public void IssueTooLargeLzwBits<TPixel>(TestImageProvider<TPixel> 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<TPixel>(TestImageProvider<TPixel> provider)
where TPixel : unmanaged, IPixel<TPixel>
{
using Image<TPixel> image = provider.GetImage();
image.DebugSaveMultiFrame(provider);
image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact);
}
}
2 changes: 2 additions & 0 deletions tests/ImageSharp.Tests/TestImages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,8 @@ public static class Issues
public const string Issue2450_B = "Gif/issues/issue_2450_2.gif";
public const string Issue2198 = "Gif/issues/issue_2198.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[] Animated =
Expand Down
3 changes: 3 additions & 0 deletions .../GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions .../GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions .../GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions tests/Images/Input/Gif/issues/issue_2859_A.gif
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions tests/Images/Input/Gif/issues/issue_2859_B.gif
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.