Skip to content

Heap-string aliasing: demote unique strings in remaining array insert paths (unshift / fill / with / from_jsvalue) #5552

Description

@proggeramlug

Follow-up to #5533 and #5548.

#5533 demoted uniquely-owned (refcount==1) heap strings to shared when stored into object fields; #5548 extended the same js_string_addref_if_heap_string demote to the array store paths it enumerated: js_array_push_f64, js_array_set_f64 / js_array_set_f64_extend, js_array_from_values, the inline codegen element-store choke point (emit_jsvalue_slot_store_on_block), the scalar-replaced array-literal init, and the splice insert loop.

The bug: a uniquely-owned heap string written into an array element aliases that slot, so a later in-place s += x on the source local mutates the stored element and corrupts it. The demote (js_string_addref_if_heap_string, a tag-checked no-op for SSO / non-string values, idempotent — it just marks refcount=0) fixes it at each store site.

Still uncovered

While auditing #5548 I found several sibling insert/replace paths that do the same raw ptr::write of a source f64 element without the demote — structurally identical to the paths whose regression tests prove the bug:

  • js_array_unshift_f64 (crates/perry-runtime/src/array/push_pop.rs:491), plus js_array_unshift_jsvalue (:533) and js_array_unshift_variadic (:543) — arr.unshift(s) writes the source value directly.
  • js_array_fill (crates/perry-runtime/src/array/concat_reverse.rs:349), plus js_array_fill_range (:385) and js_array_fill_generic (:467) — arr.fill(s) writes the same source into every slot (aliasing the source and all filled slots to each other).
  • js_array_with (crates/perry-runtime/src/array/immutable.rs:232) — arr.with(i, s) stores the source into the new array's slot.
  • js_array_from_jsvalue (crates/perry-runtime/src/array/jsvalue_api.rs) — mixed-type array-literal construction via direct ptr::write; the JSValue sibling of the already-covered js_array_from_values.

Already covered (no action needed)

  • The other jsvalue_api wrappers — js_array_set, js_array_push, js_array_set_jsvalue / js_array_set_jsvalue_extend, js_array_push_jsvalue — delegate to the covered f64 functions, so they're covered transitively.
  • Internal reshuffles (sort, splice tail shift, slice copy, copyWithin) only move values already stored in the array — already shared — so no demote is needed (same reasoning as in fix(runtime,codegen): demote unique strings stored into array elements #5548).

Repro shape

The same shape the #5548 tests use should reproduce on each uncovered path (verify before fixing — I flagged these by inspection, not a run):

let s = "prefix";   // non-SSO, shared literal
s += "_init";       // append on shared -> fresh heap string, refcount==1
const a = ["x"];
a.unshift(s);       // (or a.fill(s) / a.with(0, s)) -> aliases the slot
s += "_more";       // refcount==1 in-place append -> must NOT corrupt the stored element
console.log("a0=" + a[0] + " s=" + s);  // expect a0=prefix_init s=prefix_init_more

Suggested fix

Add crate::string::js_string_addref_if_heap_string(value) at the top of each of the runtime functions above (before the element write), mirroring js_array_push_f64 / js_array_set_f64 in #5548. For fill, demote once before the loop. Extend crates/perry/tests/string_append_heap_alias.rs with a regression test per path (unshift / fill / with / mixed-type literal), each confirmed to fail without the demote.

Refs #5533, #5548.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions