Skip to content
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.

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

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

brianpopow
Copy link
Collaborator

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • 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.

@@ -73,7 +78,7 @@ public void DecodePixels(int minCodeSize, Buffer2D<byte> pixels)
// 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 || clearCode > MaxStackSize)
if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

To save a comparison:

Suggested change
if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize)
if ((uint)minCodeSize - 2 > MaximumLzwBits - 2 || clearCode > MaxStackSize)

(due the Unsafe.Add below I guess this method is hot enough...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the suggestions @gfoidl, but I do not see any difference when I execute gif decoder benchmark.

I do not want to sacrifice the readability here, if it does not really improve the performance.

@@ -245,7 +250,7 @@ public void SkipIndices(int minCodeSize, int length)
// 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 || clearCode > MaxStackSize)
if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (minCodeSize < 2 || minCodeSize > MaximumLzwBits || clearCode > MaxStackSize)
if ((uint)minCodeSize - 2 > MaximumLzwBits - 2 || clearCode > MaxStackSize)

or move this check into a helper, than it's more self-documenting what's going on.

@brianpopow
Copy link
Collaborator Author

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
Copy link
Member

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

@brianpopow
Copy link
Collaborator Author

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
Copy link
Member

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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?

@JimBobSquarePants
Copy link
Member

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.

@JimBobSquarePants JimBobSquarePants merged commit ede2f2d into main Jun 7, 2024
5 checks passed
@JimBobSquarePants JimBobSquarePants deleted the bp/issue2743 branch June 7, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants