zstd: Fix crash on amd64 (no BMI) + Go fuzz test - #645
Merged
Conversation
Port zstd fuzz tests to Go 1.18 fuzz tests.
Owner
Author
|
Owner
Author
|
@WojciechMula It seems this problem can be reliably triggered by disabling BMI It seems like it is possible to get to It could either be triggered by an invalid table or incorrect decoding. |
This was referenced Jul 18, 2022
Closed
Owner
Author
|
updateLength appears to be the problem. |
klauspost
force-pushed
the
port-zstd-fuzz-tests
branch
from
July 20, 2022 10:27
7b38327 to
5357b67
Compare
Owner
Author
|
updateLength would output junk values if bitsRead > 64. This would happen on corrupted input. |
klauspost
changed the title
lizthegrey
added a commit
to honeycombio/compress
that referenced
this pull request
Jul 18, 2026
The per-sequence space check in executeSingleTriple (used by the sync decoder's inlined execute) requires only outPos+ll+ml <= cap(s.out). The unsafe extended copies, however, write in 16-byte blocks and overrun the logical end of a run by up to compressedBlockOverAlloc-1 (15) bytes. When a stream's decoded length lands within the 16-byte over-allocation slack -- e.g. a malformed frame whose declared content size is a few bytes below what its sequences actually produce -- the check passes but the final block copy writes past cap(s.out). The overrun is at most 15 bytes and Go's size-class rounding places it in slop that -race and -asan do not poison, so it corrupts an adjacent live allocation only intermittently, surfacing later as a crash in the garbage collector rather than at the write. That is exactly the 'rare, random crashes with fuzz testing' klauspost#644 disabled the unsafe path for in 2022; it was never the bitReader overread fixed in klauspost#645, but this missing copy margin. Reserve compressedBlockOverAlloc in the space check on the unsafe (non-safe) path only, so the overrun stays within cap(s.out); a well-formed decode ends at cap-16 and is unaffected, while an over-producing stream now errors with error_not_enough_space instead of corrupting memory. The safe copies are bounds-exact and keep the tight check. amd64 asm gains one ADDQ per unsafe variant; the safe variants are unchanged. TestDecodeSyncUnsafeOOB drives the unsafe asm with real captured sequences into a canary-guarded buffer sized to model the under-reported case; it fails (canary overwritten past cap) on the unpatched asm and passes here, on amd64 and arm64. Validated additionally with the full zstd suite and FuzzDecodeAll (default and -tags=nounsafe) on go1.25 + go1.26, both arches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
klauspost
added a commit
that referenced
this pull request
Jul 21, 2026
* zstd: Re-enable unsafe decodeSync memory copies (#1168) decodeSyncSimple has hardcoded `const useSafe = true` since #644 (2022), making the faster `sequenceDecs_decodeSync_{amd64,bmi2,arm64}` asm variants dead code that nothing calls. The #644 comment cites "rare, random crashes with fuzz testing" from the extended (16-byte-block) copies. Those copies were only the amplifier. The root cause was an unguarded bitReader overread in updateLength that produced out-of-range match offsets/lengths; it was fixed three days later in #645, which also added the Go fuzz corpus that has guarded this path ever since. With the source of the bad values gone and the +compressedBlockOverAlloc (16-byte) slack present on every output/literal buffer today, the dynamic useSafe guard — identical in shape to the still-active one in executeSimple — is sound. Restore the dynamic selection. On arm64 (Cortex-A72) DecodeAll improves by +8.7% geomean throughput (n=6, p=0.002), up to +16% on text. Because a stray 15-byte overrun lands in an adjacent live allocation, neither -race nor plain fuzzing can observe it; add a CGO+clang asan fuzz job over FuzzDecodeAll/FuzzDecAllNoBMI2 on amd64+arm64 so any future maxSyncLen/allocation regression is caught deterministically. Validated on amd64 and arm64: full test suite; asm-vs-noasm differential fuzz (byte-identical output); and asan over the decode corpus plus 2.4M mutation execs with no reports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * zstd: fix out-of-bounds write in unsafe decodeSync space check The per-sequence space check in executeSingleTriple (used by the sync decoder's inlined execute) requires only outPos+ll+ml <= cap(s.out). The unsafe extended copies, however, write in 16-byte blocks and overrun the logical end of a run by up to compressedBlockOverAlloc-1 (15) bytes. When a stream's decoded length lands within the 16-byte over-allocation slack -- e.g. a malformed frame whose declared content size is a few bytes below what its sequences actually produce -- the check passes but the final block copy writes past cap(s.out). The overrun is at most 15 bytes and Go's size-class rounding places it in slop that -race and -asan do not poison, so it corrupts an adjacent live allocation only intermittently, surfacing later as a crash in the garbage collector rather than at the write. That is exactly the 'rare, random crashes with fuzz testing' #644 disabled the unsafe path for in 2022; it was never the bitReader overread fixed in #645, but this missing copy margin. Reserve compressedBlockOverAlloc in the space check on the unsafe (non-safe) path only, so the overrun stays within cap(s.out); a well-formed decode ends at cap-16 and is unaffected, while an over-producing stream now errors with error_not_enough_space instead of corrupting memory. The safe copies are bounds-exact and keep the tight check. amd64 asm gains one ADDQ per unsafe variant; the safe variants are unchanged. TestDecodeSyncUnsafeOOB drives the unsafe asm with real captured sequences into a canary-guarded buffer sized to model the under-reported case; it fails (canary overwritten past cap) on the unpatched asm and passes here, on amd64 and arm64. Validated additionally with the full zstd suite and FuzzDecodeAll (default and -tags=nounsafe) on go1.25 + go1.26, both arches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * zstd: assert TestDecodeSyncUnsafeOOB exercises the unsafe copy path Review feedback on the regression test: it verified only that the asm path was dispatched, relying implicitly on the buffer geometry to select the unsafe (extended-copy) variant. If the useSafe conditions or the test harness allocation ever changed, the test could silently degrade into exercising the bounds-exact safe copies while still passing. Extract decodeSyncSimple's selection into useSafeDecodeSync() (pure refactor, no behavior change) and have the test call the same helper to fail loudly if its buffer sizing would select the safe variant. The canary validation and asm-path assertion are unchanged; the test still fails via the canary against the unpatched assembly on both architectures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * zstd: fold unsafe-copy margin into the space-check LEAQ Klaus's review on #1171 pointed out the compressedBlockOverAlloc margin can be folded into the existing LEAQ displacement instead of a separate ADDQ. Saves one instruction per unsafe variant on amd64/bmi2; the safe path (addMargin=0) and arm64 codegen are unaffected in substance. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Klaus Post <klauspost@gmail.com>
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
1 participant
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.
Port zstd fuzz tests to Go 1.18 fuzz tests.
(May need time extension on Go 1.18 CI)