fix(runtime): throw catchable RangeError on over-large allocations (#5067)#5103
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR converts allocation-failure panics to catchable ChangesAllocation Failure Handling and Bounds Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/map.rs (1)
756-759:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGrowth-path
reallocfailures still panic, leaving constructor-reachable abort paths.
Initial allocation paths were updated, but both growth paths still callpanic!on nullrealloc, so oversized constructor-driven population can still terminate the process.
crates/perry-runtime/src/map.rs#L756-L759: replace thepanic!("Failed to grow map entries")path withcrate::error::throw_allocation_failed().crates/perry-runtime/src/set.rs#L541-L545: replace thepanic!("Failed to grow set elements")path withcrate::error::throw_allocation_failed().🤖 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 `@crates/perry-runtime/src/map.rs` around lines 756 - 759, Replace the panic paths on realloc failure with the runtime allocation failure handler: in crates/perry-runtime/src/map.rs (lines 756-759) change the branch that currently does panic!("Failed to grow map entries") so it instead calls crate::error::throw_allocation_failed(); and in crates/perry-runtime/src/set.rs (lines 541-545) change the branch that currently does panic!("Failed to grow set elements") so it instead calls crate::error::throw_allocation_failed(); no other changes required — keep existing realloc and null-check logic but invoke throw_allocation_failed() in both places.
🤖 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 `@crates/perry-runtime/src/buffer/header.rs`:
- Around line 545-546: buffer_alloc_small currently panics when a slab block
allocation returns null (the panic! at the slab-allocation branch); change that
branch in function buffer_alloc_small to handle allocation failure the same way
as the non-slab path by calling throw_buffer_alloc_failed() (or otherwise
returning/propagating the catchable RangeError) instead of panic! so small
Buffer.alloc/Uint8Array calls surface a RangeError and do not abort the process;
update any immediate return value or control flow in buffer_alloc_small to match
the non-slab error path after invoking throw_buffer_alloc_failed().
In `@crates/perry-runtime/src/buffer/validate.rs`:
- Around line 91-104: The check currently rejects inputs like 2147483647.9
because it compares the original f64 `n` to `i32::MAX`, but Buffer.alloc should
truncate fractional sizes toward zero first; change the branch to truncate `n`
(e.g., via `n.trunc()`) before comparing to `i32::MAX as f64`, and only throw
the RangeError (`"Array buffer allocation failed"` / `js_rangeerror_new` /
`js_throw`) when the truncated value exceeds `i32::MAX`; finally return the
truncated value cast to `i32`.
---
Outside diff comments:
In `@crates/perry-runtime/src/map.rs`:
- Around line 756-759: Replace the panic paths on realloc failure with the
runtime allocation failure handler: in crates/perry-runtime/src/map.rs (lines
756-759) change the branch that currently does panic!("Failed to grow map
entries") so it instead calls crate::error::throw_allocation_failed(); and in
crates/perry-runtime/src/set.rs (lines 541-545) change the branch that currently
does panic!("Failed to grow set elements") so it instead calls
crate::error::throw_allocation_failed(); no other changes required — keep
existing realloc and null-check logic but invoke throw_allocation_failed() in
both places.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28fce235-171c-4470-bbde-da508a1b9588
📒 Files selected for processing (9)
crates/perry-codegen/src/expr/arrays_finds.rscrates/perry-runtime/src/buffer/from.rscrates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/validate.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/regex.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/typedarray/mod.rs
Comment on lines
+545
to
+546
| // #5067 — surface a catchable `RangeError` instead of aborting. | ||
| throw_buffer_alloc_failed(); |
There was a problem hiding this comment.
Handle slab allocation failures too.
This only covers the non-slab branch. buffer_alloc_small() still panic!s at Line 239 when a new slab block allocation returns null, so small Buffer.alloc(...) / Uint8Array(...) calls can still abort the process under memory pressure instead of throwing the catchable RangeError.
Suggested follow-up
fn buffer_alloc_small(capacity: u32) -> *mut BufferHeader {
let needed = std::mem::size_of::<BufferHeader>() + capacity as usize;
// Round up to 8-byte boundary so every header is naturally aligned.
let aligned = (needed + 7) & !7;
SMALL_BUF_SLAB.with(|slab_ref| {
let mut slab = slab_ref.borrow_mut();
if slab.current + aligned > slab.end {
// Current block exhausted (or first call): allocate a fresh slab.
let layout = Layout::from_size_align(SLAB_CAPACITY, 8).unwrap();
let block = unsafe { alloc(layout) };
if block.is_null() {
- panic!(
- "buffer: failed to allocate small-buffer slab ({} bytes)",
- SLAB_CAPACITY
- );
+ throw_buffer_alloc_failed();
}
let block_start = block as usize;
let block_end = block_start + SLAB_CAPACITY;🤖 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 `@crates/perry-runtime/src/buffer/header.rs` around lines 545 - 546,
buffer_alloc_small currently panics when a slab block allocation returns null
(the panic! at the slab-allocation branch); change that branch in function
buffer_alloc_small to handle allocation failure the same way as the non-slab
path by calling throw_buffer_alloc_failed() (or otherwise returning/propagating
the catchable RangeError) instead of panic! so small Buffer.alloc/Uint8Array
calls surface a RangeError and do not abort the process; update any immediate
return value or control flow in buffer_alloc_small to match the non-slab error
path after invoking throw_buffer_alloc_failed().
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/set.rs (1)
567-570:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInitialize SetHeader to safe state before elements allocation.
The SetHeader is allocated in the GC arena at line 561-565 with type
GC_TYPE_SET, but if the elements allocation at line 566 fails, we throw at line 569 before initializing the header fields at lines 573-576. Becausethrow_allocation_failed()allocates objects (string, RangeError) that may trigger GC, the collector could scan the uninitialized SetHeader and read garbage values forsize,capacity, andelements, potentially bypassing the sanity checks ingc_element_slot_range(lines 340-342) and dereferencing an invalid elements pointer.🛡️ Proposed fix to initialize header immediately after arena allocation
let ptr = crate::arena::arena_alloc_gc( std::mem::size_of::<SetHeader>(), 8, crate::gc::GC_TYPE_SET, ) as *mut SetHeader; + // Initialize to safe state BEFORE attempting elements allocation, + // so GC can safely scan if throw_allocation_failed triggers collection. + (*ptr).size = 0; + (*ptr).capacity = 0; + (*ptr).elements = std::ptr::null_mut(); + let elements = alloc(elem_layout) as *mut f64; if elements.is_null() { // `#5067` — catchable RangeError instead of aborting on OOM. crate::error::throw_allocation_failed(); } - // Initialize header - (*ptr).size = 0; - (*ptr).capacity = cap; + // Update capacity now that allocation succeeded. + (*ptr).capacity = cap; // GC_STORE_AUDIT(INIT): set elements buffer is external storage; element stores are barriered separately. (*ptr).elements = elements;🤖 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 `@crates/perry-runtime/src/set.rs` around lines 567 - 570, The SetHeader is allocated in the GC arena but its fields (size, capacity, elements) are not initialized until after the elements allocation at line 566. If that allocation fails and throw_allocation_failed() is called, the GC can be triggered before the header fields are initialized, causing the garbage collector to scan uninitialized memory. Fix this by moving the SetHeader field initialization (currently happening after line 566) to occur immediately after the header is allocated in the arena at line 565, before attempting the elements allocation. This ensures the header is in a valid state with properly initialized size, capacity, and elements fields if throw_allocation_failed() triggers garbage collection.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/set.rs`:
- Around line 567-570: The SetHeader is allocated in the GC arena but its fields
(size, capacity, elements) are not initialized until after the elements
allocation at line 566. If that allocation fails and throw_allocation_failed()
is called, the GC can be triggered before the header fields are initialized,
causing the garbage collector to scan uninitialized memory. Fix this by moving
the SetHeader field initialization (currently happening after line 566) to occur
immediately after the header is allocated in the arena at line 565, before
attempting the elements allocation. This ensures the header is in a valid state
with properly initialized size, capacity, and elements fields if
throw_allocation_failed() triggers garbage collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 187e29ed-a319-47c6-8fbf-4d0f3d529099
📒 Files selected for processing (3)
crates/perry-runtime/src/buffer/validate.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/set.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-runtime/src/map.rs
- crates/perry-runtime/src/buffer/validate.rs
proggeramlug
force-pushed
the
fix/oom-rangeerror-5067
branch
from
56c32ae to
1f66d1f
Compare
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 `@crates/perry-runtime/src/error.rs`:
- Around line 238-248: The new `throw_allocation_failed()` function in
crates/perry-runtime/src/error.rs duplicates the existing
`throw_buffer_alloc_failed()` function in
crates/perry-runtime/src/buffer/header.rs. To consolidate and maintain a single
source of truth, update `throw_buffer_alloc_failed()` in
crates/perry-runtime/src/buffer/header.rs to delegate to the new
`throw_allocation_failed()` from the error module, or alternatively, remove
`throw_buffer_alloc_failed()` entirely and update all of its call sites
throughout the codebase to use `crate::error::throw_allocation_failed()`
directly instead.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04d6ce0c-9c94-4a69-90fe-b26d363b93c5
📒 Files selected for processing (9)
crates/perry-codegen/src/expr/arrays_finds.rscrates/perry-runtime/src/buffer/from.rscrates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/validate.rscrates/perry-runtime/src/error.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/regex.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/typedarray/mod.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/perry-runtime/src/buffer/validate.rs
- crates/perry-runtime/src/regex.rs
- crates/perry-runtime/src/map.rs
- crates/perry-runtime/src/buffer/from.rs
- crates/perry-runtime/src/buffer/header.rs
- crates/perry-codegen/src/expr/arrays_finds.rs
- crates/perry-runtime/src/typedarray/mod.rs
Comment on lines
+238
to
+248
| /// #5067 — throw a catchable `RangeError: Array buffer allocation failed` | ||
| /// (V8/Node's allocation-failure message) instead of aborting the process | ||
| /// when a user-controlled-size backing buffer cannot be allocated. Shared | ||
| /// by the Set/Map/RegExp backing-store allocators. | ||
| #[cold] | ||
| pub(crate) fn throw_allocation_failed() -> ! { | ||
| let msg = b"Array buffer allocation failed"; | ||
| let s = crate::string::js_string_from_bytes(msg.as_ptr(), msg.len() as u32); | ||
| let err = js_rangeerror_new(s); | ||
| crate::exception::js_throw(crate::value::js_nanbox_pointer(err as i64)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consolidate with existing throw_buffer_alloc_failed() to eliminate duplication.
The new throw_allocation_failed() is identical to the existing throw_buffer_alloc_failed() in crates/perry-runtime/src/buffer/header.rs (lines 6-15). Since this PR aims to standardize allocation-failure error handling across allocators, the old buffer-specific function should be replaced or should delegate to this new shared helper to maintain a single source of truth.
♻️ Recommended consolidation approach
In crates/perry-runtime/src/buffer/header.rs, replace the standalone implementation with a delegation:
#[cold]
fn throw_buffer_alloc_failed() -> ! {
- let msg = b"Array buffer allocation failed";
- let s = crate::string::js_string_from_bytes(msg.as_ptr(), msg.len() as u32);
- let err = crate::error::js_rangeerror_new(s);
- crate::exception::js_throw(crate::value::js_nanbox_pointer(err as i64))
+ crate::error::throw_allocation_failed()
}Alternatively, remove throw_buffer_alloc_failed() entirely and update its call sites to use crate::error::throw_allocation_failed() directly.
🤖 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 `@crates/perry-runtime/src/error.rs` around lines 238 - 248, The new
`throw_allocation_failed()` function in crates/perry-runtime/src/error.rs
duplicates the existing `throw_buffer_alloc_failed()` function in
crates/perry-runtime/src/buffer/header.rs. To consolidate and maintain a single
source of truth, update `throw_buffer_alloc_failed()` in
crates/perry-runtime/src/buffer/header.rs to delegate to the new
`throw_allocation_failed()` from the error module, or alternatively, remove
`throw_buffer_alloc_failed()` entirely and update all of its call sites
throughout the codebase to use `crate::error::throw_allocation_failed()`
directly instead.
added 2 commits
June 14, 2026 09:33
…5067) User-controlled-length constructors aborted the process with `panic!` on allocation failure instead of surfacing a catchable JS error. E.g. `new Uint8Array(1e15)` / `Buffer.alloc(1e15)` killed the process rather than throwing `RangeError: Array buffer allocation failed`. Two distinct root causes: 1. Silent f64->integer truncation hid the over-large request before it ever reached an allocator: - codegen lowered `new Uint8Array(1e15)` / `new Float64Array(1e15)` via the `i32` fast path (`*n as i32`), turning 1e15 into a bogus ~-1.5e9 length; - `js_buffer_validate_size` / `uint8array_length_or_throw` / `typed_array_length_or_throw` accepted lengths up to `2**53-1` (Node's `kMaxLength`) and then saturated the cast to `i32`/`u32`, producing a wrong-size object or routing a multi-GB request into the allocator. 2. The leaf allocators `panic!`'d on a null result. Fixes (the tractable, user-controlled-length paths called out in the issue; the foundational `gc_malloc`/arena allocators are left as-is per the issue): - codegen (`arrays_finds.rs`): only take the `i32` fast path when the literal length actually fits in `i32`; otherwise route to the runtime f64 path which validates the full value. - `typed_array_length_or_throw` / `uint8array_length_or_throw` / `js_buffer_validate_size`: throw `RangeError: Array buffer allocation failed` when the requested length exceeds the representable capacity (`u32`/`i32`) instead of truncating. Matches Node, which passes its `<= 2**53-1` length check for these and then fails the real allocation. - TypedArray / Buffer / Set / Map / RegExp leaf allocators: convert the null-result `panic!` into the same catchable `RangeError` (new shared `error::throw_allocation_failed`). `buffer.constants.MAX_LENGTH` is unchanged (still `2**53-1`). Verified: all four headline cases now throw a catchable RangeError, normal allocations and catch/recover work, and the typed-array unit tests (60 passed) + buffer parity test are unaffected.
…et growth OOM (CodeRabbit #5067)
proggeramlug
force-pushed
the
fix/oom-rangeerror-5067
branch
from
1f66d1f to
0ea53d6
Compare
proggeramlug
pushed a commit
that referenced
this pull request
Jun 14, 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
1 participant
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.
Fixes #5067.
Problem
User-controlled-length constructors aborted the process with
panic!on allocation failure instead of throwing a catchable JS error.new Uint8Array(1e15)/new Float64Array(1e15)/Buffer.alloc(1e15)/Buffer.allocUnsafe(1e15)all killed the process.Root causes
f64 → integertruncation hid the over-large request before it reached an allocator:i32fast path (*n as i32), turning1e15into a bogus~-1.5e9length (which then threw the wrong error,Invalid typed array length: -1530494976);js_buffer_validate_size/uint8array_length_or_throw/typed_array_length_or_throwaccepted lengths up to2**53-1(Node'skMaxLength) and then saturated the cast toi32/u32.panic!'d on a null result.Fix (scoped to user-controlled-length paths)
The issue explicitly defers the foundational
gc_malloc/arena allocator refactor; this PR covers the tractable constructor paths it lists.arrays_finds.rs): only take thei32fast path when the literal length fits ini32; otherwise route to the runtime f64 path, which validates the full value.typed_array_length_or_throw/uint8array_length_or_throw/js_buffer_validate_sizenow throwRangeError: Array buffer allocation failedwhen the requested length exceeds the representable capacity (u32/i32) instead of truncating. This matches Node, which passes its<= 2**53-1length check for these and then fails the actual allocation with that same message.panic!becomes the same catchableRangeErrorvia a new sharederror::throw_allocation_failed.Verification
buffer.constants.MAX_LENGTHunchanged (9007199254740991).cargo test -p perry-runtimetyped-array suite: 60 passed, 0 failed.test_parity_buffer.tsruns clean.No version bump / changelog per maintainer instruction — to be folded in at merge.
Summary by CodeRabbit
Bug Fixes
Uint8Arrayand other typed array constructors to only take the compile-time literal fast path when the length fits withini32, preventing silent truncation and ensuring the correct catchableRangeError."Array buffer allocation failed"when computed sizes exceed what Perry can allocate (includingi32/u32limits).Improvements
RegExp, and typed arrays to throw a catchableRangeError: "Array buffer allocation failed"instead of aborting.