From 5a28f1459c10522ac4abbb214d891c1abebc63ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 30 Jul 2026 11:12:46 +0200 Subject: [PATCH 1/2] gc: reserve arena blocks with no arena borrow live (#7051) #7050 moved the allocation-point GC trigger out of Arena::alloc's &mut self borrow. The emergency-reclaim path kept the same shape: alloc_after_gc -> alloc_fresh_block -> install_fresh_block -> alloc_block, all under &mut self, and alloc_block calls gc_try_emergency_reclaim() when the OS refuses memory. That collection allocates into the arenas, so self.blocks.push(..) could grow the Vec underneath the live borrow -- the same aliasing violation, on the path that runs under heap exhaustion. Splits block acquisition in two: reserve_arena_block(min_size) -- may collect; NO arena borrow may be live across it. Used by arena_cell_alloc, which already runs borrow-free at that point. alloc_block_no_gc(min_size) -- never collects; for the callers that hold a live borrow, two of which are already executing inside a collection. Arena::install_reserved_block installs a block reserved by the former. The cfg(test) borrow-depth probe from #7050 now covers this path too, and try_alloc_block gains an injectable failure hook so a test can force the null-alloc branch that would otherwise need real heap exhaustion. Refs #7051, #7022. --- crates/perry-runtime/src/arena/block.rs | 154 +++++++++++++++++++----- crates/perry-runtime/src/arena/mod.rs | 3 +- crates/perry-runtime/src/arena/tests.rs | 33 +++++ 3 files changed, 160 insertions(+), 30 deletions(-) diff --git a/crates/perry-runtime/src/arena/block.rs b/crates/perry-runtime/src/arena/block.rs index 8ccf087d78..0fe5ae523a 100644 --- a/crates/perry-runtime/src/arena/block.rs +++ b/crates/perry-runtime/src/arena/block.rs @@ -43,35 +43,82 @@ pub(crate) const BLOCK_SIZE: usize = 1024 * 1024; pub(crate) const FRESH_GENERAL_BLOCK_MIN_USED_BYTES: usize = 256 * 1024; /// Create a block of at least the given size (for oversized allocations) -fn alloc_block(min_size: usize) -> ArenaBlock { - let size = if min_size <= BLOCK_SIZE { +#[inline] +fn block_size_for(min_size: usize) -> usize { + if min_size <= BLOCK_SIZE { BLOCK_SIZE } else { // Round up to next multiple of BLOCK_SIZE min_size.div_ceil(BLOCK_SIZE) * BLOCK_SIZE - }; + } +} + +/// One raw block allocation. NEVER collects, so it is safe under an `&mut +/// Arena` borrow. `injectable` marks the single call site a test is allowed to +/// force a failure at (see [`force_next_block_alloc_failure`]) — the collection +/// that `reserve_arena_block` then runs allocates blocks of its own through the +/// non-injectable path, so the injected refusal cannot be consumed by the wrong +/// allocation. +fn try_alloc_block(min_size: usize, injectable: bool) -> Option { + let size = block_size_for(min_size); let layout = Layout::from_size_align(size, 16).unwrap(); - let mut data = unsafe { alloc(layout) }; - if data.is_null() { - // The OS refused memory. Try one emergency full collection — - // idle-block dealloc and malloc sweep can return real pages — - // then retry once before giving up. - if crate::gc::gc_try_emergency_reclaim() { - data = unsafe { alloc(layout) }; - } + #[cfg(test)] + if injectable && FORCE_BLOCK_ALLOC_FAILURE.with(|f| f.replace(false)) { + return None; } + #[cfg(not(test))] + let _ = injectable; + let data = unsafe { alloc(layout) }; if data.is_null() { - panic!( - "Failed to allocate arena block of {} bytes (heap exhausted after emergency GC)", - size - ); + return None; } - ArenaBlock { + Some(ArenaBlock { data, size, offset: 0, dead_cycles: 0, + }) +} + +/// Reserve a block, running one emergency full collection if the OS refuses +/// memory — idle-block dealloc and the malloc sweep can return real pages. +/// +/// **NO `&mut Arena` BORROW MAY BE LIVE ACROSS THIS CALL (#7022).** The +/// emergency collection allocates into the arenas exactly like the +/// allocation-point trigger does, so it can grow `self.blocks` underneath a +/// borrow held by the frame that is extending the arena — the same aliasing +/// violation [`arena_cell_alloc`] exists to avoid, on the out-of-memory path. +/// `arena_cell_alloc` is the only caller; every `&mut self` path uses +/// [`alloc_block_no_gc`]. +pub(crate) fn reserve_arena_block(min_size: usize) -> ArenaBlock { + if let Some(block) = try_alloc_block(min_size, true) { + return block; + } + note_gc_trigger_arena_borrow_depth(); + crate::gc::gc_try_emergency_reclaim(); + if let Some(block) = try_alloc_block(min_size, false) { + return block; } + panic!( + "Failed to allocate arena block of {} bytes (heap exhausted after emergency GC)", + block_size_for(min_size) + ); +} + +/// Block allocation for the `&mut self` paths — `Arena::alloc`, the C4b +/// evacuation path's `alloc_excluding_pages`, and `arena_start_fresh_general_block`. +/// Deliberately does NOT run the emergency reclaim: those callers hold a live +/// arena borrow, and two of the three are already executing inside a collection, +/// where starting another one is precisely what must not happen. The mutator +/// allocation path — the one where heap exhaustion actually surfaces — keeps the +/// reclaim via [`reserve_arena_block`]. +fn alloc_block_no_gc(min_size: usize) -> ArenaBlock { + try_alloc_block(min_size, false).unwrap_or_else(|| { + panic!( + "Failed to allocate arena block of {} bytes (heap exhausted)", + block_size_for(min_size) + ) + }) } /// A single arena block @@ -91,8 +138,10 @@ pub(crate) struct ArenaBlock { } impl ArenaBlock { + /// The initial block of a thread's arena, built during `Arena::new` — i.e. + /// while the arena does not exist yet, so nothing may collect here. fn new() -> Self { - alloc_block(BLOCK_SIZE) + alloc_block_no_gc(BLOCK_SIZE) } /// Try to allocate within this block, respecting alignment @@ -293,8 +342,14 @@ impl Arena { }); } + /// Reserve **and** install a block. Never collects — see + /// [`alloc_block_no_gc`] for why. pub(crate) fn install_fresh_block(&mut self, size: usize) { - let fresh = alloc_block(size); + self.install_reserved_block(alloc_block_no_gc(size)); + } + + /// Install a block that was reserved with no arena borrow live (#7022). + pub(crate) fn install_reserved_block(&mut self, fresh: ArenaBlock) { let fresh_size = fresh.size; let fresh_base = fresh.data as usize; register_block_space(fresh_base, fresh_size, self.generation, self.space); @@ -353,11 +408,29 @@ impl Arena { /// one. Split out of `alloc` for #7022 — see [`arena_cell_alloc`]. #[inline] pub(crate) fn alloc_after_gc(&mut self, size: usize, align: usize) -> *mut u8 { + if let Some(ptr) = self.try_alloc_after_gc(size, align) { + return ptr; + } + // Still no room anywhere — need a fresh block. C4b-δ: + // prefer reusing a tombstoned slot (a block deallocated by + // `arena_reset_empty_blocks` after staying idle past the + // dealloc threshold) over growing the Vec, so block_idx + // semantics stay bounded even on workloads that churn + // through nursery blocks. + self.alloc_fresh_block(size, align) + } + + /// The part of [`Self::alloc_after_gc`] that can be satisfied from blocks + /// the arena already owns. Returns `None` when a fresh block is needed — + /// which `arena_cell_alloc` reserves with no borrow live, because that + /// reservation can collect (#7022). + #[inline] + pub(crate) fn try_alloc_after_gc(&mut self, size: usize, align: usize) -> Option<*mut u8> { // Retry the (possibly newly-reset) current block. arena.current // may have been changed by arena_reset_empty_blocks to point // at the lowest reset block. if let Some(ptr) = self.try_block_alloc(self.current, size, align) { - return ptr; + return Some(ptr); } // Scan forward for any other block with space — the GC may @@ -372,17 +445,10 @@ impl Arena { self.current = i; // Resync inline state to the new current block. self.resync_inline_to_current(); - return ptr; + return Some(ptr); } } - - // Still no room anywhere — need a fresh block. C4b-δ: - // prefer reusing a tombstoned slot (a block deallocated by - // `arena_reset_empty_blocks` after staying idle past the - // dealloc threshold) over growing the Vec, so block_idx - // semantics stay bounded even on workloads that churn - // through nursery blocks. - self.alloc_fresh_block(size, align) + None } /// GC-free allocation. Used by paths that already run inside a collection @@ -476,8 +542,26 @@ pub(crate) unsafe fn arena_cell_alloc(arena: *mut Arena, size: usize, align: usi note_gc_trigger_arena_borrow_depth(); crate::gc::gc_check_trigger(); + { + let _borrow = ArenaBorrowGuard::new(); + if let Some(ptr) = (*arena).try_alloc_after_gc(size, align) { + return ptr; + } + } + + // A fresh block is needed. `reserve_arena_block` runs an emergency full + // collection when the OS refuses memory, and that collection allocates into + // the arenas exactly like the trigger above — so it gets the same treatment: + // NO ARENA BORROW IS LIVE HERE either. (CodeRabbit caught this second path + // on the first cut of #7022, where the reservation still happened inside + // `alloc_fresh_block` under the borrow.) + let fresh = reserve_arena_block(size); + let _borrow = ArenaBorrowGuard::new(); - (*arena).alloc_after_gc(size, align) + (*arena).install_reserved_block(fresh); + (*arena) + .try_alloc_current(size, align) + .expect("freshly installed block should have space") } // --------------------------------------------------------------------------- @@ -492,6 +576,10 @@ pub(crate) unsafe fn arena_cell_alloc(arena: *mut Arena, size: usize, align: usi #[cfg(test)] thread_local! { + /// One-shot: make the next *injectable* block allocation report failure, so + /// a test can drive `reserve_arena_block`'s emergency-reclaim path without + /// needing the OS to actually refuse memory. + static FORCE_BLOCK_ALLOC_FAILURE: Cell = const { Cell::new(false) }; /// Number of `&mut Arena` borrows currently live inside `arena_cell_alloc`. static ARENA_BORROW_DEPTH: Cell = const { Cell::new(0) }; /// `ARENA_BORROW_DEPTH` sampled immediately before the most recent @@ -544,8 +632,16 @@ pub(crate) fn gc_trigger_arena_calls() -> u32 { GC_TRIGGER_CALLS.with(Cell::get) } +/// Arm the one-shot block-allocation failure consumed by +/// `reserve_arena_block`'s first attempt. +#[cfg(test)] +pub(crate) fn force_next_block_alloc_failure() { + FORCE_BLOCK_ALLOC_FAILURE.with(|f| f.set(true)); +} + #[cfg(test)] pub(crate) fn reset_gc_trigger_arena_probe() { + FORCE_BLOCK_ALLOC_FAILURE.with(|f| f.set(false)); GC_TRIGGER_BORROW_DEPTH.with(|d| d.set(u32::MAX)); GC_TRIGGER_CALLS.with(|c| c.set(0)); } diff --git a/crates/perry-runtime/src/arena/mod.rs b/crates/perry-runtime/src/arena/mod.rs index 1340a42701..b8adae9307 100644 --- a/crates/perry-runtime/src/arena/mod.rs +++ b/crates/perry-runtime/src/arena/mod.rs @@ -33,7 +33,8 @@ pub(crate) use block::{ }; #[cfg(test)] pub(crate) use block::{ - gc_trigger_arena_borrow_depth, gc_trigger_arena_calls, reset_gc_trigger_arena_probe, + force_next_block_alloc_failure, gc_trigger_arena_borrow_depth, gc_trigger_arena_calls, + reset_gc_trigger_arena_probe, }; pub(crate) use page_meta::{ address_span_overlaps_pages, register_block_space, register_old_object_pages, diff --git a/crates/perry-runtime/src/arena/tests.rs b/crates/perry-runtime/src/arena/tests.rs index d30a26b20c..a404b6d688 100644 --- a/crates/perry-runtime/src/arena/tests.rs +++ b/crates/perry-runtime/src/arena/tests.rs @@ -1070,3 +1070,36 @@ fn raw_arena_alloc_method_never_reaches_the_gc_trigger() { (#7022). The trigger belongs in `arena_cell_alloc`." ); } + +#[test] +fn emergency_block_reclaim_runs_with_no_live_arena_borrow() { + // The out-of-memory path is the second place a trigger can fire from an + // arena allocation: `reserve_arena_block` runs `gc_try_emergency_reclaim()` + // when the OS refuses memory, and that collection allocates into the arenas + // exactly like `gc_check_trigger` does. It gets the same rule. Driven here + // by a one-shot injected allocation failure rather than real heap + // exhaustion, so the invariant is asserted rather than assumed. + reset_gc_trigger_arena_probe(); + force_next_block_alloc_failure(); + // Bigger than any existing block, so a fresh block — and therefore + // `reserve_arena_block` — is unavoidable. + let ptr = OLD_ARENA.with(|a| unsafe { arena_cell_alloc(a.get(), BLOCK_SIZE + 1, 8) }); + assert!( + !ptr.is_null(), + "the emergency retry must still hand back a usable block" + ); + assert!( + gc_trigger_arena_calls() >= 2, + "the run must have reached BOTH the allocation-point trigger and the \ + emergency reclaim ({} trigger(s) seen); without the second one this \ + test asserts nothing", + gc_trigger_arena_calls() + ); + assert_eq!( + gc_trigger_arena_borrow_depth(), + 0, + "gc_try_emergency_reclaim() ran while an `&mut Arena` borrow was live — \ + the emergency collection allocates into this same arena and may \ + reallocate its `blocks` Vec underneath the borrow (#7022)" + ); +} From 25b0840424fb2ba35ef3b3e1f1987ad4adda60c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 30 Jul 2026 11:14:43 +0200 Subject: [PATCH 2/2] changelog: fragment for #7052 --- changelog.d/7052-arena-block-reservation.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 changelog.d/7052-arena-block-reservation.md diff --git a/changelog.d/7052-arena-block-reservation.md b/changelog.d/7052-arena-block-reservation.md new file mode 100644 index 0000000000..b13ad0685d --- /dev/null +++ b/changelog.d/7052-arena-block-reservation.md @@ -0,0 +1,19 @@ +Closed the second instance of the #7022 aliasing violation. #7050 moved the +allocation-point GC trigger out of `Arena::alloc`'s `&mut self` borrow, but the +emergency-reclaim path kept the same shape: `alloc_after_gc` → +`alloc_fresh_block` → `install_fresh_block` → `alloc_block`, all under `&mut self`, +with `alloc_block` calling `gc_try_emergency_reclaim()` when the OS refuses +memory. That collection allocates into the arenas, so `self.blocks.push(..)` could +grow the `Vec` underneath the live borrow — the same silent memory corruption, +reachable only under heap exhaustion, which is when it is least survivable. + +Block acquisition is now split by whether collecting is permitted: +`reserve_arena_block` may collect and requires that no arena borrow is live; +`alloc_block_no_gc` never collects and serves the callers that hold one (two of +which are already executing inside a collection). `Arena::install_reserved_block` +installs a block obtained from the former. + +The `cfg(test)` borrow-depth probe added in #7050 now covers this path as well, +and `try_alloc_block` gained an injectable failure hook so the null-allocation +branch can be exercised deliberately instead of waiting for real heap exhaustion — +the invariant is asserted rather than assumed.