diff --git a/changelog.d/11783-audit-gc-fixes.md b/changelog.d/11783-audit-gc-fixes.md new file mode 100644 index 0000000000..01c9c04334 --- /dev/null +++ b/changelog.d/11783-audit-gc-fixes.md @@ -0,0 +1,22 @@ +Fix three GC-safety and crash findings from the #11680 audit. + +- Imported constructors (`new ImportedClass(...)`) no longer pass stale + argument pointers across a moving collection. Positional arguments and the + values packed into a `...rest` or synthetic `arguments` array were read + once, before the array allocation, the pushes and the class-value lookup. + Each one is now re-read from its root at its use: every push re-reads its + element, and the dispatch re-reads every positional argument. The + whole-function `root_reload` pass was already repairing the emitted IR for + these shapes; the lowering no longer depends on it, and its test now also + checks the lowering with that pass switched off (a test-only seam). +- `publish_key_add_edge` roots the receiver as well as the closure. The GC + call-effects classifier cannot prove a shape mint non-collecting, so the + ConstFn prewrite, the SPECIAL stamp and the key-add convergence re-read the + receiver after each publication instead of writing through the pre-mint + pointer. The static finalizer's comment that called the mint + non-collecting is corrected. +- The static ConstFn finalizer no longer aborts the process when a receiver + has the same key names as the record already seeded under the requested + static id but a different keys array. It compares the complete record, + keys identity included, before minting: a match is stamped, and anything + else is a refusal that leaves the receiver untouched. diff --git a/crates/perry-codegen/src/expr/collecting_root_tests.rs b/crates/perry-codegen/src/expr/collecting_root_tests.rs index 4865d3df32..8ea8ce10c8 100644 --- a/crates/perry-codegen/src/expr/collecting_root_tests.rs +++ b/crates/perry-codegen/src/expr/collecting_root_tests.rs @@ -777,14 +777,24 @@ fn balanced_add_roots_left_intermediate_across_right_subtree() { }); } -fn imported_constructor_ir(walk_ancestor: bool, has_rest: bool, has_arguments: bool) -> String { +/// `backstop == false` compiles without `root_reload`, so the IR is what the +/// `new` lowering itself emits rather than what the whole-function reload pass +/// repairs afterwards. +fn imported_constructor_ir( + walk_ancestor: bool, + has_rest: bool, + has_arguments: bool, + backstop: bool, +) -> String { let mut opts = crate::temp_root_coverage::entry_opts(); opts.imported_classes.push(crate::ImportedClass { name: "WorkerMade".to_string(), local_alias: None, namespace: None, source_prefix: "worker_producer_ts".to_string(), - constructor_param_count: (usize::from(has_rest) + usize::from(has_arguments)).max(1), + // One positional parameter ahead of any packed slot, so every + // configuration dispatches a positional operand as well as arrays. + constructor_param_count: 1 + usize::from(has_rest) + usize::from(has_arguments), has_own_constructor: true, constructor_has_rest: has_rest, constructor_has_synthetic_arguments: has_arguments, @@ -855,21 +865,24 @@ fn imported_constructor_ir(walk_ancestor: bool, has_rest: bool, has_arguments: b }, )); let symbol = user_function_symbol(&module.name, "probe"); - let ir = - String::from_utf8(compile_module(&module, opts).expect("imported ctor fixture compiles")) - .unwrap(); + let previous = crate::root_reload::TEST_SKIP_ROOT_RELOAD.with(|skip| skip.replace(!backstop)); + let compiled = compile_module(&module, opts); + crate::root_reload::TEST_SKIP_ROOT_RELOAD.with(|skip| skip.set(previous)); + let ir = String::from_utf8(compiled.expect("imported ctor fixture compiles")).unwrap(); function_slice(&ir, &symbol).to_string() } #[test] fn imported_constructor_receiver_refreshes_after_initializers_and_call_preparation() { crate::temp_root_coverage::under_both_lowerings(|mode| { - for walk_ancestor in [false, true] { + for (walk_ancestor, backstop) in + [(false, true), (true, true), (false, false), (true, false)] + { for (has_rest, has_arguments) in [(false, false), (true, false), (false, true), (true, true)] { let packed = has_rest || has_arguments; - let ir = imported_constructor_ir(walk_ancestor, has_rest, has_arguments); + let ir = imported_constructor_ir(walk_ancestor, has_rest, has_arguments, backstop); let blocks = blocks(&ir); let (entry, _) = block(&blocks, "entry.", &ir); let cfg = ColdCfg { @@ -884,11 +897,12 @@ fn imported_constructor_receiver_refreshes_after_initializers_and_call_preparati let call = calls[0]; let operands = temp_slots::call_operands(call.text, ctor).unwrap(); let (this_slot, read) = cfg.slot_read(&operands[0], call); - // Fixed arguments keep their existing root; packed arrays - // must each own a distinct expression root through dispatch. - let fixed_arg = (!packed).then(|| cfg.slot_read(&operands[1], call)); + // The positional argument is re-read from its root at + // dispatch; packed arrays must each own a distinct expression + // root through dispatch. + let fixed_arg = Some(cfg.slot_read(&operands[1], call)); let packed_reads: Vec<_> = if packed { - operands[1..] + operands[2..] .iter() .map(|arg| cfg.root_read(arg, call)) .collect() @@ -992,6 +1006,60 @@ fn imported_constructor_receiver_refreshes_after_initializers_and_call_preparati } } } + // Every value pushed into a packed array, and the positional + // operand at dispatch, is re-read from its root below every + // collecting call that precedes its use: the array allocation, + // each earlier push, and the class-value lookup. + let collecting = |site: &Site<'_>| { + [ + "js_array_alloc", + "js_array_push_f64", + "js_array_push_f64_temp_rooted", + "js_class_value", + ] + .iter() + .any(|helper| site.text.contains(&format!("@{helper}("))) + }; + let pushes: Vec<_> = cfg + .sites() + .into_iter() + .filter_map(|site| { + ["js_array_push_f64", "js_array_push_f64_temp_rooted"] + .into_iter() + .find(|helper| site.text.contains(&format!("@{helper}("))) + .map(|helper| (site, helper)) + }) + .filter(|(site, _)| cfg.dominates(*site, call)) + .collect(); + let expected_pushes = usize::from(has_rest) + 2 * usize::from(has_arguments); + assert_eq!( + pushes.len(), + expected_pushes, + "{mode}: one push per packed element:\n{ir}" + ); + let mut checked: Vec<(Site<'_>, Site<'_>)> = pushes + .iter() + .map(|(push, helper)| { + let pushed = temp_slots::call_operands(push.text, helper).unwrap(); + (*push, cfg.slot_read(&pushed[1], *push).1) + }) + .collect(); + if let Some((_, arg_read)) = fixed_arg { + checked.push((call, arg_read)); + } + for (consumer, value_read) in checked { + for site in cfg.sites().into_iter().filter(|site| { + collecting(site) + && !(site.label == consumer.label && site.index == consumer.index) + && cfg.dominates(*site, consumer) + }) { + cfg.assert_before( + site, + value_read, + "argument reread follows every collecting call before its use", + ); + } + } for (slot, packed_read) in packed_reads { let publications = cfg.publications(slot); // The pool can reuse a released field-initializer root. diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index 45849bd89a..0b1887c31f 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -1496,8 +1496,7 @@ fn lower_new_impl_inner<'a>( // for the final slot (mirrors method_has_rest, #672). // Field initializers / an inlined constructor body were lowered // between the instance allocation and here, so refresh again. - lowered_args = refresh_rooted_args(ctx, group)?; - let marshalled = marshal_imported_ctor_args(ctx, &ctor, &lowered_args, group); + let marshalled = marshal_imported_ctor_args(ctx, &ctor, lowered_args.len(), group)?; let ctor_param_types: Vec = std::iter::once(DOUBLE) .chain(marshalled.iter().map(|_| DOUBLE)) .collect(); @@ -1525,10 +1524,10 @@ fn lower_new_impl_inner<'a>( // Initializers, argument packing and class-value lookup may // collect. The rooted this-slot also owns any replacement this. let ctor_this = ctx.block().load(DOUBLE, &this_slot); - let marshalled: Vec<_> = marshalled + let marshalled = marshalled .iter() .map(|arg| arg.reread(ctx, group)) - .collect(); + .collect::>>()?; let mut ctor_args = vec![(DOUBLE, ctor_this.as_str())]; ctor_args.extend(marshalled.iter().map(|arg| (DOUBLE, arg.as_str()))); let _ = ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args); @@ -1539,8 +1538,7 @@ fn lower_new_impl_inner<'a>( // slot into an array when the ctor's last param is `...rest`. // Field initializers / an inlined constructor body were lowered // between the instance allocation and here, so refresh again. - lowered_args = refresh_rooted_args(ctx, group)?; - let marshalled = marshal_imported_ctor_args(ctx, &ctor, &lowered_args, group); + let marshalled = marshal_imported_ctor_args(ctx, &ctor, lowered_args.len(), group)?; let ctor_param_types: Vec = std::iter::once(DOUBLE) .chain(marshalled.iter().map(|_| DOUBLE)) .collect(); @@ -1565,10 +1563,10 @@ fn lower_new_impl_inner<'a>( // Read after every collecting preparation step, immediately // before dispatch; obj_box still holds the allocation address. let ctor_this = ctx.block().load(DOUBLE, &this_slot); - let marshalled: Vec<_> = marshalled + let marshalled = marshalled .iter() .map(|arg| arg.reread(ctx, group)) - .collect(); + .collect::>>()?; let mut ctor_args = vec![(DOUBLE, ctor_this.as_str())]; ctor_args.extend(marshalled.iter().map(|arg| (DOUBLE, arg.as_str()))); let ctor_ret = ctx.block().call(DOUBLE, &ctor.symbol, &ctor_args); diff --git a/crates/perry-codegen/src/lower_call/new_ctor_args.rs b/crates/perry-codegen/src/lower_call/new_ctor_args.rs index 5c66ba89f8..f2a5c6e319 100644 --- a/crates/perry-codegen/src/lower_call/new_ctor_args.rs +++ b/crates/perry-codegen/src/lower_call/new_ctor_args.rs @@ -304,100 +304,130 @@ pub(super) fn lower_constructor_arg(ctx: &mut FnCtx<'_>, arg: &Expr) -> Result, + group: &RootedGroup<'_>, + i: usize, +) -> Result { + let prev_discard = ctx.discard_expr_value; + ctx.discard_expr_value = false; + let out = group.reread(ctx, i); + ctx.discard_expr_value = prev_discard; + out +} + +/// One operand of an imported-constructor dispatch. +/// +/// None of the arms holds a register. A positional argument is named by its +/// index in the caller's [`RootedGroup`] and an array by its group handle, so +/// the only way to turn either into an operand is [`ImportedCtorArg::reread`], +/// which re-reads it from its root at the point of use. Packing an array, +/// pushing into it and looking up the class value all collect between the +/// argument's lowering and the dispatch, and a register read before any of +/// them names from-space after a moving cycle. pub(super) enum ImportedCtorArg { - Value(String), + /// Index of the lowered `new`-site argument in the caller's group. + Operand(usize), + /// Padding for a missing fixed argument. + Undefined, Array(AccArray), } impl ImportedCtorArg { - pub(super) fn reread(&self, ctx: &mut FnCtx<'_>, group: &RootedGroup<'_>) -> String { - match self { - Self::Value(value) => value.clone(), + /// Re-read this operand from its root **here**. Call it below the last + /// collecting step before the dispatch that consumes the value. + pub(super) fn reread(&self, ctx: &mut FnCtx<'_>, group: &RootedGroup<'_>) -> Result { + Ok(match self { + Self::Operand(i) => reread_constructor_arg(ctx, group, *i)?, + Self::Undefined => double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)), Self::Array(acc) => { let handle = group.read_array(ctx, *acc); nanbox_pointer_inline(ctx.block(), &handle) } - } + }) } } +/// Pack the group operands `args` into a fresh array rooted in `group`. +/// +/// Each element is re-read from its root immediately before its push: the +/// array allocation and every earlier push collect, so a value read before +/// them would store a from-space pointer into the array. fn pack_imported_args_array( ctx: &mut FnCtx<'_>, group: &mut RootedGroup<'_>, - args: &[String], -) -> AccArray { + args: std::ops::Range, +) -> Result { let cap = args.len().to_string(); let acc = group.begin_array(ctx, &cap); - for value in args { - group.push_array(ctx, acc, value); + for i in args { + let value = reread_constructor_arg(ctx, group, i)?; + group.push_array(ctx, acc, &value); } - acc + Ok(acc) } -/// Marshal the lowered `new`-site args into the value list a cross-module -/// imported constructor symbol expects. The source module compiled the -/// standalone `_constructor(this, p0, …)` with `ctor.param_count` -/// explicit slots, laid out as `[fixed..., user_rest?, arguments?]`. A user -/// `...rest` slot (`ctor.has_rest`) must receive a PACKED ARRAY of every -/// trailing arg — not the first trailing arg passed raw — and the synthesized -/// `arguments` slot (`ctor.has_synthetic_arguments`, #10484) a packed array of -/// EVERY arg. Mirrors the inline-ctor `inline_constructor_param_values` -/// packing and the `method_has_rest` path for imported methods (#672). Returns -/// exactly `ctor.param_count` operands; missing fixed args are padded with -/// `undefined`. Packed arrays live in the caller's group from allocation -/// through dispatch, including packing a second array and class-value lookup. +/// Marshal the `new`-site args into the value list a cross-module imported +/// constructor symbol expects. The source module compiled the standalone +/// `_constructor(this, p0, …)` with `ctor.param_count` explicit slots, +/// laid out as `[fixed..., user_rest?, arguments?]`. A user `...rest` slot +/// (`ctor.has_rest`) must receive a PACKED ARRAY of every trailing arg — not +/// the first trailing arg passed raw — and the synthesized `arguments` slot +/// (`ctor.has_synthetic_arguments`, #10484) a packed array of EVERY arg. +/// Mirrors the inline-ctor `inline_constructor_param_values` packing and the +/// `method_has_rest` path for imported methods (#672). Returns exactly +/// `ctor.param_count` operands; missing fixed args are padded with +/// `undefined`. +/// +/// `arg_count` is the number of `new`-site arguments, which occupy group +/// indices `0..arg_count` in order. Every argument, positional or packed, is +/// read from its root at its use; packed arrays live in the caller's group +/// from allocation through dispatch. pub(super) fn marshal_imported_ctor_args( ctx: &mut FnCtx<'_>, ctor: &crate::codegen::ImportedCtor, - lowered_args: &[String], + arg_count: usize, group: &mut RootedGroup<'_>, -) -> Vec { - let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)); +) -> Result> { let param_count = ctor.param_count; + let positional = |i: usize| { + if i < arg_count { + ImportedCtorArg::Operand(i) + } else { + ImportedCtorArg::Undefined + } + }; let trailing = usize::from(ctor.has_rest) + usize::from(ctor.has_synthetic_arguments); if trailing > 0 && param_count >= trailing { let n_positional = param_count - trailing; - let mut out = Vec::with_capacity(param_count); - for i in 0..n_positional { - out.push(ImportedCtorArg::Value( - lowered_args - .get(i) - .cloned() - .unwrap_or_else(|| undef.clone()), - )); - } + let mut out: Vec<_> = (0..n_positional).map(positional).collect(); if ctor.has_rest { - let tail: Vec = lowered_args.iter().skip(n_positional).cloned().collect(); + let tail = n_positional.min(arg_count)..arg_count; out.push(ImportedCtorArg::Array(pack_imported_args_array( - ctx, group, &tail, - ))); + ctx, group, tail, + )?)); } if ctor.has_synthetic_arguments { out.push(ImportedCtorArg::Array(pack_imported_args_array( ctx, group, - lowered_args, - ))); + 0..arg_count, + )?)); } - out + Ok(out) } else { // No rest: positional, padded to `param_count` with `undefined`. - let mut out: Vec<_> = lowered_args - .iter() - .cloned() - .map(ImportedCtorArg::Value) - .collect(); - while out.len() < param_count { - out.push(ImportedCtorArg::Value(undef.clone())); - } - // #6537 review: `param_count.max(out.len())` made this a no-op, so a - // call site passing MORE args than the imported ctor's fixed arity - // emitted excess operands — violating the documented "returns exactly - // `ctor.param_count`" contract (the compiled `_constructor` - // symbol has exactly that many post-`this` params; JS ignores extra - // ctor args). Truncate for real. - out.truncate(param_count); - out + // #6537 review: a call site passing MORE args than the imported + // ctor's fixed arity must not emit excess operands — the compiled + // `_constructor` symbol has exactly `param_count` post-`this` + // params; JS ignores extra ctor args. + Ok((0..param_count).map(positional).collect()) } } diff --git a/crates/perry-codegen/src/root_reload.rs b/crates/perry-codegen/src/root_reload.rs index bb15bae1d7..2c4f4a3c02 100644 --- a/crates/perry-codegen/src/root_reload.rs +++ b/crates/perry-codegen/src/root_reload.rs @@ -446,10 +446,23 @@ struct Facts { succs: Vec, } +#[cfg(test)] +thread_local! { + /// Test seam: skip this pass on the current thread, so a lowering test can + /// assert what the lowering itself emits. Without it, a lowering that + /// carries a stale register is repaired here and its test cannot fail. + pub(crate) static TEST_SKIP_ROOT_RELOAD: std::cell::Cell = + const { std::cell::Cell::new(false) }; +} + /// Apply the reload rule to every function in `module`. Returns the number of /// operands rewritten, which the unit tests assert on so a pass that silently /// stops firing is a failure rather than a no-op. pub(crate) fn apply_to_module(module: &mut crate::module::LlModule) -> usize { + #[cfg(test)] + if TEST_SKIP_ROOT_RELOAD.with(std::cell::Cell::get) { + return 0; + } let mut total = 0; for f in module.functions_mut() { total += apply_to_function(f); diff --git a/crates/perry-runtime/src/object/constfn_key_add_tests.rs b/crates/perry-runtime/src/object/constfn_key_add_tests.rs index 461e4f96e9..6e63f0dcee 100644 --- a/crates/perry-runtime/src/object/constfn_key_add_tests.rs +++ b/crates/perry-runtime/src/object/constfn_key_add_tests.rs @@ -392,3 +392,95 @@ fn cached_constfn_key_add_preserves_preceding_f64_lane() { assert_eq!(capture_body(current, closure::JsThis::UNDEFINED), 43.0); } } + +/// M2 (#11680 audit): `publish_key_add_edge` treats every mint as a +/// collection point for the RECEIVER as well as the closure. A copying minor +/// after the structural publish (and, in the second run, also after the rep +/// publish) moves the receiver; the ConstFn prewrite, the SPECIAL stamp and +/// the convergence must all land on the moved object, and the evacuation +/// verifier must find no from-space reference. +#[test] +fn slow_constfn_key_add_survives_collection_inside_each_publish() { + slow_key_add_with_collections("cfslowmv1", 1); + slow_key_add_with_collections("cfslowmv2", 2); +} + +fn slow_key_add_with_collections(method_key: &str, collections: u32) { + let _guard = gc::CopyingNurseryTestGuard::new(0); + let _triggers = gc::GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let _forced = gc::knob_overrides::ForcedEvacuationTestGuard::on(); + let _verify = gc::knob_overrides::VerifyEvacuationTestGuard::on(); + gc::register_runtime_handle_root_scanner_for_tests(); + gc::gc_register_mutable_root_scanner(crate::string::scan_intern_table_roots_mut); + gc::gc_register_mutable_root_scanner(object::scan_object_cache_roots_mut); + gc::gc_register_mutable_root_scanner(object::scan_shape_cache_roots_mut); + gc::gc_register_mutable_root_scanner(object::scan_transition_cache_roots_mut); + gc::gc_register_mutable_root_scanner(shapes::scan_shape_table_rekey_mut); + let previous = + gc::set_conservative_stack_scan_override(Some(gc::ConservativeStackScanMode::Disabled)); + struct Restore(Option); + impl Drop for Restore { + fn drop(&mut self) { + gc::set_conservative_stack_scan_override(self.0); + field_rep_store::TEST_COLLECT_AFTER_KEY_ADD_PUBLISH.with(|n| n.set(0)); + } + } + let _restore = Restore(previous); + let scope = gc::RuntimeHandleScope::new(); + unsafe { + let key = scope.root_raw_mut_ptr(key(method_key)); + let method = scope.root_raw_mut_ptr(closure::js_closure_alloc(info(), 1)); + method.with_mut_ptr(|m| closure::js_closure_set_capture_f64(m, 0, 41.0)); + let obj = scope.root_raw_mut_ptr(object::js_object_alloc(0, 4)); + let original = obj.with_mut_ptr::(|o| o as usize); + assert!(crate::arena::pointer_in_nursery(original)); + field_rep_store::TEST_COLLECT_AFTER_KEY_ADD_PUBLISH.with(|n| n.set(collections)); + obj.with_mut_ptr(|o| { + key.with_mut_ptr(|k| { + object::js_object_set_field_by_name( + o, + k, + f64::from_bits(method.with_mut_ptr(|m| bits(m))), + ) + }) + }); + assert_eq!( + field_rep_store::TEST_COLLECT_AFTER_KEY_ADD_PUBLISH.with(|n| n.get()), + 0, + "{method_key}: every armed collection must run inside the slow key-add" + ); + assert_ne!( + obj.with_mut_ptr::(|o| o as usize), + original, + "{method_key}: the receiver must move inside the publish" + ); + let stamp = obj.with_mut_ptr(|o| shapes::object_shape_stamp(o)); + let record = shapes::shape_descriptor_by_id(stamp).expect("stamped record"); + assert_eq!( + record.special_constfn_mask, 1, + "{method_key}: the moved receiver must carry the ConstFn shape" + ); + assert_eq!( + obj.with_mut_ptr(|o| field_rep_store::object_slot_rep(o, 0)), + field_rep::REP_SPECIAL + ); + method.with_mut_ptr::(|m| { + assert_eq!( + obj.with_mut_ptr(|o| slot(o)) as usize, + m as usize, + "{method_key}: the moved receiver's slot must hold the current closure" + ) + }); + assert_eq!( + capture_body(obj.with_mut_ptr(|o| slot(o)), closure::JsThis::UNDEFINED), + 41.0 + ); + obj.with_mut_ptr(|o| { + field_rep_store::assert_field_rep_lanes(o, shapes::object_shape_record(o), 1) + }); + // One more evacuation: a stale SPECIAL slot or stamp left in + // from-space would surface here. + let (_, moved) = obj.across_mut::(|| gc::gc_collect_minor()); + assert_eq!(capture_body(slot(moved), closure::JsThis::UNDEFINED), 41.0); + } +} diff --git a/crates/perry-runtime/src/object/field_rep_store.rs b/crates/perry-runtime/src/object/field_rep_store.rs index 982b15283a..4017c4a47c 100644 --- a/crates/perry-runtime/src/object/field_rep_store.rs +++ b/crates/perry-runtime/src/object/field_rep_store.rs @@ -337,6 +337,31 @@ pub(crate) fn cached_key_add_admits(target: u32, slot: u32, value_bits: Option = + const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +fn test_collect_after_publish() { + let armed = TEST_COLLECT_AFTER_KEY_ADD_PUBLISH.with(|c| { + let n = c.get(); + c.set(n.saturating_sub(1)); + n > 0 + }); + if armed { + crate::gc::gc_collect_minor(); + } +} + +#[cfg(not(test))] +#[inline(always)] +fn test_collect_after_publish() {} + /// T2 at a slow-path key-add: publish the keys edge `new_keys`, which appends /// `slot`, with the successor's rep in the last publish. For F64 the final /// shape precedes the caller's value store. For ConstFn, an Any intermediate @@ -366,23 +391,29 @@ pub(crate) unsafe fn publish_key_add_edge( inline: bool, ) -> u32 { let rep = key_add_rep(pred_rep, slot, value_bits, inline); - // A ConstFn birth may be minted only for an executable-image body. Root - // the closure across the structural Any publication, which can collect. - // The final SPECIAL shape is published only after the current closure - // has been written into the now traced Any slot. + // A ConstFn birth may be minted only for an executable-image body. let candidate = if inline && slot < REP_SLOTS && !super::dictionary::is_dictionary(obj) { value_bits.and_then(|bits| unsafe { constfn_store_info(bits) }) } else { None }; - let scope = candidate.map(|_| crate::gc::RuntimeHandleScope::new()); - let value_root = scope - .as_ref() + // Every publication below mints, and the GC call-effects classifier + // cannot prove a mint non-collecting (the keys-array resolvers and the + // attribute lookup reach a collector), so each one is a collection point. + // Root the receiver and re-read it at every use after the first publish; + // root the closure too, so the ConstFn prewrite stores its current + // address. The final SPECIAL shape is published only after that closure + // has been written into the now traced Any slot. + let scope = crate::gc::RuntimeHandleScope::new(); + let receiver = scope.root_raw_mut_ptr(obj); + let value_root = candidate .zip(value_bits) - .map(|(scope, bits)| scope.root_nanbox_f64(f64::from_bits(bits))); + .map(|(_, bits)| scope.root_nanbox_f64(f64::from_bits(bits))); if inline && slot >= super::object_live_slot_count(obj) { - super::set_object_keys(obj, new_keys); - super::shapes::publish_object_live_slot_count_rep(obj, slot + 1, Some(rep)); + receiver.with_mut_ptr(|obj| super::set_object_keys(obj, new_keys)); + receiver.with_mut_ptr(|obj| { + super::shapes::publish_object_live_slot_count_rep(obj, slot + 1, Some(rep)) + }); } else { if slot_rep(rep, slot) == field_rep::REP_F64 { if let Some(bits) = value_bits.and_then(field_rep::f64_slot_bits) { @@ -392,6 +423,7 @@ pub(crate) unsafe fn publish_key_add_edge( let live = super::object_live_slot_count(obj); super::set_object_keys_with_live_rep(obj, new_keys, live, rep); } + test_collect_after_publish(); let fresh_bits = value_root .as_ref() .map(|root| root.get_nanbox_f64().to_bits()) @@ -403,19 +435,24 @@ pub(crate) unsafe fn publish_key_add_edge( // The current shape describes this slot as Any. The regular store // barrier makes the closure visible to a moving collection before // the body-specific shape is minted or stamped. - super::slot_store::store_object_field_slot(obj, slot as usize, bits); + receiver.with_mut_ptr(|obj| { + super::slot_store::store_object_field_slot(obj, slot as usize, bits) + }); } - let id = publish_key_add_rep(obj, pred_rep, slot, fresh_bits, inline, constfn_info); + let id = receiver.with_mut_ptr(|obj| { + publish_key_add_rep(obj, pred_rep, slot, fresh_bits, inline, constfn_info) + }); + test_collect_after_publish(); let value_bits = value_root .as_ref() .map(|root| root.get_nanbox_f64().to_bits()) .or(value_bits); - match value_bits { + receiver.with_mut_ptr(|obj| match value_bits { Some(bits) if inline && id != 0 && !crate::object::dictionary::is_dictionary(obj) => { converge_key_add(obj, id, slot, bits) } _ => id, - } + }) } /// The fix-up after [`publish_key_add_edge`]: restamp `obj` to the successor diff --git a/crates/perry-runtime/src/object/static_shapes.rs b/crates/perry-runtime/src/object/static_shapes.rs index 6e4c80f8a8..c3bca6e3d7 100644 --- a/crates/perry-runtime/src/object/static_shapes.rs +++ b/crates/perry-runtime/src/object/static_shapes.rs @@ -252,12 +252,35 @@ pub(crate) fn finalize_constfn_static( // Validation only reads inline data and Rust-owned metadata; it cannot collect. return object; }; - // This mint does not canonicalize/allocate GC keys or enter JS. Its - // summary/hash/slab path allocates only Rust-owned Box/Vec storage and - // contains no safepoint or deferred-GC lock drop. The raw keys argument - // is therefore consumed without collection; the rooted object owns its - // keys throughout. A collecting interner must root/reload the argument - // inside the mint, not rely on the validation below. + let requested = Some(requested).filter(|&id| id != 0); + // A record already under the requested id (the seed's, or a worker's + // installed seed) is the only shape this receiver may take under that + // id. The mint would either hit it by facts or abort on the miss, and a + // miss is reachable from an ordinary receiver: equal key NAMES in a + // different keys array. Compare the complete record here instead; a match + // is stamped without minting, anything else is a refusal that leaves the + // receiver untouched. + if let Some((id, existing)) = + requested.and_then(|id| shapes::shape_descriptor_by_id(id).map(|d| (id, d))) + { + return root.with_mut_ptr::(|obj| { + if final_record_names_receiver(&existing, ¤t, count, live, rep, &infos) { + // SAFETY: `obj` is the rooted receiver validated above; + // nothing between that validation and here can collect. + unsafe { shapes::stamp_object_shape_id_with_carrier_note(obj, id) }; + note_static_request("finalized-constfn", id, id); + } + obj as usize as u64 + }); + } + // The mint must be treated as a collection point: the GC call-effects + // classifier cannot prove it non-collecting (the key-attribute and + // keys-array resolvers reach a collector). The receiver is therefore + // rooted across it and re-read below. The raw keys argument is the rooted + // receiver's keys array; if a collection inside the mint moved it, the + // minted record names the old address, and the identity check after the + // mint refuses instead of stamping it. No record sits under `requested` + // (checked above), so the adoption cannot be refused. let (minted, obj) = root.across_mut::(|| { shapes::final_shape_ensure_constfn( current.keys as usize as *const ArrayHeader, @@ -266,7 +289,7 @@ pub(crate) fn finalize_constfn_static( class_id, rep, &infos, - Some(requested).filter(|&id| id != 0), + requested, ) }); // Reload and revalidate after the mint; no closure address spans it. @@ -274,32 +297,45 @@ pub(crate) fn finalize_constfn_static( unsafe { finalized_constfn_facts(obj, packed, count, live, class_id, rep, &infos, rebuilt) } { if let Ok(id) = minted { - // The interner compares every fact on its by-facts hit and aborts - // a conflicting requested-id adoption. Still check the complete - // record here, including learned deprecation, before publication. + // Still check the complete record here, including learned + // deprecation, before publication. if shapes::shape_descriptor_by_id(id).is_some_and(|d| { - d.keys == current.keys - && d.logical_key_count == count - && d.live_inline_slot_count == live - && d.proto_id == current.proto_id - && d.object_kind == current.object_kind - && d.semantic_generation == 0 - && d.hole_count == 0 - && d.summary == 0 - && d.rep == rep - && d.deprecation_targets() == (0, 0) - && d.special_constfn_mask - == infos.iter().fold(0, |mask, i| mask | (1 << i.slot)) - && d.constfn_infos() == infos + final_record_names_receiver(&d, ¤t, count, live, rep, &infos) }) { unsafe { shapes::stamp_object_shape_id_with_carrier_note(obj, id) }; - note_static_request("finalized-constfn", requested, id); + note_static_request("finalized-constfn", requested.unwrap_or(0), id); } } } obj as usize as u64 } +/// Whether the shape record `d` describes the validated receiver `current` +/// with the finalized ConstFn facts: the same keys array (identity, not just +/// equal names), bounds, prototype and kind, a birth record with no learned +/// deprecation, and exactly the requested bodies. +fn final_record_names_receiver( + d: &shapes::ShapeDescriptor, + current: &shapes::ShapeDescriptor, + count: u32, + live: u32, + rep: u64, + infos: &[shapes::ConstFnSlotInfo], +) -> bool { + d.keys == current.keys + && d.logical_key_count == count + && d.live_inline_slot_count == live + && d.proto_id == current.proto_id + && d.object_kind == current.object_kind + && d.semantic_generation == 0 + && d.hole_count == 0 + && d.summary == 0 + && d.rep == rep + && d.deprecation_targets() == (0, 0) + && d.special_constfn_mask == infos.iter().fold(0, |mask, i| mask | (1 << i.slot)) + && d.constfn_infos() == infos +} + /// No getter, proxy, or user code runs during this validation. All slots are /// read from the current rooted receiver and must be ordinary inline data. unsafe fn finalized_constfn_facts( diff --git a/crates/perry-runtime/src/object/static_shapes_tests.rs b/crates/perry-runtime/src/object/static_shapes_tests.rs index cad492e529..b502a6ef57 100644 --- a/crates/perry-runtime/src/object/static_shapes_tests.rs +++ b/crates/perry-runtime/src/object/static_shapes_tests.rs @@ -817,3 +817,128 @@ fn declared_class_final_mint_keeps_birth_ordinary_and_uses_each_current_closure( }) }); } + +const ALIAS_KEYS_CHILD_ENV: &str = "PERRY_TEST_CONSTFN_ALIAS_KEYS_CHILD"; + +/// Store a fresh closure of `info` into slot 0 of the rooted receiver and run +/// the finalizer for the one-method shape seeded under `requested`. +fn finalize_one_method( + scope: &crate::gc::RuntimeHandleScope, + raw: *mut crate::object::ObjectHeader, + info: *const crate::closure::JsFunctionInfo, + requested: u32, + packed: &[u8], + entries: &[ConstFnStaticEntry], +) -> (u32, u32) { + let obj = scope.root_raw_mut_ptr(raw); + let closure = scope.root_raw_mut_ptr(crate::closure::js_closure_alloc(info, 0)); + let before = unsafe { + obj.with_mut_ptr::(|obj_ptr| { + crate::object::store_object_field_slot( + obj_ptr, + 0, + closure + .with_mut_ptr::(|closure_ptr| crate::JSValue::object_ptr(closure_ptr)) + .bits(), + ); + shapes::object_shape_stamp(obj_ptr) + }) + }; + let after = obj.with_mut_ptr::(|obj_ptr| { + js_object_finalize_constfn_static( + obj_ptr as usize as u64, + requested, + packed.as_ptr(), + packed.len() as u32, + 1, + 1, + 0, + 3, + entries.as_ptr(), + entries.len() as u32, + ) + }); + (before, unsafe { + shapes::object_shape_stamp(after as usize as *mut _) + }) +} + +/// M1 (#11680 audit): a receiver whose keys array holds the same NAMES as the +/// record already seeded under the requested id, but is a different array, +/// passes every name-level check. The mint then misses by facts and finds the +/// id taken; before the fix that was `static_shape_id_refused_abort`. The +/// finalizer must refuse instead and leave the receiver untouched. Runs in a +/// child so the pre-fix abort fails this test rather than the test binary. +#[test] +fn constfn_finalizer_refuses_equal_names_in_a_different_keys_array() { + const NAME: &str = "object::static_shapes::tests::constfn_finalizer_refuses_equal_names_in_a_different_keys_array"; + if std::env::var_os(ALIAS_KEYS_CHILD_ENV).is_none() { + let out = std::process::Command::new(std::env::current_exe().expect("test binary")) + .arg(NAME) + .arg("--exact") + .arg("--nocapture") + .arg("--test-threads=1") + .env(ALIAS_KEYS_CHILD_ENV, "1") + .output() + .expect("launch the child"); + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + out.status.success(), + "the finalizer must refuse, not abort: {stderr}" + ); + assert!( + stdout.contains("alias-keys refusal checked"), + "the child must actually run the scenario: {stdout}" + ); + return; + } + let _lock = crate::gc::global_side_table_test_lock(); + let info = crate::fn_info!(seeded_constfn_body_a, 0; with_flags(crate::codegen_abi::FN_PERMANENT_IMAGE)); + let entries = [ConstFnStaticEntry { slot: 0, info }]; + let packed = b"ltcf_alias_m\0"; + let requested = SHAPE_ID_BASE + 0x7872; + let seeded = js_shape_seed_plain_constfn( + requested, + packed.as_ptr(), + packed.len() as u32, + 1, + 1, + 3, + entries.as_ptr(), + 1, + ); + assert_eq!(seeded, requested); + let scope = crate::gc::RuntimeHandleScope::new(); + // Control: the canonical-keys receiver finalizes, so the refusal below + // is the keys identity and nothing else. + let control = alloc_constfn_plain_fixture(&[b"ltcf_alias_m"]); + let (_, control_after) = + finalize_one_method(&scope, control, info, requested, packed, &entries); + assert_eq!(control_after, requested, "control must finalize"); + // Same names, a different (non-canonical) keys array. + let keys = unsafe { + let _immortal = crate::gc::ImmortalLayoutScope::new(); + let arr = crate::object::alloc::build_longlived_keys_array( + std::ptr::null_mut(), + 0, + &[b"ltcf_alias_m"], + ); + crate::gc::layout_init_all_pointer_slots(arr as *mut u8); + crate::object::ObjectKeys::new(arr, 1) + }; + let alias = crate::object::alloc_plain::alloc_plain_record_with_keys(1, keys); + let birth = shapes::shape_descriptor_by_id(unsafe { shapes::object_shape_stamp(alias) }) + .expect("alias birth descriptor"); + let record = shapes::shape_descriptor_by_id(requested).expect("seeded record"); + assert_ne!( + birth.keys, record.keys, + "the fixture must use a different keys array" + ); + assert_eq!(birth.object_kind, shapes::ShapeObjectKind::Ordinary); + assert_eq!(birth.rep, crate::object::field_rep::REP_ANY); + let (before, after) = finalize_one_method(&scope, alias, info, requested, packed, &entries); + assert_ne!(before, requested); + assert_eq!(after, before, "refusal must leave the receiver untouched"); + println!("alias-keys refusal checked"); +}