GifDecoder: Limit lzw bits to a maximum of 12 bits

SixLabors / ImageSharp

:camera: A modern, cross-platform, 2D Graphics library for .NET

https://sixlabors.com/products/imagesharp/

Other

7.26k stars 844 forks source link

GifDecoder: Limit lzw bits to a maximum of 12 bits #2744

Closed brianpopow closed 1 month ago

brianpopow commented 1 month ago

Prerequisites

[x] I have written a descriptive pull-request title
[x] I have verified that there are no overlapping pull-requests open
[x] I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules :cop:.
[x] I have provided test coverage for my change (where applicable)

Description

The image provided in #2743 has some invalid lzwCode length. This PR changes the gif lzw decoder to raise an exception when the lzwCode is larger then 12 Bits.

brianpopow commented 1 month ago

There are some undisposed buffers when throwing the exception. I have trouble figuring out what buffers are not disposed here.

[xUnit.net 00:00:01.10]     IssueTooLargeLzwBits<Rgba32>(provider: Gif/issues/issue_2743.gif[Rgba32]) [FAIL]
  Fehler IssueTooLargeLzwBits<Rgba32>(provider: Gif/issues/issue_2743.gif[Rgba32]) [2 ms]
  Fehlermeldung:
   Expected 0 undisposed buffers but found 7

JimBobSquarePants commented 1 month ago

Shouldn't we be clamping rather than throwing? The provided image can be opened in the browser.

brianpopow commented 1 month ago

Shouldn't we be clamping rather than throwing?

The specification says 12 bits is the maximum code length, see spec-gif89a.txt in the gif folder. Appendix F, Compression, section 4:

4. The output codes are of variable length, starting at <code size>+1 bits per
code, up to 12 bits per code. This defines a maximum code value of 4095
(0xFFF). Whenever the LZW code value would exceed the current code length, the
code length is increased by one. The packing/unpacking of these codes must then
be altered to reflect the new code length.

ImageMagick raises an error with this image, so I thought we should, too. I am not sure what will happen when we clamp.

I am more in favor of throwing an exception, because > 12 bits is against the spec.

The provided image can be opened in the browser.

The first 7 images are fine, the issue happens with the 8th image.

JimBobSquarePants commented 1 month ago

@brianpopow The best approach here is to actually adopt our standard of attempting to decode as much as possible. This way we don't throw we simply return and preserve the data for all the previous frames.

I don't know why I chose to throw for #2012 but that was a bad choice.

brianpopow commented 1 month ago

@brianpopow The best approach here is to actually adopt our standard of attempting to decode as much as possible. This way we don't throw we simply return and preserve the data for all the previous frames.

Ok, makes sense. I think this then ready for a final review.

brianpopow commented 1 month ago

I don't know why I chose to throw for https://github.com/SixLabors/ImageSharp/issues/2012 but that was a bad choice.

@JimBobSquarePants should we change that here to also return instead of throwing a exception?

JimBobSquarePants commented 1 month ago

I don't know why I chose to throw for #2012 but that was a bad choice.

@JimBobSquarePants should we change that here to also return instead of throwing a exception?

I've already made the change. It was the same conditional check in the LZWDecoder.