Tolerate GIFs missing the final trailer byte (0x3B)#237
Open
lilith wants to merge 1 commit into
Open
Conversation
kornelski
requested changes
May 31, 2026
| /// are partway through a structure (image data, a sub-block, a palette, | ||
| /// an extension, …) and an EOF there is genuine truncation. | ||
| pub(crate) fn is_at_block_boundary(&self) -> bool { | ||
| matches!(self.state, BlockStart(_) | BlockEnd) |
Contributor
There was a problem hiding this comment.
BlockStart means a block already started parsing, which is a bug here
lilith
added a commit
to lilith/image-gif
that referenced
this pull request
Jun 3, 2026
Addresses kornelski's review on PR image-rs#237: `is_at_block_boundary` matched `BlockStart(_) | BlockEnd`, but `BlockStart(type_)` is reached only after the next block introducer byte was already read in `BlockEnd` — a block has started parsing and its body is now expected. EOF there is a truncated block, not a missing trailer. The `any_frame_decoded` guard masked the hole for the header-only case, but a stream with a complete first frame followed by a bare introducer byte and then EOF was silently accepted with the truncated trailing block dropped (decode returned only frame 1, no error). Restrict the boundary to `BlockEnd` (the post-frame / post-extension resting state where the introducer has not yet been read). Add a regression test that fails on the old condition: a complete frame + `0x2C` introducer + EOF must surface UnexpectedEof, not silently truncate.
Many real-world GIFs omit the GIF trailer block (0x3B). Browsers, ImageMagick, and PIL all decode them, but previously the decoder hit EOF while looking for the next block introducer and surfaced UnexpectedEof, rejecting a fully decodable GIF as corrupt. This relaxes only the clean-boundary case: when at least one frame has been fully decoded and the input ends exactly in the `BlockEnd` state (a complete frame finished, the next block introducer not yet read), a trailing EOF is reported as a clean end-of-stream. Only `BlockEnd` qualifies — `BlockStart(_)` means the next introducer byte was already read, so a block has started and its body is owed; an EOF there is genuine truncation. EOF mid-frame, after a bare introducer, or before any frame still errors. Adds tests covering the positive case and the three truncation cases (mid-frame, introducer-then-EOF after a frame, and zero-frame).
e8b5c67 to
2db69fd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A surprising number of real-world GIFs omit the final trailer block (
0x3B). NB: This crate, if the encoder is not dropped before accessing the stream, also fails to emit the final trailer block. This is a consumer usage error but a light footgun I've personally run into. Browsers, ImageMagick, and PIL all decode them without complaint. Currently, after decoding every frame, the decoder hits EOF while looking for the next block introducer and surfacesUnexpectedEof— so a strict consumer (e.g. theimagecrate's collect path) rejects a fully-decodable GIF as corrupt.Concrete repro
A real 67-frame GIF in the wild (example, 1.97 MB, 260×342) ends with
0x00(the block terminator of its last image sub-block) and has no0x3Btrailer.gif0.14.2 and currentmaster: decode all 67 frames correctly (RGBA buffers match ImageMagick / PIL), then returnErr(UnexpectedEof).Fix
This relaxes only the clean-boundary case: when the decoder is between blocks — the previous frame fully completed and the next byte it expects is a block introducer/trailer — and at least one frame has been decoded, a trailing EOF is reported as a clean end-of-stream (
Ok(None)) instead of an error.Concretely, in
ReadDecoder::decode_next, whenfill_buf()returns empty:is_at_block_boundary()is true only for theBlockStart(_)/BlockEndstates (waiting for the next block introducer).any_frame_decodedis set when a frame'sDecoded::DataEndis observed.What still errors (zero-trust boundary)
UnexpectedEof/Truncated.This extends the same "accept non-terminated streams by cutting off when all bytes are decoded" philosophy noted in #235 to the missing-end-trailer case. It mirrors a guard we ship downstream.
Tests (
tests/missing_trailer.rs)0x3Btrailer stripped decodes all frames cleanly, pixel-identical to the with-trailer decode.The full existing suite passes unchanged (trailer-present, truncation, stall/byte-at-a-time streaming).
Notes
is_at_block_boundaryispub(crate);any_frame_decodedis a private field).cargo fmtclean;clippyclean on the touched files.