V3: Gracefully handle LZW overflows #2880
Conversation
JimBobSquarePants
requested review from
tocsoft,
antonfirsov,
dlemstra,
brianpopow and
Copilot
There was a problem hiding this comment.
Copilot reviewed 3 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (5)
- tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_A.gif/00.png: Language not supported
- tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/00.png: Language not supported
- tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2859_LZWPixelStackOverflow_Rgba32_issue_2859_B.gif/01.png: Language not supported
- tests/Images/Input/Gif/issues/issue_2859_A.gif: Language not supported
- tests/Images/Input/Gif/issues/issue_2859_B.gif: Language not supported
| @@ -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) | |||
There was a problem hiding this comment.
Since we are here, can we also un-Unsafe the methods touched?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 |
Prerequisites
Description
The issue is caused by using the full capacity of the pixel stack for intermediate pushes without reserving a slot for the final push, which leads to an overflow. By capping out the max index we can skip corrupt blocks.
Fixes #2859