fix(runtime): Next.js standalone wall 45 — dynamic-parent capture corruption#5152
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds Dynamic-parent subclass allocation and vtable arity fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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: 1
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/object/class_registry.rs (1)
4298-4435:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the 64-argument ceiling in release builds too.
debug_assert!disappears in optimized builds, so a ctor/method with 65+ params still enters this arm and is invoked with only 64 actual args. That reopens the same truncation/garbage-read corruption this patch is fixing, just above a higher threshold. Make this a hard invariant at runtime, or reject oversized registrations upstream.Possible fix
- debug_assert!( + assert!( param_count as usize <= 64, "call_vtable_method: param_count {} exceeds fixed dispatch arity 64", param_count );🤖 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/object/class_registry.rs` around lines 4298 - 4435, Replace the debug_assert! that checks param_count in the call_vtable_method function with a runtime assertion that will execute even in release builds. Change debug_assert! to assert! to enforce the 64-argument ceiling as a hard invariant, ensuring that methods with 65+ parameters cannot bypass this check in optimized builds and cause memory corruption from reading garbage data or out-of-bounds memory.
🤖 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/object/alloc.rs`:
- Around line 427-507: The merged path in this function derives field_count from
the merged_arr.length (via merged_len), but it should instead compute
field_count by adding the registered parent field count (parent_fc) and
own_field_count parameters. Currently, the registered field counts (parent_fc
extracted from registered_class_keys_array and own_field_count passed as a
parameter) are retrieved but never used in the merged path, causing the
allocated object to be sized based on visible key count rather than logical slot
count. Replace the line that sets field_count based on merged_len with a
calculation that adds parent_fc and own_field_count to ensure the object
allocation matches the slot indexing that codegen will perform.
---
Outside diff comments:
In `@crates/perry-runtime/src/object/class_registry.rs`:
- Around line 4298-4435: Replace the debug_assert! that checks param_count in
the call_vtable_method function with a runtime assertion that will execute even
in release builds. Change debug_assert! to assert! to enforce the 64-argument
ceiling as a hard invariant, ensuring that methods with 65+ parameters cannot
bypass this check in optimized builds and cause memory corruption from reading
garbage data or out-of-bounds memory.
🪄 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: 698f97a2-923b-4549-a67f-b37b23ece83e
📒 Files selected for processing (7)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-runtime/src/object/alloc.rscrates/perry-runtime/src/object/class_registry.rs
Comment on lines
+427
to
+507
| let parent_cid = crate::object::get_parent_class_id(class_id).unwrap_or(0); | ||
| let parent_keys = if parent_cid != 0 { | ||
| registered_class_keys_array(parent_cid) | ||
| } else { | ||
| None | ||
| }; | ||
| let Some((parent_arr, _parent_fc)) = parent_keys else { | ||
| // No dynamic parent layout available — own-only fallback keeps the | ||
| // prior baseline (correct for parentless / builtin-parent classes). | ||
| return js_object_alloc_class_with_keys( | ||
| class_id, | ||
| parent_cid, | ||
| own_field_count, | ||
| own_packed_keys, | ||
| own_packed_keys_len, | ||
| ); | ||
| }; | ||
| let parent_len = unsafe { (*parent_arr).length }; | ||
|
|
||
| // Cache the merged keys-array per class. The shape id is namespaced away | ||
| // from the own-only shape (`+ 2_000_000`) so it can't collide with the | ||
| // `js_build_class_keys_array` / `js_object_alloc_class_with_keys` shapes. | ||
| let shape_id = class_id.wrapping_mul(10007).wrapping_add(2_000_000); | ||
| let cached = shape_cache_get(shape_id); | ||
| let (merged_arr, field_count) = if !cached.is_null() { | ||
| (cached, unsafe { (*cached).length }) | ||
| } else { | ||
| let own_keys: Vec<&[u8]> = if own_packed_keys.is_null() || own_packed_keys_len == 0 { | ||
| Vec::new() | ||
| } else { | ||
| let bytes = unsafe { | ||
| std::slice::from_raw_parts(own_packed_keys, own_packed_keys_len as usize) | ||
| }; | ||
| bytes.split(|&b| b == 0).filter(|s| !s.is_empty()).collect() | ||
| }; | ||
| let merged_len = parent_len as usize + own_keys.len(); | ||
| let arr = crate::array::js_array_alloc_with_length_longlived(merged_len as u32); | ||
| let dst = unsafe { (arr as *mut u8).add(8) as *mut f64 }; | ||
| let src = unsafe { (parent_arr as *mut u8).add(8) as *const f64 }; | ||
| unsafe { | ||
| for i in 0..parent_len as usize { | ||
| let bits = (*src.add(i)).to_bits(); | ||
| *dst.add(i) = f64::from_bits(bits); | ||
| crate::array::note_array_slot_layout_only(arr, i, bits); | ||
| } | ||
| for (j, key_bytes) in own_keys.iter().enumerate() { | ||
| let str_ptr = crate::string::js_string_from_bytes_longlived( | ||
| key_bytes.as_ptr(), | ||
| key_bytes.len() as u32, | ||
| ); | ||
| let nanboxed = f64::from_bits( | ||
| crate::value::STRING_TAG | (str_ptr as u64 & crate::value::POINTER_MASK), | ||
| ); | ||
| let idx = parent_len as usize + j; | ||
| *dst.add(idx) = nanboxed; | ||
| crate::array::note_array_slot_layout_only(arr, idx, nanboxed.to_bits()); | ||
| } | ||
| } | ||
| shape_cache_insert(shape_id, arr); | ||
| (arr, merged_len as u32) | ||
| }; | ||
|
|
||
| let header_size = std::mem::size_of::<ObjectHeader>(); | ||
| let alloc_field_count = std::cmp::max(field_count as usize, 8); | ||
| let fields_size = alloc_field_count * std::mem::size_of::<JSValue>(); | ||
| let total_size = header_size + fields_size; | ||
| let ptr = arena_alloc_gc(total_size, 8, crate::gc::GC_TYPE_OBJECT) as *mut ObjectHeader; | ||
| unsafe { | ||
| (*ptr).object_type = crate::error::OBJECT_TYPE_REGULAR; | ||
| (*ptr).class_id = class_id; | ||
| (*ptr).parent_class_id = parent_cid; | ||
| (*ptr).field_count = field_count; | ||
| let fields_ptr = (ptr as *mut u8).add(header_size) as *mut JSValue; | ||
| for i in 0..alloc_field_count { | ||
| // GC_STORE_AUDIT(INIT): freshly allocated object field slot is initialized pointer-free. | ||
| ptr::write(fields_ptr.add(i), JSValue::undefined()); | ||
| } | ||
| set_object_keys_array(ptr, merged_arr); | ||
| crate::gc::layout_init_pointer_free(ptr as *mut u8); | ||
| } | ||
| remember_class_keys_array(class_id, field_count, merged_arr); |
There was a problem hiding this comment.
Use the registered field counts to size the merged instance.
The merged path throws away both parent_fc and own_field_count and instead derives field_count from keys_array.length. That only works while visible key count exactly matches logical slot count. If those ever diverge, this allocator under-sizes the object/header while codegen still indexes slots by parent_fc + own_field_count, recreating the layout mismatch this function is meant to fix.
Possible fix
- let Some((parent_arr, _parent_fc)) = parent_keys else {
+ let Some((parent_arr, parent_fc)) = parent_keys else {
// No dynamic parent layout available — own-only fallback keeps the
// prior baseline (correct for parentless / builtin-parent classes).
return js_object_alloc_class_with_keys(
class_id,
parent_cid,
@@
- let (merged_arr, field_count) = if !cached.is_null() {
- (cached, unsafe { (*cached).length })
+ let field_count = parent_fc.saturating_add(own_field_count);
+ let merged_arr = if !cached.is_null() {
+ cached
} else {
@@
- let merged_len = parent_len as usize + own_keys.len();
+ let merged_len = parent_len as usize + own_keys.len();
+ debug_assert_eq!(merged_len as u32, field_count);
let arr = crate::array::js_array_alloc_with_length_longlived(merged_len as u32);
@@
- (arr, merged_len as u32)
+ arr
};
+ debug_assert_eq!(unsafe { (*merged_arr).length }, field_count);🤖 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/object/alloc.rs` around lines 427 - 507, The merged
path in this function derives field_count from the merged_arr.length (via
merged_len), but it should instead compute field_count by adding the registered
parent field count (parent_fc) and own_field_count parameters. Currently, the
registered field counts (parent_fc extracted from registered_class_keys_array
and own_field_count passed as a parameter) are retrieved but never used in the
merged path, causing the allocated object to be sized based on visible key count
rather than logical slot count. Replace the line that sets field_count based on
merged_len with a calculation that adds parent_fc and own_field_count to ensure
the object allocation matches the slot indexing that codegen will perform.
added 2 commits
June 14, 2026 21:34
…ruption (v0.5.1169) class Derived extends _base.default read inherited __perry_cap_* captures as garbage from inherited methods. Two composing roots: 1. call_vtable_method truncated any >9-param call to 10 args (the _ => catch-all invoked higher-arity fns as if they had 10 params), so a base ctor with opts + 40 capture params lost captures 10+ when run via the runtime dispatch path (run_class_constructor_on_this_flat). Widened to a 64-arity transmute with undefined padding (overshooting args is caller-cleaned/safe), with a debug_assert backstop. Mirrors the closure path's 16-arity fan-out. 2. Dynamic-parent instances were sized for own fields only and laid inherited fields out at wrong slot indices. New js_object_alloc_class_dynamic_parent resolves the runtime-registered parent edge + keys-array and allocates the merged [parent keys] ++ [own keys] layout; new.rs routes extends_expr classes to it. Merged keys shape-cached per class. Repro: 40-capture base extended via _mod.default now reads all captures as object from inherited methods. Wall 38/42 green. perry-runtime 1035/0 single-threaded.
proggeramlug
force-pushed
the
feat/nextjs-wall-45
branch
from
fa2dbf1 to
9828605
Compare
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.
What
class Derived extends _base.default(the interop-ESM default-export base shape — wall 38) read the parent's captured module-levelrequires (this.__perry_cap_*) as garbage numbers/functions when accessed from inherited methods. In Next.js this surfaced as(0,_iserror.getProperError)on anundefinedcapture inside base-server'shandleRequestImpl.Two composing root causes
1. Argument truncation in
call_vtable_method(the masking bug)The runtime vtable dispatcher had explicit arity arms only for 0–9 params; the
_ =>catch-all invoked every higher-arity function as if it had exactly 10 params, silently dropping args 10+. A base class capturing 40 module-level requires compiles to a 41-param constructor (opts+ 40__perry_cap_*); run on a derived instance viarun_class_constructor_on_this_flat→call_vtable_method, only the first ~9 captures were passed — the rest wrote uninitialized stack/register garbage.Widened the catch-all to a fixed 64-arity transmute with
undefinedpadding. Passing more args than the callee declares is safe on every target (caller-allocated, caller-cleaned arg area; the callee reads only its declared params). Adebug_assertflags the rare class exceeding 64 so truncation surfaces in tests, not as silent corruption. This is the vtable-dispatch counterpart to the closure path's existing 16-arity fan-out.2. Under-allocation + mis-layout of dynamic-parent instances
Because
Derived's parent resolves dynamically (extends_nameis the unresolved"default", soclass_field_global_index's parent walk bails), codegen sized the instance forDerived's OWN fields only (max(1,8)slots) and laid inherited fields at the wrong indices. New runtime allocatorjs_object_alloc_class_dynamic_parentresolves the runtime-registered parent edge (js_register_class_parent_dynamic) + the parent's keys-array (js_build_class_keys_array) — both established at module init, before anynew X()— and allocates the mergedfield_count = parent + own,keys = [parent keys] ++ [own keys](parent first, matching the slot order the parent's compiled code expects).new.rsroutesextends_exprclasses to it; merged keys are shape-cached per class.Verification
_mod.default): all 40 captures read asobjectfrom inherited methods (was garbage for captures 10+). Same-module directnew Base()unaffected throughout.super.method()dispatches).perry-runtimetests: 1035 passed / 0 failed single-threaded (the 3 parallel failures are a pre-existingPoisonErrorpollution cascade in untouched modules — all pass isolated).Part of the Next.js standalone bring-up (#793 umbrella; continues #5125).
Summary by CodeRabbit
Release Notes
Bug Fixes
undefinedinstead of incorrect values.Performance