GIF: Check for end of stream when reading comments. #2954
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an explicit end-of-stream check when reading GIF comment sub-blocks to prevent infinite loops or silent failures, and covers the change with a new test case for issue #2953.
- Insert EOF check in
ReadCommentsto throwInvalidImageContentExceptionon truncated GIF comments - Add a new LFS pointer test asset and register it in
TestImages - Add
GifDecoderTests.Issue2953to verifyIdentifyandLoadboth throw on invalid GIF
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/Images/Input/Gif/issues/issue_2953.gif | Added new test asset (LFS pointer placeholder) |
| tests/ImageSharp.Tests/TestImages.cs | Registered Issue2953 constant for the new test image |
| tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs | Added Issue2953 test to assert InvalidImageContentException |
| src/ImageSharp/Formats/Gif/GifDecoderCore.cs | Inserted EOF (length == -1) check in ReadComments to catch streams ending prematurely |
Comments suppressed due to low confidence (1)
tests/Images/Input/Gif/issues/issue_2953.gif:1
- This is an LFS pointer file rather than a real or truncated GIF; it may not actually trigger the new EOF branch. Consider replacing it with a minimal or truncated GIF file to exercise the comment-reading EOF check in tests.
version https://git-lfs.github.com/spec/v1
| @@ -414,6 +414,11 @@ private void ReadComments(BufferedReadStream stream) | |||
| GifThrowHelper.ThrowInvalidImageContentException($"Gif comment length '{length}' exceeds max '{GifConstants.MaxCommentSubBlockLength}' of a comment data block"); | |||
| } | |||
|
|
|||
| if (length == -1) | |||
There was a problem hiding this comment.
[nitpick] Consider moving the EOF check (length < 0) before the max-length validation to follow a consistent ordering of bounds checks and avoid any accidental fall-through.
Copilot uses AI. Check for mistakes.
| @@ -414,6 +414,11 @@ private void ReadComments(BufferedReadStream stream) | |||
| GifThrowHelper.ThrowInvalidImageContentException($"Gif comment length '{length}' exceeds max '{GifConstants.MaxCommentSubBlockLength}' of a comment data block"); | |||
| } | |||
|
|
|||
| if (length == -1) | |||
| { | |||
| GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading gif comment"); | |||
There was a problem hiding this comment.
[nitpick] The exception message uses lowercase 'gif'; for consistency with other GIF-related messages in the codebase, consider capitalizing it to 'GIF'.
| GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading gif comment"); | |
| GifThrowHelper.ThrowInvalidImageContentException("Unexpected end of stream while reading GIF comment"); |
Copilot uses AI. Check for mistakes.
| public void Issue2953<TPixel>(TestImageProvider<TPixel> provider) | ||
| where TPixel : unmanaged, IPixel<TPixel> | ||
| { | ||
| // We should throw a InvalidImageContentException when trying to identify or load an invalid GIF file. |
There was a problem hiding this comment.
Replace 'a InvalidImageContentException' with 'an InvalidImageContentException' to correct the article usage.
| // We should throw a InvalidImageContentException when trying to identify or load an invalid GIF file. | |
| // We should throw an InvalidImageContentException when trying to identify or load an invalid GIF file. |
Copilot uses AI. Check for mistakes.
Prerequisites
Description
Fixes #2953