From 23d0b8d3eedeb687dbb4196687a064b56372e5b1 Mon Sep 17 00:00:00 2001 From: Ralph Date: Mon, 22 Jun 2026 22:32:32 -0700 Subject: [PATCH 1/2] fix(runtime): #5552 demote unique strings in remaining array insert paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #5533 (object fields) and #5548 (the array store paths it enumerated). A uniquely-owned (refcount==1) heap string written into an array element aliases that slot, so a later in-place `s += x` on the source local (`js_string_append`'s refcount==1 fast path) rewrites the stored element and corrupts it. #5548 fixed the push / set / from_values / splice-insert paths; the sibling insert/replace paths below do the same raw element write without the demote. Apply the same tag-checked `js_string_addref_if_heap_string` (no-op for SSO / non-string, idempotent) before the element write at each: - `js_array_unshift_f64` (covers `js_array_unshift_jsvalue` transitively) and the per-item loop in `js_array_unshift_variadic`. - `js_array_fill` / `js_array_fill_range` (demote once before the fill loop — the source aliases every filled slot) and `js_array_fill_generic` (its object-receiver loop writes `value` into each index directly; the array receiver delegates to the two above, so the extra demote is idempotent). - `js_array_with` (the replacement value stored into the new array's slot; the cloned elements are already shared). - `js_array_from_jsvalue` (mixed-type literal construction — the JSValue sibling of the already-covered `js_array_from_values`). Internal reshuffles (sort, splice tail shift, slice copy, copyWithin) only move values already stored in an array — already shared — so no demote is needed. Tests: `string_append_heap_alias.rs` gains compile-run regressions for unshift / fill / with, each confirmed to fail without the demote. `js_array_from_jsvalue` is not emitted by codegen from any TypeScript source, so it gets a runtime unit test (`array/tests.rs`) instead, also confirmed to fail without the demote. Refs #5533, #5548. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../perry-runtime/src/array/concat_reverse.rs | 13 +++ crates/perry-runtime/src/array/immutable.rs | 5 ++ crates/perry-runtime/src/array/jsvalue_api.rs | 7 +- crates/perry-runtime/src/array/push_pop.rs | 10 ++- crates/perry-runtime/src/array/tests.rs | 39 +++++++++ .../perry/tests/string_append_heap_alias.rs | 86 +++++++++++++++++++ 6 files changed, 158 insertions(+), 2 deletions(-) diff --git a/crates/perry-runtime/src/array/concat_reverse.rs b/crates/perry-runtime/src/array/concat_reverse.rs index 5bb539b0ac..768781a9c7 100644 --- a/crates/perry-runtime/src/array/concat_reverse.rs +++ b/crates/perry-runtime/src/array/concat_reverse.rs @@ -347,6 +347,11 @@ fn reverse_throw_type_error(message: &[u8]) -> ! { /// `value`. Returns the same array pointer. #[no_mangle] pub extern "C" fn js_array_fill(arr: *mut ArrayHeader, value: f64) -> *mut ArrayHeader { + // #5552: `fill` writes the same source into every slot — a uniquely-owned + // (refcount==1) string would alias the source AND every filled slot to each + // other, so a later `s += x` corrupts them all. Demote once to shared (no-op + // for SSO / non-string; mirrors `js_array_push_f64`, #5548). + crate::string::js_string_addref_if_heap_string(value); let arr = clean_arr_ptr_mut(arr); if arr.is_null() { return arr; @@ -388,6 +393,9 @@ pub extern "C" fn js_array_fill_range( start: f64, end: f64, ) -> *mut ArrayHeader { + // #5552: demote a uniquely-owned source string once before it fills the + // range (no-op for SSO / non-string). See `js_array_fill`. + crate::string::js_string_addref_if_heap_string(value); let arr = clean_arr_ptr_mut(arr); if arr.is_null() { return arr; @@ -472,6 +480,11 @@ pub extern "C" fn js_array_fill_generic( has_end: i32, end: f64, ) -> f64 { + // #5552: demote a uniquely-owned source string once before any slot write. + // The array receiver delegates to `js_array_fill`/`_range` (which also + // demote — idempotent), but the generic object-receiver loop below writes + // `value` into every index directly, so the demote must happen here too. + crate::string::js_string_addref_if_heap_string(value); let receiver_value = JSValue::from_bits(receiver.to_bits()); if receiver_value.is_null() || receiver_value.is_undefined() { throw_fill_nullish_receiver(); diff --git a/crates/perry-runtime/src/array/immutable.rs b/crates/perry-runtime/src/array/immutable.rs index 9d8f957b33..5f4a9d53d2 100644 --- a/crates/perry-runtime/src/array/immutable.rs +++ b/crates/perry-runtime/src/array/immutable.rs @@ -234,6 +234,11 @@ pub extern "C" fn js_array_with( index: f64, value: f64, ) -> *mut ArrayHeader { + // #5552: the replacement `value` is stored into the new array's slot — demote + // a uniquely-owned (refcount==1) string to shared so a later `s += x` on the + // source local can't corrupt it. No-op for SSO / non-string. The cloned + // elements come from `arr` and are already shared. (Mirrors #5548.) + crate::string::js_string_addref_if_heap_string(value); let arr = clean_arr_ptr(arr); if arr.is_null() { return js_array_alloc(0); diff --git a/crates/perry-runtime/src/array/jsvalue_api.rs b/crates/perry-runtime/src/array/jsvalue_api.rs index 741264afae..5073bfc1e2 100644 --- a/crates/perry-runtime/src/array/jsvalue_api.rs +++ b/crates/perry-runtime/src/array/jsvalue_api.rs @@ -36,8 +36,13 @@ pub extern "C" fn js_array_from_jsvalue(elements: *const u64, count: u32) -> *mu // Each u64 contains NaN-boxed JSValue bits, store as f64 bits for i in 0..count as usize { let bits = *elements.add(i); + let value = f64::from_bits(bits); + // #5552: demote a uniquely-owned source string before it aliases its + // slot — the JSValue sibling of the already-covered + // `js_array_from_values` (#5548). No-op for SSO / non-string. + crate::string::js_string_addref_if_heap_string(value); // GC_STORE_AUDIT(BARRIERED): JSValue array initialization is followed by layout/barrier rebuild. - ptr::write(arr_elements.add(i), f64::from_bits(bits)); + ptr::write(arr_elements.add(i), value); } rebuild_array_layout(arr); } diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index b60eeba465..c244c8f516 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -496,6 +496,11 @@ pub extern "C" fn js_array_shift_f64(arr: *mut ArrayHeader) -> f64 { /// Returns a pointer to the (possibly reallocated) array #[no_mangle] pub extern "C" fn js_array_unshift_f64(arr: *mut ArrayHeader, value: f64) -> *mut ArrayHeader { + // #5552: a uniquely-owned (refcount==1) string unshifted to the front aliases + // the new slot — demote it to shared so a later `s += x` on the source local + // allocates fresh instead of mutating the stored element. No-op for SSO / + // non-string (mirrors `js_array_push_f64`, #5548). + crate::string::js_string_addref_if_heap_string(value); let arr = clean_arr_ptr_mut(arr); if arr.is_null() { return js_array_alloc(0); @@ -590,8 +595,11 @@ pub extern "C" fn js_array_unshift_variadic( // Shift existing elements up by `n`. // GC_STORE_AUDIT(BARRIERED): memmove + new slots followed by layout/barrier rebuild. ptr::copy(elements_ptr, elements_ptr.add(n), length as usize); - // Write items in source order at the front. + // Write items in source order at the front. #5552: demote each + // uniquely-owned string before it aliases its slot (no-op for SSO / + // non-string). for (i, v) in item_vec.into_iter().enumerate() { + crate::string::js_string_addref_if_heap_string(v); ptr::write(elements_ptr.add(i), v); } (*arr).length = length + n as u32; diff --git a/crates/perry-runtime/src/array/tests.rs b/crates/perry-runtime/src/array/tests.rs index c1d4a66037..abf59aa21e 100644 --- a/crates/perry-runtime/src/array/tests.rs +++ b/crates/perry-runtime/src/array/tests.rs @@ -739,6 +739,45 @@ fn test_array_from_jsvalue_int32_rebuild_canonicalizes_raw_slots() { assert_ne!(js_array_get_f64_unchecked(arr, 1).to_bits(), elements[1]); } +/// #5552: `js_array_from_jsvalue` (the JSValue sibling of `js_array_from_values`, +/// used for mixed-type array construction) must demote a uniquely-owned +/// (refcount==1) heap string before it aliases an element slot — otherwise a +/// later in-place `js_string_append` on the source rewrites the stored element. +/// Codegen never emits this symbol today, so this can only be exercised at the +/// runtime level (no compiled-TS regression test can reach it). +#[test] +fn test_array_from_jsvalue_demotes_unique_string_against_inplace_append() { + // A uniquely-owned heap string with spare capacity, so `js_string_append` + // takes its in-place (refcount==1) fast path. + let init = b"prefix_init"; + let s = crate::string::js_string_from_bytes_with_capacity(init.as_ptr(), init.len() as u32, 64); + unsafe { + (*s).refcount = 1; + } + let s_bits = crate::value::js_nanbox_string(s as i64).to_bits(); + + // Construct a mixed-type array via the JSValue path. + let elements = [s_bits, int32_jsvalue_bits(1)]; + let arr = js_array_from_jsvalue(elements.as_ptr(), elements.len() as u32); + + // Grow the source in place. With the demote, `s` is now shared (refcount==0) + // so this allocates fresh and leaves the stored element untouched; without + // it, the in-place append corrupts arr[0]. + let more = crate::string::js_string_from_bytes(b"_more".as_ptr(), 5); + let grown = crate::string::js_string_append(s, more); + + let stored_bits = js_array_get_jsvalue(arr, 0); + let stored_ptr = (stored_bits & crate::value::POINTER_MASK) as *const crate::StringHeader; + let stored = crate::string::string_as_str(stored_ptr); + assert_eq!( + stored, "prefix_init", + "string stored via js_array_from_jsvalue must not be corrupted by a later in-place append" + ); + // Sanity: the source itself did grow (the append happened). + let grown_str = crate::string::string_as_str(grown as *const crate::StringHeader); + assert_eq!(grown_str, "prefix_init_more"); +} + #[test] fn test_nonnumeric_append_downgrades_raw_f64_and_preserves_payload() { let bool_bits = crate::value::JSValue::bool(true).bits(); diff --git a/crates/perry/tests/string_append_heap_alias.rs b/crates/perry/tests/string_append_heap_alias.rs index 01fc3a2e34..7948421a7e 100644 --- a/crates/perry/tests/string_append_heap_alias.rs +++ b/crates/perry/tests/string_append_heap_alias.rs @@ -265,6 +265,92 @@ console.log("a0=" + a[0] + " s=" + s); ); } +// #5552: sibling array insert/replace paths that #5548 left uncovered — +// `unshift`, `fill`, `with`, and mixed-type literal construction +// (`js_array_from_jsvalue`). Each does a raw write of the source value into a +// slot without the demote; the same snapshot-then-grow shape corrupts the +// stored element. Each fails without the matching runtime demote. + +/// `a.unshift(s)` — front insertion (`js_array_unshift_f64`). +#[test] +fn unique_string_unshifted_into_array_is_not_corrupted() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + std::fs::write( + &entry, + r#" +let s = "prefix"; // non-SSO, shared literal +s += "_init"; // append on shared -> fresh heap string, refcount==1 +const a = ["x"]; +a.unshift(s); // front insert -> must demote s to shared +s += "_more"; // refcount==1 in-place append -> must NOT corrupt a[0] +console.log("a0=" + a[0] + " s=" + s); +"#, + ) + .expect("write entry"); + let out = compile_and_run(dir.path(), &entry); + assert!( + out.contains("a0=prefix_init s=prefix_init_more"), + "string unshifted into an array must not be corrupted by a later += (got: {out:?})" + ); +} + +/// `a.fill(s)` — fills every slot with the same source (`js_array_fill`). The +/// source aliases the source local AND every filled slot, so a later `+=` must +/// not corrupt any of them. +#[test] +fn unique_string_filled_into_array_is_not_corrupted() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + std::fs::write( + &entry, + r#" +let s = "prefix"; +s += "_init"; +const a = ["x", "y", "z"]; +a.fill(s); // fill all slots -> must demote s to shared +s += "_more"; +console.log("a0=" + a[0] + " a2=" + a[2] + " s=" + s); +"#, + ) + .expect("write entry"); + let out = compile_and_run(dir.path(), &entry); + assert!( + out.contains("a0=prefix_init a2=prefix_init s=prefix_init_more"), + "string filled into an array must not be corrupted by a later += (got: {out:?})" + ); +} + +/// `a.with(i, s)` — immutable replace stores the source into the new array's +/// slot (`js_array_with`). +#[test] +fn unique_string_stored_via_with_is_not_corrupted() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + std::fs::write( + &entry, + r#" +let s = "prefix"; +s += "_init"; +const a = ["placeholder"].with(0, s); // replace -> must demote s to shared +s += "_more"; +console.log("a0=" + a[0] + " s=" + s); +"#, + ) + .expect("write entry"); + let out = compile_and_run(dir.path(), &entry); + assert!( + out.contains("a0=prefix_init s=prefix_init_more"), + "string stored via arr.with() must not be corrupted by a later += (got: {out:?})" + ); +} + +// NOTE: the mixed-type construction path `js_array_from_jsvalue` (#5552) is not +// emitted by codegen from any TypeScript source, so it has no compiled-TS +// regression test here. Its demote is covered by the runtime unit test +// `test_array_from_jsvalue_demotes_unique_string_against_inplace_append` in +// crates/perry-runtime/src/array/tests.rs. + /// The snapshot-then-grow shape end to end: store the latest value into a heap /// field each step, then keep growing the source. Every stored snapshot must /// retain the value it had when stored (no in-place rewrite). From 960049930814101e60c240cb0830897a4dfc51ea Mon Sep 17 00:00:00 2001 From: Ralph Date: Mon, 22 Jun 2026 23:20:52 -0700 Subject: [PATCH 2/2] fix(runtime): restore GC_STORE_AUDIT marker on unshift_variadic insert write The #5552 demote line pushed the ptr::write past the proximity window of the existing GC_STORE_AUDIT(BARRIERED) marker, failing the lint job's GC store-site inventory check. Re-annotate the insert write directly. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/perry-runtime/src/array/push_pop.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index c244c8f516..7a462e1123 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -600,6 +600,7 @@ pub extern "C" fn js_array_unshift_variadic( // non-string). for (i, v) in item_vec.into_iter().enumerate() { crate::string::js_string_addref_if_heap_string(v); + // GC_STORE_AUDIT(BARRIERED): inserted slots are followed by the layout/barrier rebuild below. ptr::write(elements_ptr.add(i), v); } (*arr).length = length + n as u32;