Fix silent corruption for large backreference offsets#22
Open
erseco wants to merge 2 commits into
Open
Conversation
The 4-byte offset bit-read in the sequence loop supplies only 32 - (spos & 7) usable bits, so offset codes >= 26 could silently lose their high bits whenever the field straddled past that window. The 2^ofc leading term survives, leaving an in-bounds but too-small offset that splices data from the wrong position: output length stays correct, no error is thrown, and decompress() and Decompress corrupt identically. Read the fifth byte when the field needs it. Also adds streaming regression tests for the issue 101arrowz#19 scenario (embedded frames, exhaustive chunk-split sweeps, never-final mode, multi-frame) plus an opt-in large-offset test (FZSTD_BIG_TESTS=1), and updates the README note about the former 2^25 backreference limit.
Reject offset codes outside the decoder's signed 31-bit range, reuse the bit alignment and mask in the sequence loop, and keep the fifth-byte read limited to fields that actually cross the 32-bit window. Also narrow the README claims to the support actually provided and correct the large regression test's threshold, generator name, and measured memory requirement.
4d6eb36 to
15007f3
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.
Related to #19
Summary
fzstdcould silently decode the wrong bytes when a sequence's backreference offset needed more bits than the fixed four-byte read made available after alignment.For
sh = spos & 7, the existing read exposes only32 - shusable bits. Offset codesofc >= 26can therefore cross into a fifth byte whensh + ofc > 32. The first affected match distance is approximately 96 MiB (2^26 + 2^25 - 3), so this is relevant to frames that actually use long-distance matches, typically withwindowLog >= 27,--long, or suitable ultra-level settings.The leading
2^ofcterm remained intact, so the truncated value was normally still in range. The decoder then copied from a nearer, incorrect position without throwing, while preserving the output length.Fix
Validation
The offset arithmetic was checked against a
BigIntreference for everyofcfrom 0 through 30, all eight bit alignments, and randomized five-byte inputs.The included regressions cover:
zstd -1 --long=27corpus with approximately 100 MiB match distances.Local large-test result:
Performance
The hot sequence path gains one range check and one predictable conditional. The fifth-byte read is only executed for offset fields that actually cross the four-byte window. No end-to-end performance claim is made by this PR.
Scope
This is separate from upstream PR #21, which fixes int32 overflow in bit positions for one-shot frames whose compressed size exceeds 256 MiB. The changes are compatible.
The historical
"hello world ".repeat(11_000)streaming symptom occurs onfzstd <= 0.1.0and is already fixed on currentmaster; the new small regression cases keep that behavior covered.