[bug-fix] Fix a determinism bug with the DUBT #2726
Merged
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
Cyan4973
reviewed
Jul 15, 2021
| window->dictLimit = 1; /* start from 1, so that 1st position is valid */ | ||
| window->lowLimit = 1; /* it ensures first and later CCtx usages compress the same */ | ||
| window->nextSrc = window->base + 1; /* see issue #1241 */ | ||
| window->base = (BYTE const*)" "; |
There was a problem hiding this comment.
for my understanding : why " " ? avoiding UB pointer arithmetic ?
There was a problem hiding this comment.
Yeah, CI compiler warnings were complaining about nextSrc = window->base + 2 being beyond the end of the index.
The DUBT can be non-deterministic if an index is equal to `ZSTD_DUBT_UNSORTED_MARK`. Ensure that never happens by starting the indices at 2. This bug was found by the OSS-Fuzz determinism fuzzer. With this change the fuzzer test passes. And I've confirmed that this is the root cause, not just hiding the problem. Aside: This took me a long time to figure out, because I thought I had tried this first thing. But, apparantly I messed it up, because when I was going through it again with @felixhandte, I was pointing out that it wasn't the case, but it turns out it was. Credit to: OSS-Fuzz
felixhandte
added a commit
to felixhandte/zstd
that referenced
this pull request
Jul 22, 2021
This PR fixes an incorrect comparison in figuring out `minChain` in `ZSTD_dedicatedDictSearch_lazy_loadDictionary()`. This incorrect comparison had been masked by the fact that `idx` was always 1, until @terrelln changed that in facebook#2726. Credit-to: OSS-Fuzz
Merged
felixhandte
added a commit
to felixhandte/zstd
that referenced
this pull request
Jul 27, 2021
This PR fixes an incorrect comparison in figuring out `minChain` in `ZSTD_dedicatedDictSearch_lazy_loadDictionary()`. This incorrect comparison had been masked by the fact that `idx` was always 1, until @terrelln changed that in facebook#2726. Credit-to: OSS-Fuzz
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 DUBT can be non-deterministic if an index is equal to
ZSTD_DUBT_UNSORTED_MARK. Ensure that never happens by starting theindices at 2.
This bug was found by the OSS-Fuzz determinism fuzzer. With this change
the fuzzer test passes. And I've confirmed that this is the root cause,
not just hiding the problem.
Aside: This took me a long time to figure out, because I thought I had
tried this first thing. But, apparantly I messed it up, because when I
was going through it again with @felixhandte, I was pointing out that it
wasn't the case, but it turns out it was.
Credit to: OSS-Fuzz