Repository navigation
legacy/v0.2 and v0.3: stop truncating the offset bound to 32 bits - #4817
Open
ManoharPaturi wants to merge 1 commit into
Open
ManoharPaturi wants to merge 1 commit into
ManoharPaturi wants to merge 1 commit into
Conversation
The offset validation in ZSTD_execSequence() compares the sequence offset against (U32)(oLitEnd - base). Once more than 4 GiB has been decoded into a single destination buffer, the cast truncates the distance to its low 32 bits, so the bound wraps to distance minus 4 GiB and offsets that are legal for the data decoded so far get rejected. A valid frame then fails with corruption_detected as soon as one of its sequences crosses the 4 GiB mark. Compare in size_t instead, as every decoder from v0.4 onward already does. This is the same correction proposed for the v0.1 decoder in meta#4680. Demonstrated with a crafted v0.2 frame (and its v0.3 twin, byte identical apart from the magic number): 256 blocks of 16 MiB each, then a final block whose sequence carries offset 131072 while the decoded position is exactly 4 GiB. Unfixed, both decoders reject the frame with corruption_detected. Fixed, both decode the full 4294967466 bytes.
Author
|
@Cyan4973 Hi, wanted to follow up on this PR. Let me know if there's anything else I can do to help move it forward. Thanks! |
Author
|
@Cyan4973 these legacy decoder fixes (#4817 among the v0.2 through v0.7 batch) have been green on all checks for a few weeks now with no reviewer assigned yet. Each one is a narrow correctness fix in one legacy format with a round-trip regression test in tests/legacy.c. Could you or another maintainer take a look when the queue allows? |
This branch has not been deployed
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.
The offset validation in the v0.2 and v0.3 implementations of ZSTD_execSequence() reads:
The cast truncates the distance between the current write position and the start of the destination buffer to its low 32 bits. As soon as a single call has decoded more than 4 GiB, the bound wraps to distance minus 4 GiB, so an offset that is perfectly legal for the amount of data decoded so far is rejected and the whole frame fails with corruption_detected. Frames of this size are legitimate: these decoders are one shot only, and nothing prevents a caller from handing them a destination buffer above 4 GiB.
Every decoder from v0.4 onward already compares in size_t. This change does the same in v0.2 and v0.3, and is the same correction proposed for the v0.1 decoder in #4680, which this complements for the two remaining format versions carrying the cast.
Demonstration, using a crafted v0.2 frame (its v0.3 twin is byte identical apart from the magic number): 256 blocks expanding to 16 MiB each, then a final block whose sequence carries offset 131072 while the decoded position sits exactly at 4 GiB, for a total of 4294967466 bytes. On current dev both decoders reject the frame:
With the size_t comparison both decode the frame completely:
The frame is 8283 bytes and decodes in under two seconds, but needs a 4 GiB destination buffer, so it is not added as a committed test. The existing legacy tests pass at every level from 1 to 7.