harden(decode): fallible BufferBackend writes for RLE/Raw direct path#251
Merged
Conversation
Adds a parallel `try_*` write surface to `BufferBackend`:
* `BackendOverflow` error type with tail / requested / capacity
diagnostics.
* `try_extend`, `try_extend_and_fill`, `try_extend_from_within`
trait methods. Default impls call the existing panic-on-overflow
variants — growable backends (FlatBuf / RingBuffer) keep their
current behaviour because they cannot fail for capacity reasons
(the Vec grows on demand).
* `UserSliceBackend` overrides all three with explicit capacity
checks that return `Err(BackendOverflow)` instead of panicking
via the release-mode `assert!`.
* `DecodeBuffer::try_push` and `try_extend_and_fill` wrap the
backend methods and keep the `total_output_counter` bookkeeping
consistent on Ok.
* `BlockDecoder::decode_block_content_from_slice` RLE / Raw
branches switch to `try_extend_and_fill` / `try_push` and
surface `DecodeBlockContentError::BackendOverflow { step }` on
overshoot.
* `FrameDecoder::decode_to_slice_trusted` converts that variant
into `FrameDecoderError::FrameContentSizeMismatch` so the public
API stays panic-free on these paths even if the per-block
pre-flight FCS check is bypassed (defense in depth — the
pre-flight catches the common case first).
Scope deliberately limited to RLE / Raw. The Compressed-block
sequence executor still writes via the existing unchecked path;
threading `try_*` through the fused decode+execute pipeline is the
next step toward unconditional adversarial-input safety. The
`_trusted` suffix on `decode_to_slice_trusted` reflects that
limitation — the rename to plain `decode_to_slice` waits for the
Compressed path.
Full test suite (652 tests) green; 8 existing direct-path tests
unchanged in behaviour. Clippy clean.
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 34 minutes and 5 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces fallible buffer backend operations to replace panic-on-overflow behavior with structured error handling for fixed-capacity buffers. It adds ChangesFallible Buffer Overflow Handling
Possibly related issues
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/decoding/buffer_backend.rs`:
- Around line 185-196: The default BufferBackend::try_extend_from_within must
validate the source-range bound before calling the unsafe helper: first check
that start.checked_add(len).map_or(false, |end| end <= self.len()) and return
Err(BackendOverflow) if it fails, then call self.reserve(len) and finally call
unsafe { self.extend_from_within_unchecked(start, len) } and return Ok(()).
Ensure you reference BufferBackend::try_extend_from_within, reserve,
extend_from_within_unchecked, len(), and BackendOverflow in the change so
growable backends uphold the documented safety contract.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d51fbdd-9e51-4cbd-8dcb-86bdae183f88
📒 Files selected for processing (6)
zstd/src/decoding/block_decoder.rszstd/src/decoding/buffer_backend.rszstd/src/decoding/decode_buffer.rszstd/src/decoding/errors.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/user_slice_buf.rs
There was a problem hiding this comment.
Pull request overview
This PR hardens the direct decode-to-user-slice path by adding a fallible (try_*) write API to BufferBackend and switching Raw/RLE block decode to use it, so fixed-capacity output buffers can return structured errors instead of panicking on overflow.
Changes:
- Introduces
BackendOverflowplusBufferBackend::{try_extend, try_extend_and_fill, try_extend_from_within}fallible write methods. - Implements capacity-checked fallible writes for
UserSliceBackendand threads them throughDecodeBuffer+ Raw/RLE block decode. - Converts Raw/RLE backend-overflow failures into
FrameDecoderError::FrameContentSizeMismatchat thedecode_to_slice_trustedboundary.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| zstd/src/decoding/user_slice_buf.rs | Adds UserSliceBackend fallible write overrides with explicit capacity checks. |
| zstd/src/decoding/buffer_backend.rs | Extends BufferBackend with fallible try_* methods and introduces BackendOverflow. |
| zstd/src/decoding/decode_buffer.rs | Adds fallible DecodeBuffer::{try_push, try_extend_and_fill} wrappers and keeps counters consistent on success. |
| zstd/src/decoding/block_decoder.rs | Switches Raw/RLE block write paths to use the new fallible buffer methods and maps overflow to a decode error. |
| zstd/src/decoding/errors.rs | Adds BackendOverflow variants to decode error enums and updates Display formatting. |
| zstd/src/decoding/frame_decoder.rs | Converts block-level backend overflow into FrameContentSizeMismatch for the direct decode-to-slice API. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
* `UserSliceBackend::try_extend`, `try_extend_and_fill`, `try_extend_from_within` now use `checked_add` on every `tail + len`, `head + start`, `start + len` computation. Without the wrap check, an adversarial `len` near `usize::MAX` could wrap the sum below the existing upper-bound test, bypassing the fallible path and panicking inside `copy_bytes_overshooting` / `slice[tail..new_tail]` (start > end). * `BufferBackend::try_extend_from_within` default impl validates BOTH halves of the underlying unsafe contract — source range via `start + len <= self.len()` (with `checked_add`) and destination range via `tail + len <= cap` (after `reserve`) — before forwarding to `unsafe extend_from_within_unchecked`. The earlier default impl only reserved capacity; the safe entry point could trigger UB on growable backends if a caller passed a bad `start`. Fixed-capacity backends override with a tighter pair of checks that does not call `reserve` (no-op on them). * `BufferBackend::try_extend` doc-string corrected: the default impl returns `Ok(())` for growable backends; Vec OOM is an abort, not a recoverable `Err` — the previous wording falsely implied otherwise. * `DecodeBlockContentError::BackendOverflow` doc-string corrected: this is an internal diagnostic variant that always converts to `FrameDecoderError::FrameContentSizeMismatch` before reaching the public API. Reworded to say "in-crate debugging" rather than "callers can distinguish".
…ttern Addresses two Copilot review threads on PR #251: 1. errors.rs DecodeBufferError::BackendOverflow doc comment claimed the variant is surfaced from the direct-decode path via FrameContentSizeMismatch, but no production code constructs it yet — the Compressed-block sequence executor still uses the unchecked extend_from_within and panics on overshoot via release-mode assert!. The variant is reserved for the follow-up that wires try_extend_from_within through DecodeBuffer::repeat*. Clarified the doc to say the variant is PRESENT but NOT WIRED on production paths yet; the _trusted suffix on decode_to_slice_trusted reflects that the burst path is still hardened via the panic-path until that follow-up lands. 2. buffer_backend.rs module-level comment claimed FlatBuf / RingBuffer 'override' the new try_* methods. They do not — they rely on the default trait impls that delegate to the panic-on-overflow variant and always return Ok. The phrasing implied per-backend impls that readers might look for and not find. Reworded to say growable backends rely on the default impls (which call .extend / .extend_and_fill / .extend_from_within_unchecked and never produce Err because the underlying Vec grows on demand). No behaviour change — only doc-comment text updates. Existing tests unaffected; cargo check release clean.
Codecov flagged the new try_extend / try_extend_and_fill / try_extend_from_within paths at 23.95% patch coverage on PR #251 — 73 missing lines out of the 305-line file delta. The new tests exercise the production paths so codecov's patch threshold (85%) holds for the introduced lines: try_extend - exact-fit happy path (advances tail to capacity) - over-capacity returns BackendOverflow with tail/requested/capacity diagnostics, tail unchanged - partially-full state reports current tail in overflow - checked_add wrap branch (documented as unreachable from safe Rust, reachable only via fuzz_exports — covers the line for codecov) try_extend_and_fill - exact-fit pattern fill - over-capacity returns BackendOverflow with same diagnostics try_extend_from_within - within-bounds repeats history correctly (donor wildcopy semantics) - source range past tail returns BackendOverflow - destination range past capacity returns BackendOverflow 17 tests run, 17 pass. No code change — pure test additions.
…isleading test Addresses three Copilot review threads on PR #251: 1. buffer_backend.rs:224 — default try_extend_from_within had a linear tail+len <= cap check that's wrong for wrap-aware backends. RingBuffer's tail wraps modulo cap; a tail near cap with a new write that straddles the wrap is valid (extend_from_within_unchecked handles it after reserve), but the default check rejected it as BackendOverflow. Removed the destination check from the default impl — reserve(len) + the per-backend invariant maintained by extend_from_within_unchecked is the contract growable backends already honor. Fixed-capacity backends (UserSliceBackend) override this method with their own non-wrap-aware capacity check, unchanged. 2. frame_decoder.rs:1541 — BackendOverflow -> FrameContentSizeMismatch conversion computed reported produced as with unchecked u64 +. This arm is reached precisely when block content exceeded backend capacity — exactly the case where adversarial decompressed_size (up to u32::MAX, plus produced up to whatever previous blocks wrote) could nominally overflow u64 itself. Using saturating_add caps the reported value at u64::MAX instead of wrapping; preserves defense-in-depth without panicking in debug builds. 3. user_slice_buf.rs:697 — try_extend_checked_add_wrap_returns_overflow test was misnamed. The checked_add wrap branch is unreachable from safe Rust (would require forging a slice with len near usize::MAX via from_raw_parts, which is UB). The test ended with try_extend(&[]).is_ok() asserting the zero-length happy path, not overflow. Renamed to try_extend_zero_length_succeeds_and_leaves_tail_unchanged, expanded to cover both tail=0 and tail>0 zero-length paths, and the doc comment now correctly states the wrap branch is fuzz-harness territory. 660/661 nextest pass on release. The single fail (bit_writer::catches_dirty_upper_bits) is a #[should_panic] test on debug_assert! and is pre-existing release-mode behaviour unrelated to this change.
- buffer_backend: scope wording to Raw/RLE; document try_extend_from_within
ring-aware semantics (no linear tail+len<=cap check in default impl)
- decode_buffer: try_push doc references Raw block fast path
- user_slice_buf: ok_or -> ok_or_else on try_* paths (lazy BackendOverflow
construction on success branch); trim test header
- frame_decoder: pattern BackendOverflow {..} ignoring step field
- user_slice_buf: ok_or_else -> ok_or on try_* paths. BackendOverflow is Copy with three usize fields; closure indirection is heavier than the struct construction it gates. Restores clippy::unnecessary_lazy_evaluations cleanliness. - buffer_backend: drop misleading "validate tail+len<=cap" from try_extend_from_within default-impl comment — wrap-aware RingBuffer intentionally skips the linear check; fixed-capacity backends override. - frame_decoder: rewrite saturating_add rationale to reflect BlockHeader.decompressed_size being u32, not u64. Overflow concern lives on accumulated produced (u64), not per-block size.
- block_decoder: add rle_oversized_against_user_slice_backend_returns_backend_overflow
exercising DecodeBlockContentError::BackendOverflow { step: RLE }
when a 10-byte RLE block targets a 4-byte UserSliceBackend slice.
Asserts no bytes are written on the failure branch (transactional
surface).
- buffer_backend: rewrite BackendOverflow doc to list all three
failure modes (destination capacity overshoot, arithmetic overflow
on checked_add, source-range violation in try_extend_from_within),
not only the capacity-overshoot case.
- buffer_backend: drop unsubstantiated "benches confirmed <=1%"
claim from the fallible-write surface comment; bench validation
is a pre-merge follow-up.
`decode_block_content_from_slice` advances `*source = &source[1..]` BEFORE calling the fallible `try_extend_and_fill`. When the write errors (BackendOverflow on fixed-capacity backends), 1 input byte was already consumed but the caller exits early, so the input cursor and the decoder's bytes_read accounting diverge. This commit adds the failing test that pins the contract — `*source` must remain unchanged when the RLE write fails. The fix lands in a separate commit so the bisect history shows the bug existed and was deliberately addressed.
Mirrors the Raw arm's split_at-then-try_push-then-advance ordering. On `UserSliceBackend` (fixed-capacity), an oversized RLE block returns `Err(BackendOverflow)`; without this fix, `*source` had already been moved forward by 1 byte, leaving the input cursor and the decoder's bytes_read accounting out of sync. Verified by the regression test added in the previous commit (`rle_overflow_leaves_source_unadvanced`), which now passes alongside the happy-path and empty-source assertions.
…w Display
Codecov flagged 27 missing lines in `buffer_backend.rs` (22.85% patch
coverage) and 4 missing in `errors.rs` (0%) — both belong to the
fallible-write surface added by this PR but unexercised by tests.
- buffer_backend: four #[cfg(test)] unit tests against `FlatBuf` (the
default-impl path; `UserSliceBackend` overrides):
- happy path: try_extend_from_within copies head bytes into tail
- arithmetic overflow: start.checked_add(len) wrap returns Err
without touching the backend
- source-range violation: start+len > self.len() returns Err
- BackendOverflow Display renders all three diagnostic fields
- errors: assert DecodeBlockContentError::BackendOverflow Display
for BlockType::RLE and Raw arms
All 5 tests PASS, clippy clean.
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
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.
Summary
First chunk of #246 (DoS-safe direct decode). Adds parallel fallible
try_*write surface toBufferBackend, plumbs RLE / Raw block pathsto use it, and wires the resulting
BackendOverflowinto the existingFrameDecoderError::FrameContentSizeMismatchso callers never see apanic on these paths even if the per-block pre-flight FCS check is
bypassed.
Closes part of #246. The Compressed-block sequence executor still
writes via the existing unchecked path; threading
try_*through thefused decode+execute pipeline is the remaining work — the
_trustedsuffix on
decode_to_slice_trustedstays until that path is hardenedtoo.
What lands
BackendOverflowerror type inbuffer_backend.rswith tail /requested / capacity diagnostics.
BufferBackend::try_extend,try_extend_and_fill,try_extend_from_withintrait methods. Default impls call theexisting panic-on-overflow variants — growable backends
(FlatBuf / RingBuffer) keep current behaviour because they cannot
fail for capacity reasons (the Vec grows on demand).
UserSliceBackendoverrides all three with explicit capacitychecks that return
Err(BackendOverflow)instead of panicking viathe release-mode
assert!.DecodeBuffer::try_pushandtry_extend_and_fillwrap thebackend methods and keep
total_output_counterbookkeepingconsistent on Ok.
BlockDecoder::decode_block_content_from_sliceRLE / Rawbranches switch to
try_extend_and_fill/try_pushand surfaceDecodeBlockContentError::BackendOverflow { step }on overshoot.FrameDecoder::decode_to_slice_trustedconverts that variantinto
FrameDecoderError::FrameContentSizeMismatch.What does NOT change
decode_allpath (FlatBuf / RingBuffer backends) — they grow ondemand and never hit
BackendOverflow. Default trait impls givethem zero behavioural change.
extend/extend_and_fill/extend_from_within_uncheckedpanic-on-overflow methods — stillpresent, still used by tests + non-direct paths. Removing them is
premature; they document the unchecked contract for the
sequence executor's eventual port.
the sequence-executor side of the refactor lands.
Test plan
cargo nextest run -p structured-zstd --features hash,std,dict_builder— 652/652 green.cargo clippy --all-targets --features hash,std -- -D warnings— clean.Follow-up (this PR doesn't claim to finish #246)
try_*throughexecute_sequences_fields/decode_and_execute_sequencesso the Compressed-block intra-blockoverflow surfaces structured-error instead of panic.
_trustedsuffix ondecode_to_sliceonce the Compressedpath is hardened.
compare_ffion i9 to confirm the new bounds-check overheadon
UserSliceBackendstays within the issue's ≤ 1 % regressionbound.
Summary by CodeRabbit