Prevent zigzag overflow. Fix #922 #924
Conversation
Codecov Report
@@ Coverage Diff @@
## master #924 +/- ##
==========================================
- Coverage 89.26% 89.14% -0.12%
==========================================
Files 1076 1077 +1
Lines 47209 47218 +9
Branches 3315 3319 +4
==========================================
- Hits 42140 42093 -47
- Misses 4366 4371 +5
- Partials 703 754 +51
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #924 +/- ##
==========================================
+ Coverage 89.26% 89.26% +<.01%
==========================================
Files 1076 1077 +1
Lines 47209 47230 +21
Branches 3315 3320 +5
==========================================
+ Hits 42140 42159 +19
- Misses 4366 4367 +1
- Partials 703 704 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Although #922 is not being reproducible with the runtimes currently covered by CI, and we have unit test in this PR, I strongly recommend to introduce an "integration" test, based on an image referenced in that issue. I asked whether it's reproducible with a smaller one.
Image-based regression tests are less prone to large-scale refactors, and an extensive set of images is a robust, implementation-agnostic specification for a modern production-quality decoder. (Especially if they come with issue references serving as explanatory docs.)
We've gotta collect 'em all!
| @@ -68,7 +75,7 @@ public static ZigZag CreateUnzigTable() | |||
| { | |||
| ZigZag result = default; | |||
| byte* unzigPtr = result.Data; | |||
| Marshal.Copy(Unzig, 0, (IntPtr)unzigPtr, 64); | |||
| Marshal.Copy(Unzig, 0, (IntPtr)unzigPtr, 64 + 16); | |||
There was a problem hiding this comment.
Might make sense to define a constant for 64+16 or use Unzig.Length.
|
I wasn’t sure about adding the image as it only exposes the issue and didn’t, to me seem important since we can test without inflating the project size. We do render it differently to libjpeg though (I don’t know why) so maybe we should add it yeah. |
|
@antonfirsov I've added the image but commented it out of the decoder tests. We don't produce the same output as libjpeg and I don't want to spend the time attempting to discover why for a corrupted image. |
There was a problem hiding this comment.
Cool, I can do the rest, in another PR.
I think we can address most repo size-related issues by implementing #868 and switching to Azure Pipelines.
Prevent zigzag overflow. Fix SixLabors#922
Prerequisites
Description
Prevents potential overflow from corrupt jpegs. This is how libjpeg handles it.