fix issue 44108 #3033
Merged
fix issue 44108 #3033
Conversation
This file contains 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
credit to oss-fuzz In rare circumstances, the block-splitter might cut a block at the exact beginning of a repcode. In which case, since litlength=0, if the repcode expected 1+ literals in front, its signification changes. This scenario is controlled in ZSTD_seqStore_resolveOffCodes(), and the repcode is transformed into a raw offset when its new meaning is incorrect. In more complex scenarios, the previous block might be emitted as uncompressed after all, thus modifying the expected repcode history. In the case discovered by oss-fuzz, the first block is emitted as uncompressed, so the repcode history remains at default values: 1,4,8. But since the starting repcode is repcode3, and the literal length is == 0, its meaning is : = repcode1 - 1. Since repcode1==1, it results in an offset value of 0, which is invalid. So that's what the `assert()` was verifying : the result of the repcode translation should be a valid offset. But actually, it doesn't matter, because this result will then be compared to reality, and since it's an invalid offset, it will necessarily be discarded if incorrect, then the repcode will be replaced by a raw offset. So the `assert()` is not useful. Furthermore, it's incorrect, because it assumes this situation cannot happen, but it does, as described in above scenario.
felixhandte
approved these changes
Jan 27, 2022
Merged
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.
credit to oss-fuzz
In rare circumstances, the block-splitter might cut a new block at the exact beginning of a repcode.
In which case, since litlength=0, if the repcode expected 1+ literals in front, its signification changes.
Thankfully, this scenario is controlled in
ZSTD_seqStore_resolveOffCodes(),and the repcode is transformed into a raw offset when its new meaning is incorrect.
In more complex scenarios, the previous block might be emitted as uncompressed at a later decision stage,
thus modifying the expected repcode history.
In the specific case discovered by oss-fuzz, the first block is emitted as uncompressed,
so the repcode history remains at its starting default values: 1,4,8.
But since the starting repcode is repcode3, and the literal length is == 0,
its meaning is now : = repcode1 - 1.
Since repcode1==1, it results in an offset value of 0, which is invalid.
So that's what the
assert()was verifying : the result of the repcode translation should be a valid offset.And since it was not, it was firing.
But actually, it doesn't matter, because this result will then be compared to reality,
and since it's an invalid offset, it will necessarily be discarded if incorrect,
then the repcode will be replaced by a raw offset.
So the
assert()is not useful.Furthermore, it's incorrect, because it assumes this situation cannot happen, while it actually can happen, as just described in above scenario.