Conversation
`can_fit_tracked_in_chunk_segment` admitted a layout whenever `size + size_of::<TrackingHeader>() + (align - 1)` fitted `BUMP_SEGMENT_SIZE`, ignoring the allocator-owned prefix that begins every chunk segment. `allocate_tracked` places the user pointer after that prefix, so requests in the resulting gap were admitted, never fitted, and drove the retry loop to map a fresh 64 KiB chunk on every attempt until the process exhausted memory. The window is reachable from safe code with the default options, because `max_allocation_bytes` defaults to `BUMP_SEGMENT_SIZE`: a `Vec` grown to roughly 32.7 KiB inside `with_hint(&Heap::bump(bump::Options::new()), ..)` is enough. Probing each size in its own process with a fresh bump heap put the defect at exactly 32_705..=32_713 for 16-byte alignment; sizes through 32_704 were served by a chunk's roomier second segment and sizes from 32_714 were already rejected and fell back to the general heap. Model the placement `allocate_tracked` actually performs, keyed to a fresh normal chunk's first segment. That segment is the tightest one the retry loop can be forced onto, so an admitted layout now always fits within a bounded number of advances, and the largest requests fall back to the general heap. A debug assertion states the invariant directly instead of silently capping retries, so a future arithmetic regression fails loudly rather than hanging. Add hardening coverage for the allocator: payload, alignment, reallocation and cross-thread invariants, arena lifecycles, private region and failure paths, real container workloads, and a bounded operation-trace model shared by a seeded soak and a Bolero fuzz target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fb1df27a-b81e-4982-ad7d-42cfeee3fde3
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let heap = hal::map(size_of::<ReusableHeapState>()).cast::<ReusableHeapState>(); | ||
| assert!(!heap.is_null()); | ||
| unsafe { | ||
| heap.write(ReusableHeapState::new(options, domain)); |
| let allocator = unsafe { Rallocator::<Standard>::new() }; | ||
| let domain = new_domain_state(); | ||
| let heap = new_retirable_heap(domain, GeneralOptions::from_values(MEDIUM_SLICE_SIZE, MEDIUM_SLICE_SIZE)); | ||
| let heap_ref = unsafe { &mut *heap }; |
| assert!(!direct.is_null()); | ||
| assert_eq!(domain_used_slices(domain), 2); | ||
| unsafe { | ||
| direct.write(0xDA); |
| let domain = new_domain_state(); | ||
| let heap = new_retirable_heap(domain, GeneralOptions::from_values(MEDIUM_SLICE_SIZE, 0)); | ||
| let layout = Layout::from_size_align(MEDIUM_SLICE_SIZE, 16).unwrap(); | ||
| let live = allocator.allocate_medium(layout, unsafe { &mut *heap }, None); |
| let live = allocator.allocate_medium(layout, unsafe { &mut *heap }, None); | ||
| assert!(!live.is_null()); | ||
| unsafe { | ||
| live.write(0xA5); |
| let medium_layout = Layout::from_size_align(MEDIUM_SLICE_SIZE, 16).unwrap(); | ||
| let direct_layout = Layout::from_size_align(96, MAX_MEDIUM_ALIGNMENT * 2).unwrap(); | ||
|
|
||
| let medium = allocator.allocate_medium(medium_layout, unsafe { &mut *heap }, None); |
| assert!(!medium.is_null()); | ||
| assert!(!direct.is_null()); | ||
| unsafe { | ||
| medium.write(0xC3); |
| unsafe { | ||
| medium.write(0xC3); | ||
| medium.add(MEDIUM_SLICE_SIZE - 1).write(0x3C); | ||
| direct.write(0xD1); |
|
|
||
| unsafe { retire_general_heap(heap) }; | ||
| assert!(slice_marked(region, medium_slice)); | ||
| assert_eq!(unsafe { medium.read() }, 0xC3); |
| assert!(slice_marked(region, medium_slice)); | ||
| assert_eq!(unsafe { medium.read() }, 0xC3); | ||
| assert_eq!(unsafe { medium.add(MEDIUM_SLICE_SIZE - 1).read() }, 0x3C); | ||
| assert_eq!(unsafe { direct.read() }, 0xD1); |
There was a problem hiding this comment.
🔵 Needs a closer look
Extensive allocator and hardening changes warrant final human review.
Pull request overview
Fixes tracked bump allocations that could retry indefinitely by accounting for chunk-segment prefixes, with extensive allocator hardening coverage.
Changes:
- Corrects tracked allocation bounds and adds a debug invariant.
- Adds lifecycle, alignment, reallocation, concurrency, and container tests.
- Adds bounded scenario testing and Bolero fuzz coverage.
File summaries
| File | Reviewed changes |
|---|---|
crates/rallocator/tests/scenarios/mod.rs |
Adds bounded mixed-heap allocation scenarios. |
crates/rallocator/tests/hardening.rs |
Adds boundary, lifecycle, concurrency, and soak tests. |
crates/rallocator/tests/global_hardening.rs |
Adds global container workload coverage. |
crates/rallocator/tests/common/mod.rs |
Adds allocation payload and ownership helpers. |
crates/rallocator/tests/bolero_allocator.rs |
Adds fuzz-driven allocation sequence testing. |
crates/rallocator/tests/arena_hardening.rs |
Adds arena lifecycle and retention tests. |
crates/rallocator/src/heap/bump/state.rs |
Fixes tracked bump admission bounds and adds regression coverage. |
crates/rallocator/src/allocator/hardening.rs |
Adds internal allocator lifecycle and failure-path tests. |
crates/rallocator/src/allocator.rs |
Registers allocator hardening tests. |
crates/rallocator/Cargo.toml |
Adds hardening-test dependencies. |
Cargo.lock |
Records the new dependency versions. |
Review details
Suppressed comments (2)
crates/rallocator/tests/arena_hardening.rs:237
- After
drop(heap)at line 243, the loop at lines 247–249 callsBlock::reallocatewithoutwith_hint;Block::reallocateforwards directly toRallocator::realloc(tests/common/mod.rs:90-105), and the allocation path only consults the active hint (src/allocator.rs:1683-1692). Thus the first growth leaves the bump heap, but the later sizes remain on the general heap—they never move “back” into the arena. Please correct the comment or keep the hint active if a round trip is intended, otherwise the claimed coverage is misleading.
// A bump-backed allocation escapes its dropped heap handle and is then
// reallocated. Growing past the 32 KiB bump maximum moves it out of the
// arena into the general heap and back; the preserved prefix must survive
// every move. `Block::reallocate` verifies the preserved prefix before
// initializing any extension.
crates/rallocator/tests/global_hardening.rs:32
rallocator::rallocator!()above installsGlobalRallocatoras this binary's#[global_allocator], soSystemis not the process allocator here. The test'sVec,Arc, and other bookkeeping allocations therefore do run through rallocator and can affect its counters/hint state; please update this safety rationale to describe the intentional global-allocator coverage instead of claiming the opposite.
}
fn check(&self, id: usize) {
- Files reviewed: 10/11 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (99.9%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #756 +/- ##
========================================
- Coverage 100.0% 99.9% -0.1%
========================================
Files 634 635 +1
Lines 84746 85116 +370
========================================
+ Hits 84746 85114 +368
- Misses 0 2 +2
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:
|
| .and_then(|size| size.checked_add(layout.align().max(align_of::<TrackingHeader>()) - 1)) | ||
| .is_some_and(|size| size <= BUMP_SEGMENT_SIZE) | ||
| let alignment = layout.align().max(align_of::<TrackingHeader>()); | ||
| align_up(size_of::<BumpChunk>(), 16) |
There was a problem hiding this comment.
🤖: Non-blocking — The segment-prefix offset remains independently encoded in admission and placement, so a later change to chunk_start can again admit a layout that never fits and repeatedly maps chunks.
Please share the prefix-alignment constant and first-segment offset calculation between chunk_start, both admission predicates, and the test oracle. Today can_fit_tracked_in_chunk_segment, can_fit_in_chunk_segment, and tracked_first_segment_capacity each repeat align_up(size_of::<BumpChunk>(), 16), while chunk_start computes the real pointer separately. The 32_688 assertion pins today's copied model, but it would not fail if placement alone changed; a shared helper would make the admission/placement invariant mechanical and leave the tests as a backstop.
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Complete Review Lens coverage found two non-blocking naming and maintainability issues. Existing unresolved discussion and failing exact-head CodeQL and Codecov checks mean this is not a clean report.
Coverage included API design, correctness, tests, performance, naming, telemetry, resilience, consistency, public API, and public documentation across all 11 changed files. The paired public API and rustdoc comparisons found no public-surface changes.
Existing unresolved items remain around independently encoded segment-prefix arithmetic, two inaccurate safety/lifecycle comments, 14 open high-severity CodeQL invalid-pointer alerts, and two uncovered changed lines reported by Codecov.
Material limitations: Miri was unavailable locally (the exact-head runtime-analysis CI passed); rustdoc coverage used default features on the host target; concurrent benchmark runs were not a paired Criterion comparison.
| // the rallocator counters or inadvertently consume active allocation hints. | ||
| static ALLOCATOR: rallocator::Rallocator = unsafe { rallocator::Rallocator::new() }; | ||
| static TEST_LOCK: Mutex<()> = Mutex::new(()); | ||
| const MAX_SCENARIO_LIVE_BLOCKS: usize = if cfg!(miri) { 4 } else { 16 }; |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
The scenario live-set limit has two names and definitions
Problem
MAX_SCENARIO_LIVE_BLOCKS duplicates scenarios::MAX_LIVE, including their cfg!(miri) values.
Why this matters
Changing one definition without the other silently invalidates scenario_backing_retention_bound, weakening the soak retention check.
Suggested fix
Expose one scenarios::MAX_LIVE_BLOCKS constant and use it in both the scenario runner and bound calculation.
| pub(crate) const INPUT_BYTES: usize = 1 + 6 * MAX_OPERATIONS; | ||
| const MAX_LIVE: usize = if cfg!(miri) { 4 } else { 16 }; | ||
|
|
||
| pub(crate) struct Recording; |
There was a problem hiding this comment.
Posted by an AI agent · Nit
The recording scope diverges from the crate’s guard naming
Problem
Recording changes global recorder configuration and restores it through Drop, while the sibling RAII type is named RecordingGuard.
Why this matters
The current name reads like captured data and obscures the type's cleanup behavior.
Suggested fix
Rename it to RecordingGuard and update its three consumers.
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Reviewed the complete pinned allocator-hardening change at 0ed0a2172322c04f269d9e72acca8d2a74f7b558 across all ten required areas. I found one new non-blocking regression-coverage gap: tracked bump admission is tested only through 4 KiB alignment even though the internal option supports alignments through 32 KiB. Existing discussions about duplicated segment-prefix arithmetic, scenario live-set constants, guard naming, the PR test-count claim, and arena reallocation wording remain applicable and were not duplicated.
The exact-head default-feature rallocator suite and targeted hardening tests passed. Default-feature host public API captures were byte-identical and public rustdoc had no changes. CodeQL remains red with 14 invalid-pointer annotations concentrated in allocator hardening/test paths, and Codecov reports two uncovered lines in allocator/hardening.rs; the required CI owners still need to disposition those checks.
| assert_eq!(size_of::<TrackingHeader>(), 40); | ||
| assert_eq!(tracked_first_segment_capacity(16), 32_688); | ||
|
|
||
| for &align in &[1_usize, 16, 64, 4096] { |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
Tracked admission tests omit supported high-alignment boundaries
Problem
This matrix stops at 4 KiB, while with_max_alignment accepts 8, 16, and 32 KiB and the first-segment capacity changes with alignment. The integration tests do not distinguish bump allocation from fallback to the general heap.
Why this matters
A regression in high-alignment admission could remain hidden because payload correctness still holds after fallback.
Suggested fix
Cover 8, 16, and 32 KiB at the exact first-segment capacity and one byte beyond it, and include those admitted layouts in the bounded chunk-growth test.
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
A fresh complete Review Lens pass at the pinned head found no additional findings. Existing unresolved discussion remains around shared segment-prefix arithmetic, scenario live-set constants, guard naming, high-alignment admission coverage, and two inaccurate test comments; those were not duplicated.
All ten required areas completed. The paired public API and rustdoc comparisons found no public-surface changes, and cargo check -p rallocator passed. Exact-head CodeQL still reports 14 invalid-pointer annotations, while Codecov reports two uncovered changed lines.
Summary
can_fit_tracked_in_chunk_segmentadmitted a bump layout wheneversize + size_of::<TrackingHeader>() + (align - 1)fittedBUMP_SEGMENT_SIZE, ignoring the allocator-owned prefix that begins every chunk segment.allocate_trackedplaces the user pointer after that prefix, so requests landing in the gap were admitted, never fitted, and drove the retry loop to map a fresh 64 KiB chunk on every attempt until the process exhausted memory.This is reachable from safe code with the default options, because
max_allocation_bytesdefaults toBUMP_SEGMENT_SIZE:The test-only
allocatealready usedcan_fit_in_chunk_segment, which does account forsize_of::<BumpChunk>(), so only the tracked production variant was affected.Defect window
Each size was probed in its own process with a fresh bump heap, under an external memory/time supervisor, with the original predicate in place:
That window is nine bytes wide, which is why uniform sampling had not reached it.
Fix
Model the placement
allocate_trackedactually performs, keyed to a fresh normal chunk's first segment. Becauseadvance_chunkyields either a chunk's second segment (a 16-byte prefix) or a new/recycled normal chunk reset throughchunk_start(a 32-byte aligned prefix), the first segment is the tightest placement the retry loop can be forced onto. An admitted layout therefore always fits within a bounded number of advances, and the largest requests fall back to the general heap. The measurable cost is at most 16 bytes of bump capability at the very top of the range.A
debug_assert!states the invariant directly rather than silently capping retries, so future arithmetic drift fails loudly instead of hanging. Restoring the defective predicate makes that assertion fire in 0.00 s, which was verified by fault injection.Hardening coverage
Adds 78 tests: payload, alignment, reallocation and cross-thread invariants; arena lifecycles including attachment-cache eviction and escaped allocations; private region, retirement and failure-injection paths; real container workloads through the installed global allocator; and a bounded operation-trace model shared by a seeded soak and a Bolero fuzz target.
Test design follows practices taken from mimalloc (cookie-stamped payloads verified at free), jemalloc (packing and accounting-based retention rather than RSS), snmalloc (live-set uniqueness, dirty-then-zero
calloc), tcmalloc (realloc pattern preservation across size-class boundaries) and bumpalo (operation model with overlap and containment invariants). Boundary sizes around the chunk segment capacity are enumerated explicitly, since a nine-byte window is not reachable by sampling.Verification
cargo test -p rallocator --all-features --lockedcargo clippy --all-targets --all-features -- -D warningscargo fmt --checkhardening,arena_hardening,global_hardening)The soak returns
live_bytesto baseline on every seed andmapped_bytesplateaus at 4 784 128 bytes from roughly seed 3 840, against a derived ceiling of 105 218 048 bytes. Seed 74, which originally consumed 741 691 392 bytes of private commit, now peaks at 286 720 bytes.seeded_soakis#[ignore]d and opt-in throughRALLOCATOR_STRESS_SEEDSandRALLOCATOR_STRESS_START_SEED; any scenario panic reports the exact seed to replay.Limitations
Systemallocations, recorder buffers or virtual address reservations.