From ef00bec2cbdc6a4f1811fc20adb3478e36af14c3 Mon Sep 17 00:00:00 2001 From: Ralph Kuepper Date: Wed, 5 Aug 2026 23:00:18 +0200 Subject: [PATCH 1/2] fix(runtime): a borrowed builtin is only an Array builtin when it IS Array.prototype[m] (#5902) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classify_own_slot` decides whether an own slot holding a builtin closure means "the array-like engine must run on this receiver". It asked only "is this a non-constructable builtin closure?" — a test EVERY builtin prototype method passes. So var o = { toString(){ return "one" } }; o.concat = String.prototype.concat; o.concat("two", undefined); was read as a borrowed *Array* builtin and ran the array algorithm, producing [o, "two", undefined] instead of "onetwoundefined", even though the equivalent String.prototype.concat.call(o, …) was already correct (test262 built-ins/String/prototype/concat/S15.5.4.6_A4_T1). Adds `is_array_prototype_method_value`, which compares the slot against the real Array.prototype[method] by closure FUNCTION POINTER. Not by closure identity: reading Array.prototype.concat can hand back a freshly reified closure, while every reification of one builtin shares a single code address. A func ptr is a code address, not a heap pointer, so nothing here becomes GC-visible and no root scanner is required. The BOUND_METHOD_FUNC_PTR arm is untouched, and the new check gates only the raw-builtin-thunk arm, so the behavior that classification existed to protect is preserved: a genuine `obj.pop = Array.prototype.pop` borrow still runs the engine on the real receiver. Verified: - test262 built-ins/String/prototype/concat 20/21 -> 21/21 (parity 100%). - test262 Array/prototype {concat,push,pop,splice,sort,shift,unshift, reverse}: zero regressions. Every remaining failure is pre-existing — 10 are named in #5898's baseline, and Array/prototype/concat/ S15.4.4.4_A1_T2 (absent from that snapshot) was proven pre-existing by building without this change and reproducing it identically. - A differential probe matches node byte-for-byte on all 8 lines, covering BOTH directions: the String.prototype.concat borrow, genuine Array.prototype borrows of concat/push/pop/splice, a same-named user method, and plain arrays. New per-PR-visible coverage (perry-runtime --lib, not crates/*/tests/): a unit test pinning both directions of the discriminator plus the wrong-method-name and non-callable cases. --- crates/perry-runtime/src/array/generic.rs | 43 ++++--- crates/perry-runtime/src/array/tests.rs | 107 ++++++++++++++++++ .../perry-runtime/src/object/global_this.rs | 20 ++-- .../src/object/global_this/ctor_thunks.rs | 52 +++++++++ crates/perry-runtime/src/object/mod.rs | 3 +- 5 files changed, 199 insertions(+), 26 deletions(-) diff --git a/crates/perry-runtime/src/array/generic.rs b/crates/perry-runtime/src/array/generic.rs index c72f063564..9ad6adf10c 100644 --- a/crates/perry-runtime/src/array/generic.rs +++ b/crates/perry-runtime/src/array/generic.rs @@ -1263,7 +1263,7 @@ pub fn try_array_proto_chain_method( let raw = (object.to_bits() & 0x0000_FFFF_FFFF_FFFF) as *const crate::object::ObjectHeader; let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32); let own = crate::object::js_object_get_field_by_name_f64(raw, key); - if matches!(classify_own_slot(own), OwnSlot::UserMethod) { + if matches!(classify_own_slot(own, method), OwnSlot::UserMethod) { return None; } if !proto_chain_contains_real_array(raw as usize) { @@ -1462,7 +1462,7 @@ enum OwnSlot { BorrowedBuiltin, } -fn classify_own_slot(v: f64) -> OwnSlot { +fn classify_own_slot(v: f64, method: &str) -> OwnSlot { let jv = JSValue::from_bits(v.to_bits()); if !jv.is_pointer() { return OwnSlot::Absent; @@ -1473,19 +1473,32 @@ fn classify_own_slot(v: f64) -> OwnSlot { } let fp = crate::closure::get_valid_func_ptr(c); if fp.is_null() { - OwnSlot::Absent - } else if fp == crate::closure::BOUND_METHOD_FUNC_PTR - // A raw built-in prototype-method closure (`{ splice: - // Array.prototype.splice }` stores the thunk itself, not a bound - // reification) must also run the generic engine on THIS receiver — - // dispatching it as a user method loses the receiver entirely - // (test262 splice/S15.4.4.12_A6.1_T3). - || crate::object::builtin_closure_is_non_constructable_value(v) + return OwnSlot::Absent; + } + if fp == crate::closure::BOUND_METHOD_FUNC_PTR { + return OwnSlot::BorrowedBuiltin; + } + // A raw built-in prototype-method closure (`{ splice: + // Array.prototype.splice }` stores the thunk itself, not a bound + // reification) must also run the generic engine on THIS receiver — + // dispatching it as a user method loses the receiver entirely + // (test262 splice/S15.4.4.12_A6.1_T3). + // + // #5902: but "a non-constructable builtin closure" is NOT the same claim as + // "a borrowed *Array* builtin". Every builtin prototype method answers that + // test, so `obj.concat = String.prototype.concat` was misclassified and ran + // the ARRAY algorithm — `[obj, "two", undefined]` instead of + // `"onetwoundefined"` (test262 concat/S15.5.4.6_A4_T1), even though the + // equivalent `String.prototype.concat.call(obj, …)` was already correct. + // Require the slot to hold the actual `Array.prototype[method]`; anything + // else is a foreign builtin that must reach its own reflective thunk via + // the normal dispatch, which binds `this` to the receiver. + if crate::object::builtin_closure_is_non_constructable_value(v) + && crate::object::is_array_prototype_method_value(v, method) { - OwnSlot::BorrowedBuiltin - } else { - OwnSlot::UserMethod + return OwnSlot::BorrowedBuiltin; } + OwnSlot::UserMethod } /// True when `object` owns a user method (an own callable field, not a borrowed @@ -1498,7 +1511,7 @@ pub(crate) fn object_owns_user_method(object: f64, method: &str) -> bool { let raw = (object.to_bits() & 0x0000_FFFF_FFFF_FFFF) as *const crate::object::ObjectHeader; let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32); let own = crate::object::js_object_get_field_by_name_f64(raw, key); - matches!(classify_own_slot(own), OwnSlot::UserMethod) + matches!(classify_own_slot(own, method), OwnSlot::UserMethod) } /// Dispatch a generic `Array.prototype` mutator over an array-like receiver. @@ -1540,7 +1553,7 @@ pub fn try_object_arraylike_mutator( // method (`{ push(x) {…} }`) is left to the normal dispatch. let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32); let own = crate::object::js_object_get_field_by_name_f64(raw, key); - if matches!(classify_own_slot(own), OwnSlot::UserMethod) { + if matches!(classify_own_slot(own, method), OwnSlot::UserMethod) { return None; } run_object_mutator(object, method, args_ptr, args_len) diff --git a/crates/perry-runtime/src/array/tests.rs b/crates/perry-runtime/src/array/tests.rs index 17f315d57c..8cf29cf12e 100644 --- a/crates/perry-runtime/src/array/tests.rs +++ b/crates/perry-runtime/src/array/tests.rs @@ -1642,3 +1642,110 @@ fn push_built_array_gets_and_keeps_dense_raw_f64_flag() { ); } } + +/// #5902: the array-like engine may only claim an own slot when the borrowed +/// builtin it holds is genuinely `Array.prototype[method]`. +/// +/// `classify_own_slot` used to ask only "is this a non-constructable builtin +/// closure?", which EVERY builtin prototype method answers yes to — so +/// `obj.concat = String.prototype.concat` was read as a borrowed *Array* +/// builtin and ran the array algorithm, returning `[obj, "two", undefined]` +/// where the spec gives `"onetwoundefined"` (test262 +/// `built-ins/String/prototype/concat/S15.5.4.6_A4_T1`). The discriminator is +/// the closure's function pointer; this test pins both directions of it, so a +/// regression that re-broadens (or over-narrows) the predicate fails here +/// rather than only in the parity sweep. +#[test] +fn array_prototype_method_discriminator_separates_foreign_builtins() { + // Reads realm intrinsics and holds raw pointers across allocating calls, + // and libtest gives each test its own thread where `GLOBAL_THIS_PTR` can be + // re-created — so resolve the whole snapshot inside one iteration and retry + // until a GC-quiet pass yields a self-consistent view. Mirrors + // `array_literal_shares_the_realm_array_prototype`'s loop above. + let mut checked = false; + for _ in 0..256 { + let global = crate::object::js_get_global_this(); + let global_ptr = + crate::value::js_nanbox_get_pointer(global) as *const crate::object::ObjectHeader; + if global_ptr.is_null() { + std::thread::yield_now(); + continue; + } + + let proto_of = |ctor_name: &[u8]| -> Option { + let ctor = + crate::object::js_object_get_field_by_name(global_ptr, string_key(ctor_name)); + if !ctor.is_pointer() { + return None; + } + let ctor_ptr = + crate::value::js_nanbox_get_pointer(f64::from_bits(ctor.bits())) as usize; + if ctor_ptr == 0 { + return None; + } + Some(crate::closure::closure_get_dynamic_prop( + ctor_ptr, + "prototype", + )) + }; + + let (Some(array_proto), Some(string_proto)) = (proto_of(b"Array"), proto_of(b"String")) + else { + std::thread::yield_now(); + continue; + }; + let array_proto_ptr = + crate::value::js_nanbox_get_pointer(array_proto) as *const crate::object::ObjectHeader; + let string_proto_ptr = + crate::value::js_nanbox_get_pointer(string_proto) as *const crate::object::ObjectHeader; + if array_proto_ptr.is_null() || string_proto_ptr.is_null() { + std::thread::yield_now(); + continue; + } + + let array_concat = + crate::object::js_object_get_field_by_name_f64(array_proto_ptr, string_key(b"concat")); + let string_concat = + crate::object::js_object_get_field_by_name_f64(string_proto_ptr, string_key(b"concat")); + if !crate::value::JSValue::from_bits(array_concat.to_bits()).is_pointer() + || !crate::value::JSValue::from_bits(string_concat.to_bits()).is_pointer() + { + std::thread::yield_now(); + continue; + } + + // The Array borrow must still be claimed — this is the behavior the + // original classification existed to protect (`obj.concat = + // Array.prototype.concat` has to run the array engine on `obj`). + assert!( + crate::object::is_array_prototype_method_value(array_concat, "concat"), + "Array.prototype.concat must be recognized as an Array builtin" + ); + // The foreign borrow must NOT be claimed. + assert!( + !crate::object::is_array_prototype_method_value(string_concat, "concat"), + "String.prototype.concat must not be mistaken for an Array builtin" + ); + // Right closure, wrong method name is also a mismatch — the predicate + // keys on the (method, closure) pair, not on "is some Array builtin". + assert!( + !crate::object::is_array_prototype_method_value(array_concat, "push"), + "Array.prototype.concat must not answer for `push`" + ); + // Non-callable / non-pointer slots are never a borrowed builtin. + assert!(!crate::object::is_array_prototype_method_value( + 1.0, "concat" + )); + assert!(!crate::object::is_array_prototype_method_value( + f64::from_bits(crate::value::TAG_UNDEFINED), + "concat" + )); + + checked = true; + break; + } + assert!( + checked, + "never obtained a GC-quiet view of Array.prototype / String.prototype" + ); +} diff --git a/crates/perry-runtime/src/object/global_this.rs b/crates/perry-runtime/src/object/global_this.rs index 8d59bf39f6..5db69de8ca 100644 --- a/crates/perry-runtime/src/object/global_this.rs +++ b/crates/perry-runtime/src/object/global_this.rs @@ -97,16 +97,16 @@ pub(crate) use ctor_thunks::{ builtin_prototype_value, cryptokey_algorithm_getter_thunk, cryptokey_extractable_getter_thunk, cryptokey_type_getter_thunk, cryptokey_usages_getter_thunk, error_constructor_call_thunk, eval_error_constructor_call_thunk, global_this_crypto_getter_thunk, - global_this_url_pattern_call_thunk, is_function_prototype_object_value, - map_constructor_call_thunk, normalize_eval_this_body, promise_constructor_call_thunk, - range_error_constructor_call_thunk, reference_error_constructor_call_thunk, - regexp_constructor_call_thunk, set_constructor_call_thunk, subtle_crypto_method_value, - syntax_error_constructor_call_thunk, type_error_constructor_call_thunk, - typed_array_constructor_call_thunk, uri_error_constructor_call_thunk, - weak_map_constructor_call_thunk, weak_ref_constructor_call_thunk, - weak_set_constructor_call_thunk, webcrypto_get_random_values_thunk, - webcrypto_illegal_constructor_thunk, webcrypto_method_value, webcrypto_random_uuid_thunk, - webcrypto_subtle_getter_thunk, + global_this_url_pattern_call_thunk, is_array_prototype_method_value, + is_function_prototype_object_value, map_constructor_call_thunk, normalize_eval_this_body, + promise_constructor_call_thunk, range_error_constructor_call_thunk, + reference_error_constructor_call_thunk, regexp_constructor_call_thunk, + set_constructor_call_thunk, subtle_crypto_method_value, syntax_error_constructor_call_thunk, + type_error_constructor_call_thunk, typed_array_constructor_call_thunk, + uri_error_constructor_call_thunk, weak_map_constructor_call_thunk, + weak_ref_constructor_call_thunk, weak_set_constructor_call_thunk, + webcrypto_get_random_values_thunk, webcrypto_illegal_constructor_thunk, webcrypto_method_value, + webcrypto_random_uuid_thunk, webcrypto_subtle_getter_thunk, }; #[cfg(feature = "temporal")] pub(crate) use fetch_globals::temporal_subclass_super; diff --git a/crates/perry-runtime/src/object/global_this/ctor_thunks.rs b/crates/perry-runtime/src/object/global_this/ctor_thunks.rs index dda8d7d90a..6713de0db7 100644 --- a/crates/perry-runtime/src/object/global_this/ctor_thunks.rs +++ b/crates/perry-runtime/src/object/global_this/ctor_thunks.rs @@ -221,6 +221,58 @@ pub(crate) fn builtin_prototype_value(name: &str) -> f64 { crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype") } +/// #5902: is `value` literally `Array.prototype[method]`? +/// +/// The array-like engine treats *any* borrowed builtin closure stored in an +/// own slot as a borrowed **Array** builtin (`classify_own_slot`), which is +/// only true when the borrow actually came off `Array.prototype`. +/// `obj.concat = String.prototype.concat` stores a non-constructable builtin +/// closure too, so it was misread and ran the array algorithm — returning +/// `[obj, "two", undefined]` where the spec (and the working +/// `String.prototype.concat.call(obj, …)` form) gives `"onetwoundefined"`. +/// +/// Compared by closure FUNCTION POINTER, not by closure identity: reading +/// `Array.prototype.concat` can hand back a freshly reified closure, but every +/// reification of the same builtin shares one code address. A func ptr is a +/// code address rather than a heap pointer, so nothing here is GC-visible and +/// no root scanner is required. +pub(crate) fn is_array_prototype_method_value(value: f64, method: &str) -> bool { + let slot = crate::value::JSValue::from_bits(value.to_bits()); + if !slot.is_pointer() { + return false; + } + let slot_ptr = slot.as_pointer::(); + if slot_ptr.is_null() { + return false; + } + let slot_fp = crate::closure::get_valid_func_ptr(slot_ptr); + if slot_fp.is_null() { + return false; + } + + let proto = builtin_prototype_value("Array"); + let proto_bits = proto.to_bits(); + if (proto_bits >> 48) != 0x7FFD { + return false; + } + let proto_ptr = (proto_bits & crate::value::POINTER_MASK) as *const super::super::ObjectHeader; + if proto_ptr.is_null() { + return false; + } + let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32); + let canonical = super::super::js_object_get_field_by_name_f64(proto_ptr, key); + let canonical_jv = crate::value::JSValue::from_bits(canonical.to_bits()); + if !canonical_jv.is_pointer() { + return false; + } + let canonical_ptr = canonical_jv.as_pointer::(); + if canonical_ptr.is_null() { + return false; + } + let canonical_fp = crate::closure::get_valid_func_ptr(canonical_ptr); + !canonical_fp.is_null() && canonical_fp == slot_fp +} + pub(crate) extern "C" fn webcrypto_illegal_constructor_thunk( _closure: *const crate::closure::ClosureHeader, ) -> f64 { diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index ff4f4a3e3b..c2e620e2a6 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -55,7 +55,8 @@ mod global_this; pub mod handle_expando; pub(crate) mod prop_plan; pub(crate) use global_this::{ - default_prepare_stack_trace_func_ptr, scan_error_constructor_root_mut, ERROR_CONSTRUCTOR_PTR, + default_prepare_stack_trace_func_ptr, is_array_prototype_method_value, + scan_error_constructor_root_mut, ERROR_CONSTRUCTOR_PTR, }; mod global_this_tables; mod groupby; From 0f9bb6cc58e9cf7188b48c36f6166c4e13f3bfc4 Mon Sep 17 00:00:00 2001 From: Ralph Kuepper Date: Wed, 5 Aug 2026 23:01:11 +0200 Subject: [PATCH 2/2] docs: changelog fragment for #7471 --- changelog.d/7471-borrowed-array-builtin-identity.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7471-borrowed-array-builtin-identity.md diff --git a/changelog.d/7471-borrowed-array-builtin-identity.md b/changelog.d/7471-borrowed-array-builtin-identity.md new file mode 100644 index 0000000000..24acf3edc9 --- /dev/null +++ b/changelog.d/7471-borrowed-array-builtin-identity.md @@ -0,0 +1 @@ +fix(runtime): a borrowed builtin only runs the array engine when it IS `Array.prototype[method]` (#5902). `classify_own_slot` (`array/generic.rs`) decides whether an own slot holding a builtin closure means "the array-like engine must run on this receiver", but it asked only "is this a non-constructable builtin closure?" — a test every builtin prototype method passes, with the owning prototype never checked. So `obj.concat = String.prototype.concat` was read as a borrowed *Array* builtin and ran the array algorithm, yielding `[obj, "two", undefined]` instead of `"onetwoundefined"` (test262 `built-ins/String/prototype/concat/S15.5.4.6_A4_T1`) — while the equivalent `String.prototype.concat.call(obj, …)` was already correct, which is the tell that this was dispatch and not coercion. The new `is_array_prototype_method_value` compares the slot against the real `Array.prototype[method]` by closure FUNCTION POINTER rather than by closure identity: reading `Array.prototype.concat` can hand back a freshly reified closure, so identity would give false negatives, while every reification of one builtin shares a single code address — and a func ptr is a code address, not a heap pointer, so nothing here becomes GC-visible and no root scanner is required. The `BOUND_METHOD_FUNC_PTR` arm is untouched and the new check gates only the raw-builtin-thunk arm, so the behavior the classification exists to protect is preserved: a genuine `obj.pop = Array.prototype.pop` borrow still runs the engine on the real receiver instead of looping on its captured `Array.prototype`. test262 `built-ins/String/prototype/concat` 20/21 → 21/21 (parity 100%); `Array/prototype/{concat,push,pop,splice,sort,shift,unshift,reverse}` zero regressions — 10 remaining failures are named in #5898's baseline (including `unshift/S15.4.4.13_A4_T1`, which itself borrows `Array.prototype.unshift` and is unchanged), and `Array/prototype/concat/S15.4.4.4_A1_T2`, absent from that snapshot, was proven pre-existing by rebuilding without the change and reproducing it identically. A differential probe matches node byte-for-byte on all 8 lines, covering both directions (the String borrow, genuine Array borrows of concat/push/pop/splice, a same-named user method, plain arrays). New per-PR-visible coverage: a `perry-runtime --lib` unit test pinning both directions of the discriminator plus the wrong-method-name and non-callable cases, which cannot pass vacuously — an always-true or always-false predicate fails one of its asserts.