zstd: Re-enable unsafe decodeSync memory copies (#1168) - #1171
Conversation
decodeSyncSimple has hardcoded `const useSafe = true` since klauspost#644 (2022), making the faster `sequenceDecs_decodeSync_{amd64,bmi2,arm64}` asm variants dead code that nothing calls. The klauspost#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 klauspost#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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changeszstd decode safety and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant sequenceDecs
participant decodeSyncSimple
participant decodeSyncAsm
participant outputBuffer
sequenceDecs->>decodeSyncSimple: provide buffer capacities and sync length
decodeSyncSimple->>decodeSyncSimple: select safe or extended-copy mode
decodeSyncSimple->>decodeSyncAsm: decode sequences with selected mode
decodeSyncAsm->>outputBuffer: copy literals and matches
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/go.yml (1)
199-200: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseinactions/checkout.To improve security hygiene and prevent credential persistence through GitHub Actions artifacts, it's recommended to set
persist-credentials: falsewhen checking out the code, especially in jobs running tests or fuzzing.🛠️ Proposed fix
- name: Checkout code uses: actions/checkout@v7.0.0 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/go.yml around lines 199 - 200, Update the actions/checkout step in the workflow to set persist-credentials to false, while preserving the existing checkout action version and job behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/go.yml:
- Around line 199-200: Update the actions/checkout step in the workflow to set
persist-credentials to false, while preserving the existing checkout action
version and job behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aea2c5ea-523c-4459-8a3e-85056a82d8fb
📒 Files selected for processing (2)
.github/workflows/go.ymlzstd/seqdec_asm.go
|
the irony: "enable unsafe, it's safe now!" crashes on fuzz failure on the very first CI build. |
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>
|
Pushed a fix for the arm64 CI crash ( Root cause — not the bitReader overread, but a missing copy margin. The per-sequence space check in Fix — reserve Repro/regression — Since asan can't see this overrun class, the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zstd/seqdec_oob_test.go`:
- Around line 101-115: Update the test around decodeSyncSimple so it explicitly
exercises the unsafe copy path rather than only confirming assembly dispatch.
Invoke decodeSyncAsm with useSafe=false, or otherwise instrument the selection
to verify unsafe mode was chosen, while preserving the existing canary
validation and asmPaths assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d0c1be3-0c2c-4d69-b654-fa172e4d0900
📒 Files selected for processing (4)
zstd/_generate/gen.gozstd/seqdec_amd64.szstd/seqdec_arm64.szstd/seqdec_oob_test.go
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>
Klaus's review on klauspost#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.
|
Done in 47ea5b1 — folded |
2 participants
Closes #1168 by taking option 2 (root-cause + re-enable with regression coverage).
Background
(*sequenceDecs).decodeSyncSimplehas hadconst useSafe = truehardcoded since #644 (2022-07-17), so thesequenceDecs_decodeSync_{amd64,bmi2,arm64}asm variants are generated and shipped but never called. #644's comment blames "rare, random crashes with fuzz testing" from the extended (16-byte-block) memory copies.There were two defects, not one
The extended copies overrun the end of a literal run or match by up to 15 bytes by design, relying on correct decoded offsets/lengths and 16 bytes (
compressedBlockOverAlloc) of slack on the output/literal buffers. Two independent bugs could break that contract:1. The bitReader overread (fixed in 2022). An unguarded
bitReaderoverread inupdateLengthproduced out-of-range match offsets/lengths, with only debug-only asserts to catch them. Fixed three days after the disable in #645 (4b4f3c9), which added runtime overread guards and introduced the Go fuzz corpus. This PR's first commit restored the dynamicuseSafeselection on that basis.2. A missing margin in the space check (found and fixed in this PR). The original analysis above missed a case, and this PR's own arm64 CI caught it: a SIGSEGV in
runtime.scanobject— the GC tripping over corrupted heap. The per-sequence space check requires onlyoutPos+ll+ml <= cap(s.out), with no margin for the copies' 15-byte overrun. A well-formed decode ends atcap-16and the overrun lands in slack — but a malformed frame whose declared content size is a few bytes below what its sequences actually produce ends inside the slack: the check passes, and the final block copy writes pastcap(s.out).This, not the overread, is the likely source of #644's "rare, random" crashes: the ≤15-byte overrun lands in size-class slop that neither
-racenor-asanpoisons, so it only intermittently corrupts an adjacent live object and surfaces much later as a GC crash rather than at the faulting write.The fix reserves
compressedBlockOverAllocin the space check on the unsafe path only (oneADDQper unsafe variant; the bounds-exact safe variants keep the tight check). Well-formed streams are unaffected; an over-producing stream now returnserror_not_enough_spaceinstead of corrupting memory.Regression coverage
TestDecodeSyncUnsafeOOB— the guard that actually catches this class. It drives the unsafe asm with real captured sequences into a canary-guarded buffer carved from a larger backing array, sized to model the under-reporting case, and asserts (via the extracteduseSafeDecodeSync()helper) that the unsafe variant is really selected. It fails on the unpatched assembly (canary overwritten pastcap) and passes with the fix, on amd64 and arm64.fuzz-zstd-asanCI job (CGO+clang, amd64+arm64) overFuzzDecodeAll/FuzzDecAllNoBMI2. Caveat learned the hard way: asan does not see sub-size-class slice overruns (they land in unpoisoned slop), so it guards gross overruns and allocation regressions, not this ≤15-byte class — that's what the canary test is for.Validation (amd64 + arm64/Cortex-A72, go1.25 + go1.26)
zstdtest suite, all build-tag combos, both architectures.-tags=nounsafe).Performance
BenchmarkDecoder_DecodeAllon arm64 (Cortex-A72), n=6, p=0.002:Summary by CodeRabbit
decodeSyncsafety by dynamically selecting between safer and optimized copy behavior based on output/literals geometry.