zstd: Re-enable unsafe decodeSync memory copies (#1168)

[lizthegrey]

Closes #1168 by taking option 2 (root-cause + re-enable with regression coverage).

Background

(*sequenceDecs).decodeSyncSimple has had const useSafe = true hardcoded since #644 (2022-07-17), so the sequenceDecs_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 bitReader overread in updateLength produced 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 dynamic useSafe selection 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 only outPos+ll+ml <= cap(s.out), with no margin for the copies' 15-byte overrun. A well-formed decode ends at cap-16 and 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 past cap(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 -race nor -asan poisons, 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 compressedBlockOverAlloc in the space check on the unsafe path only (one ADDQ per unsafe variant; the bounds-exact safe variants keep the tight check). Well-formed streams are unaffected; an over-producing stream now returns error_not_enough_space instead 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 extracted useSafeDecodeSync() helper) that the unsafe variant is really selected. It fails on the unpatched assembly (canary overwritten past cap) and passes with the fix, on amd64 and arm64.
• fuzz-zstd-asan CI job (CGO+clang, amd64+arm64) over FuzzDecodeAll/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)

• Full zstd test suite, all build-tag combos, both architectures.
• asm-vs-noasm differential fuzz: byte-identical output.
• Canary reproducer: fail-before/pass-after verified on both architectures.
• ASan over the full decode corpus + 2.4M mutation execs: zero reports; plain fuzz 22.8M+ execs clean (default and -tags=nounsafe).

Performance

BenchmarkDecoder_DecodeAll on arm64 (Cortex-A72), n=6, p=0.002:

• +8.7% geomean throughput, up to +16% on text (asyoulik +16%, plrabn12 +15%, alice29 +15%, kppkn +12%).
• No change on already-fast inputs (jpeg), as expected.
• The added space-check margin is one register add per sequence batch; no measurable cost.

Summary by CodeRabbit

• Bug Fixes
  • Improved zstd decodeSync safety by dynamically selecting between safer and optimized copy behavior based on output/literals geometry.
  • Tightened output-capacity checks in the decode assembly paths to reduce the risk of out-of-bounds writes.
• Tests
  • Added an amd64/arm64 regression test that reproduces an unsafe/extended-copy scenario and verifies no memory beyond the output buffer is modified.
• Chores
  • Added an AddressSanitizer-protected CI fuzzing job for zstd decode fuzz targets.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3c945107-e1cc-4816-84b7-3cfb6dd8029d

📥 Commits

Reviewing files that changed from the base of the PR and between 47ea5b1 and 12cd4b4.

📒 Files selected for processing (1)

• zstd/seqdec_arm64.s

🚧 Files skipped from review as they are similar to previous changes (1)

• zstd/seqdec_arm64.s

---

📝 Walkthrough

Walkthrough

decodeSyncSimple now selects safe or extended-copy decoding from buffer slack, while generated assembly checks reserve extended-copy slack. Regression coverage and an AddressSanitizer fuzzing job exercise the affected zstd decode paths.

Changes

zstd decode safety and validation

Layer / File(s)	Summary
Decode copy bounds zstd/_generate/gen.go, zstd/seqdec_amd64.s, zstd/seqdec_arm64.s	Defines the 16-byte extended-copy allocation and updates generated and architecture-specific output-space checks to account for it.
Dynamic decode copy selection zstd/seqdec_asm.go	decodeSyncSimple computes safe-copy mode from output and literal buffer capacity before calling decodeSyncAsm.
Decode safety validation zstd/seqdec_oob_test.go, .github/workflows/go.yml	Adds canary-based unsafe-path OOB coverage and a CGO-enabled Ubuntu matrix job using clang and Go 1.25.x for targeted ASAN fuzzing.

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

Loading

Possibly related issues

• zstd: decodeSync's unsafe path has been dead code since 2022 (#644) — remove or fix? #1168: Directly covers re-enabling and safeguarding decodeSync unsafe-path selection and regression coverage.

Suggested reviewers: klauspost

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: re-enabling unsafe decodeSync copies in zstd.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

.github/workflows/go.yml (1)

199-200: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Set persist-credentials: false in actions/checkout.

To improve security hygiene and prevent credential persistence through GitHub Actions artifacts, it's recommended to set persist-credentials: false when 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9960386 and 404b74c.

📒 Files selected for processing (2)

• .github/workflows/go.yml
• zstd/seqdec_asm.go

[lizthegrey]

the irony: "enable unsafe, it's safe now!" crashes on fuzz failure on the very first CI build.

[lizthegrey]

Pushed a fix for the arm64 CI crash (ba6f83c).

Root cause — not the bitReader overread, but a missing copy margin. The per-sequence space check in executeSingleTriple requires only outPos+ll+ml <= cap(s.out), but the unsafe extended copies write in 16-byte blocks and overrun the logical end by up to 15 bytes. When a decode ends inside the 16-byte over-allocation slack — e.g. a malformed frame whose declared content size is a few bytes under what its sequences produce — the check passes yet the final block copy writes past cap(s.out). Go's size-class rounding hides the ≤15-byte overrun from -race and even -asan (it lands in unpoisoned slop), so it only intermittently corrupts an adjacent live object and surfaces later as a GC crash. That matches the "rare, random crashes" #644 originally disabled this path for.

Fix — reserve compressedBlockOverAlloc in the space check on the unsafe path only. A well-formed decode ends at cap-16 and is unaffected; an over-producing stream now returns error_not_enough_space instead of corrupting memory. The safe copies stay bounds-exact with the tight check. One ADDQ per unsafe variant; safe variants unchanged.

Repro/regression — TestDecodeSyncUnsafeOOB drives the unsafe asm with the 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 with the fix, on both amd64 and arm64. Also validated with the full suite + FuzzDecodeAll (default and -tags=nounsafe) on go1.25 and go1.26.

Since asan can't see this overrun class, the fuzz-zstd-asan job gives false confidence here — the canary test is the real guard and belongs in CI.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 404b74c and ba6f83c.

📒 Files selected for processing (4)

• zstd/_generate/gen.go
• zstd/seqdec_amd64.s
• zstd/seqdec_arm64.s
• zstd/seqdec_oob_test.go

[lizthegrey]

Done in 47ea5b1 — folded compressedBlockOverAlloc into the LEAQ displacement as you suggested. Only the unsafe amd64/bmi2 variants change (one fewer instruction each); the safe path (addMargin=0) and arm64 codegen are unaffected. Verified via regenerated asm diff + full zstd suite (incl. TestDecodeSyncUnsafeOOB) on both amd64 and arm64.

[klauspost]

lgtm