fix: yield_on_full correctness on wuffs branch (#68)#72
Merged
197g merged 1 commit intoimage-rs:wuffsfrom Apr 14, 2026
Merged
Conversation
Three fixes for the wuffs burst decoder: 1. Unconditional buffer drain (image-rs#68): the buffer drain at the start of advance() was gated on code_link.is_some(). After a clear code sets code_link=None, buffered data was lost. Also caused false NoProgress with buf=1 when the buffer had undrained data. 2. debug_assert fix: the non-broken-burst path asserted next_code <= size_switch_at, which is wrong when the table is full in TIFF mode (next_code=4096 > size_switch_at=4094). 3. reconstruct_simple overwrite guard: require 8 bytes of slack past the code length before using the relaxed 8-byte-chunk overwrite, preventing data corruption with tight buffers. All three regression tests pass, including buf=1. No perf impact — the fix is in the per-call buffer drain, not in the burst hot loop.
lilith
force-pushed
the
wuffs-yield-on-full-fix
branch
from
54e9f3c to
5460037
Compare
This was referenced Apr 13, 2026
lilith
added a commit
to lilith/image-gif
that referenced
this pull request
Apr 13, 2026
Three valid interlaced GIFs (7x7, 9x9, 47x63) produced by gifsicle 1.95 are rejected with "image truncated". All decode correctly with giftopnm (netpbm) and are valid per the GIF89a spec. Root cause: when decoding interlaced frames, read_into_buffer passes per-row slices to the LZW decoder. For small images (7-47 byte rows), weezl's NoProgress status triggers two bugs in DecodeSubBlock: 1. Input data is skipped instead of retried with a smaller buffer 2. NoProgress during flush is treated as end-of-stream Related: image-rs/weezl#72
lilith
added a commit
to lilith/image-gif
that referenced
this pull request
Apr 13, 2026
Three valid interlaced GIFs (7x7, 9x9, 47x63) produced by gifsicle 1.95 are rejected with "image truncated". All decode correctly with giftopnm (netpbm) and are valid per the GIF89a spec. Root cause: when decoding interlaced frames, read_into_buffer passes per-row slices to the LZW decoder. For small images (7-47 byte rows), weezl's NoProgress status triggers two bugs in DecodeSubBlock: 1. Input data is skipped instead of retried with a smaller buffer 2. NoProgress during flush is treated as end-of-stream Related: image-rs/weezl#72
lilith
added a commit
to lilith/image-gif
that referenced
this pull request
Apr 13, 2026
Three valid interlaced GIFs (7x7, 9x9, 47x63) produced by gifsicle 1.95 are rejected with "image truncated". All decode correctly with giftopnm (netpbm) and are valid per the GIF89a spec. Root cause: when decoding interlaced frames, read_into_buffer passes per-row slices to the LZW decoder. For small images (7-47 byte rows), weezl's NoProgress status triggers two bugs in DecodeSubBlock: 1. Input data is skipped instead of retried with a smaller buffer 2. NoProgress during flush is treated as end-of-stream Related: image-rs/weezl#72
lilith
added a commit
to lilith/image-gif
that referenced
this pull request
Apr 13, 2026
When decoding interlaced frames, read_into_buffer passes per-row slices to the LZW decoder. For small images (7-47 byte rows), weezl returns NoProgress because the output buffer is too small for its internal state machine to advance. Two bugs fixed in DecodeSubBlock: 1. Input data skipped (left > 0): When NoProgress with consumed=0 and bytes_len=0, the decoder executed `consumed = n`, skipping all remaining sub-block input without processing it. 2. Premature frame end (left = 0): During flush, NoProgress with 0 output was treated as end-of-stream, but weezl still had buffered data. Fix: retry LZW decode with a 1-byte temp buffer (lzw_drain_one) to unstick weezl, then append the byte to the caller's output. Related: image-rs/weezl#72
3 tasks
197g
reviewed
Apr 14, 2026
| } | ||
|
|
||
| debug_assert!(self.next_code <= size_switch_at); | ||
| debug_assert!(self.table.is_full() || self.next_code <= size_switch_at); |
197g
approved these changes
Apr 14, 2026
Member
197g
left a comment
197g
left a comment
There was a problem hiding this comment.
I had planned for this to go into 0.2 but this probably deserves a backport. Also now I'm wondering if the logic of the initial block can be cleaned up with this. Alas, good fix.
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
2 participants
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.
Summary
Three fixes for yield_on_full correctness on the
wuffsbranch, plus regression tests.Fixes
Unconditional buffer drain (yield_on_full + small output buffer: false NoProgress loses data #68): The buffer drain at the start of
advance()was gated oncode_link.is_some(). After a clear code setscode_link = None, buffered data was silently lost. Same root cause as onmaster, same fix (PR test: failing regression tests for yield_on_full data loss (#68) #71).debug_assert in non-broken-burst path:
debug_assert!(self.next_code <= size_switch_at)fires in TIFF mode when the table is full (next_code= 4096 exceedssize_switch_at= 4094). Guard:self.table.is_full() || ....reconstruct_simple overwrite guard: The relaxed 8-byte-chunk overwrite can clobber bytes past the code length. With
yield_on_fullor tight buffers, those bytes may be returned to the caller before subsequent codes overwrite them. Guard: require >= 8 bytes of slack past the code length.Regression tests
Three tests in
tests/yield_on_full_regression.rs:regression_yield_on_full_fuzz_crash: minimized fuzz input (size=7, MSB)regression_yield_on_full_small_buffers: buffer sizes 1-64yield_on_full_sweep_sizes: all code sizes 2-12Status
Fixes 1 and 2 pass. Fix 3 resolves the overwrite concern but
small_bufferswithbuf=1still fails — the inline burst reconstruction has a separate data-loss path whenyield_on_fullcauses mid-burst returns. The burst loop writes directly intooutduring validation, and when the burst breaks mid-way, some of that written data may not be properly accounted for. This needs further investigation specific to the wuffs burst loop restructuring.Test plan
regression_yield_on_full_fuzz_crashpassesyield_on_full_sweep_sizespassesregression_yield_on_full_small_buffers(buf=1) — still fails, needs separate fix