fix(bytesbuf): guard total capacity against overflow at growth sites#593
Open
sandersaares wants to merge 5 commits into
Open
fix(bytesbuf): guard total capacity against overflow at growth sites#593sandersaares wants to merge 5 commits into
sandersaares wants to merge 5 commits into
Conversation
BytesBuf's `capacity() == len + available` type invariant (`capacity <= usize::MAX`) was asserted but not enforced. The two capacity-growing sites each guarded only the component they grow: `extend_capacity_by_at_least` (reserve) checked `available` alone and `append` (put_bytes) checked `len` alone. Because appended shared memory lets the logical `len` grow independently of physical address-space consumption, `len + available` could exceed `usize::MAX` while neither component overflowed on its own, causing `capacity()`'s `wrapping_add` to silently return a wrapped value. This is reachable on 32-bit targets. Both growth sites now check the total `len + available` via a shared `assert_capacity_within_bounds` helper, before mutating any state, so the invariant the `# Panics` docs already promise is actually upheld. Regression coverage tests the helper directly with `usize`-boundary values; the overflow regime is unreachable end-to-end on 64-bit and cannot be forced by crafting field values without constructing an invalid buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15947dc4-48a0-44e0-b4af-00c1ae525e5f
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens bytesbuf::BytesBuf’s capacity accounting by enforcing the invariant that total capacity (len + available) never exceeds usize::MAX, preventing capacity()’s wrapping_add from silently producing wrapped values (notably reachable on 32-bit targets via shared-span aliasing + reserved capacity).
Changes:
- Introduces a shared
assert_capacity_within_bounds(len, available)guard for the total-capacity invariant. - Wires the guard into both growth sites:
extend_capacity_by_at_least(reserve path) andappend(put_bytes path), before mutating the relevant state. - Adds focused unit tests that exercise boundary and overflow cases for the helper.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Add `#[track_caller]` to `assert_capacity_within_bounds` so an overflow panic reports the actual growth site (`reserve`/`append`) rather than the helper, and include the `len`/`available` values in the panic message for easier diagnosis. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15947dc4-48a0-44e0-b4af-00c1ae525e5f
Reword `assert_capacity_within_bounds`'s doc: `capacity()` returns `usize`, so "`capacity() <= usize::MAX`" was tautological. State the real invariant instead - the true (non-wrapping) sum `len + available` must fit in `usize` so `capacity()` does not wrap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15947dc4-48a0-44e0-b4af-00c1ae525e5f
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/bytesbuf/src/buf.rs:137
- The new helper/doc explains the real invariant (non-wrapping
len + available), but there are still internal comments elsewhere in this file that say "capacity <= usize::MAXis a type invariant" (e.g.,capacity()andcalculate_len()), which is tautological and no longer matches what’s actually being enforced. Please update those comments to refer to the checkedlen + remaining_capacityinvariant (guarded byassert_capacity_within_boundsat the two growth sites) so future readers don’t get misled.
/// Panics if the true (non-wrapping) sum `len + available` would overflow `usize`.
///
/// `capacity()` returns `len + available`, so that sum must fit in `usize` for `capacity()` not to
/// wrap. The two capacity-growing sites (`reserve` and `append`) each grow only one of `len` or
/// `available`. A bounds check on the grown component alone is insufficient: `len` can grow via
/// appended shared memory while `available` holds separately reserved capacity, so only their sum
/// is bounded.
…overflow guard Reword the internal comments that described a tautological "`capacity <= usize::MAX` is a type invariant" (in `capacity()`, `calculate_len()`, the consume span-count loop, `ConsumeManifest::required_spans_capacity`, and the remaining-capacity iterator) to state the real, enforced invariant: the true (non-wrapping) sum `len + available` never overflows `usize`, guarded at the growth sites by `assert_capacity_within_bounds`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15947dc4-48a0-44e0-b4af-00c1ae525e5f
Vaiz
approved these changes
Jul 22, 2026
kate-shine
approved these changes
Jul 22, 2026
Move the capacity-overflow check in `append` ahead of `freeze_from_first` so a rejected append leaves the buffer untouched, as the docs promise. Freezing changes neither `len` nor `available`, so the check is equivalent before or after it, but running it first ensures the panic path performs no mutation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15947dc4-48a0-44e0-b4af-00c1ae525e5f
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 359 359
Lines 27800 27808 +8
=======================================
+ Hits 27800 27808 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ralfbiedert
reviewed
Jul 23, 2026
| /// `available`. A bounds check on the grown component alone is insufficient: `len` can grow via | ||
| /// appended shared memory while `available` holds separately reserved capacity, so only their sum | ||
| /// is bounded. | ||
| #[track_caller] |
ralfbiedert
approved these changes
Jul 23, 2026
ralfbiedert
approved these changes
Jul 24, 2026
ralfbiedert
left a comment
ralfbiedert
left a comment
Collaborator
There was a problem hiding this comment.
Approved. The overflow checks cover the actual total-capacity growth sites, execute before mutation, and do not affect the byte-write hot path. Nearby unsafe copying invariants are unchanged.
geeknoid
approved these changes
Jul 24, 2026
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.
[Copilot speaking]
What
BytesBuf's type invariant that totalcapacity() == len + availablenever exceedsusize::MAXwas asserted (relied upon bycapacity()'swrapping_add) but not actually enforced. The two capacity-growing sites each guarded only the single component they grow:extend_capacity_by_at_least(thereservepath) checked only thatavailabledid not overflow.append(theput_bytespath) checked only thatlendid not overflow.Appending an existing
BytesViewreuses shared, immutable memory that can be aliased by multiple views, so the logicallencan grow independently of physical address-space consumption. As a resultlen + availablecould passusize::MAXwhile neither component overflowed on its own, andcapacity()would silently return a wrapped value, violating internal accounting assumptions. This is reachable on 32-bit targets, where the address space is small enough that shared-span aliasing plus reserved capacity can push the sum pastusize::MAX.Fix
Both growth sites now validate the total
len + available(not just the grown component) through a shared privateassert_capacity_within_boundshelper, and do so before mutating any state, so a rejected grow leaves the buffer untouched. The# Panicsdocs onreserve/put_bytesalready promised the total buffer capacity would be guarded, so this makes the code match the contract and makescapacity()'s invariant comment true.Testing
capacity_bounds_check_*tests exerciseassert_capacity_within_boundsdirectly atusizeboundaries (usize::MAX,usize::MAX - 4plus8,usize::MAX/2 + 1twice). The overflow regime is unreachable end-to-end on 64-bit (you cannot hold2^64bytes, even aliased), and forcing it by craftinglen/availablefield values would construct an invalidBytesBuf, so the pure-function test is the honest way to lock in the overflow logic. The check-site wiring remains covered by the existing normal-pathappend/reservetests plus review.Verified locally:
cargo test -p bytesbuf,cargo clippy -p bytesbuf --all-targets,just format, andjust spellcheckall clean.