fix(gc): re-read imported-ctor args, root the key-add receiver, refuse ConstFn finalize on a keys-identity miss

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.

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.

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<crate::types::LlvmType> = 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::<Result<Vec<_>>>()?;
                 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<crate::types::LlvmType> = 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::<Result<Vec<_>>>()?;
                 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);

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<S
     lowered
 }
 
+/// Re-read constructor argument `i` from `group` **here**.
+///
+/// The re-read re-lowers a `Reload` operand, so it runs under the same
+/// `discard_expr_value` suppression [`lower_constructor_arg`] applied to the
+/// first lowering (#7590); `refresh_rooted_args` does the same for the whole
+/// list.
+pub(super) fn reread_constructor_arg(
+    ctx: &mut FnCtx<'_>,
+    group: &RootedGroup<'_>,
+    i: usize,
+) -> Result<String> {
+    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<String> {
+        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<usize>,
+) -> Result<AccArray> {
     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 `<class>_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
+/// `<class>_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<ImportedCtorArg> {
-    let undef = double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED));
+) -> Result<Vec<ImportedCtorArg>> {
     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<String> = 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 `<class>_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
+        // `<class>_constructor` symbol has exactly `param_count` post-`this`
+        // params; JS ignores extra ctor args.
+        Ok((0..param_count).map(positional).collect())
     }
 }