diff --git a/changelog.d/8443-string-repeat-reentrant-count.md b/changelog.d/8443-string-repeat-reentrant-count.md new file mode 100644 index 0000000000..1e0d7328bd --- /dev/null +++ b/changelog.d/8443-string-repeat-reentrant-count.md @@ -0,0 +1,3 @@ +### Fixed + +- `String.prototype.repeat()` no longer reads a stale string payload when the `count` argument's `valueOf`/`Symbol.toPrimitive` triggers a moving garbage collection. The receiver is now rooted across the coercion and its payload borrowed only afterwards, so a relocated receiver repeats correct bytes instead of from-space garbage. `padStart`/`padEnd` were audited for the same window and are unaffected — codegen runs both of their coercions before the receiver handle is re-read. diff --git a/crates/perry-runtime/src/string/pad.rs b/crates/perry-runtime/src/string/pad.rs index 4cd20a6325..89126ea3a1 100644 --- a/crates/perry-runtime/src/string/pad.rs +++ b/crates/perry-runtime/src/string/pad.rs @@ -299,14 +299,40 @@ pub extern "C" fn js_string_repeat(s: *const StringHeader, count_value: f64) -> return js_string_from_bytes("".as_ptr(), 0); } - let str_data = string_as_str(s); - let count_number = crate::builtins::js_number_coerce(count_value); + // `count` may be an object, so ToNumber runs its + // `valueOf`/`Symbol.toPrimitive` — arbitrary user JS, whose loop back-edge + // polls are moving-GC safepoints (default-on since #7721). Two distinct + // hazards follow, and the ordering below is what closes both (#8427): + // + // * The receiver's WTF-8 payload must NOT be borrowed across the + // coercion. `string_as_str` materializes a `&str` at the *pre-move* + // address; rooting rewrites slots, never an already-live borrow, so no + // root can repair one (`HeapKeyBytes`, `object/field_get_set.rs`). + // Pre-fix, an evacuating minor inside `valueOf` left `str_data` + // pointing at retired from-space and `repeat` copied garbage. + // * `s` itself is a raw pointer in a native Rust frame, which the + // collector does not scan by default — so deferring the borrow is not + // enough on its own. Park it in a transient root and take the + // *post-collection* address back out of the handle. + let scope = crate::gc::RuntimeHandleScope::new(); + let receiver_root = scope.root_string_ptr(s); + let (count_number, s) = receiver_root + .across_const::(|| crate::builtins::js_number_coerce(count_value)); + let count_integer = to_integer_or_infinity(count_number); if count_integer < 0.0 || count_integer.is_infinite() { throw_repeat_range_error(count_number); } - if count_integer == 0.0 || str_data.is_empty() { + // ECMA-262 §22.1.3.17 step 4 returns "" for n = 0 before the receiver is + // consulted at all; an empty receiver repeats to "" for every remaining n. + // Both are checked here so the payload is borrowed only on the path that + // actually reads it — and only once no user code is left to run. + if count_integer == 0.0 { + return js_string_from_bytes("".as_ptr(), 0); + } + let str_data = string_as_str(s); + if str_data.is_empty() { return js_string_from_bytes("".as_ptr(), 0); } diff --git a/test-files/test_gap_gc_string_repeat_reentrant_count.ts b/test-files/test_gap_gc_string_repeat_reentrant_count.ts new file mode 100644 index 0000000000..d2b751c7c0 --- /dev/null +++ b/test-files/test_gap_gc_string_repeat_reentrant_count.ts @@ -0,0 +1,133 @@ +// #8427: `String.prototype.repeat` used to borrow the receiver's inline WTF-8 +// payload BEFORE coercing `count`, and ToNumber on an object count runs user +// JS. A moving collection inside that `valueOf` relocated the receiver, so the +// borrow named retired from-space and the result was copied out of garbage +// ("repeat [invalid utf8]" under the instruments below). Keep the reentrant +// count shapes under moving-GC pressure so the ordering contract stays gated. +// parity-env: PERRY_GC_MOVING_LOOP_POLLS=1 PERRY_GC_SCHEDULE_SEED=8427 PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_SCHEDULE_ALLOC_KB=0 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1 PERRY_GC_PROTECT_FROMSPACE=1 + +// Allocate hard enough inside the callback to reach loop back-edge safepoints +// while the receiver is still young. +function churn(): number { + let junk = ""; + for (let i = 0; i < 25; i++) { + junk = junk + "x"; + } + return junk.length; +} + +let badRepeat = 0; +let callbackRuns = 0; +for (let i = 0; i < 3; i++) { + // A fresh, joined receiver is a young heap string, not SSO and not interned. + const subject = ["abc", "def", String(i % 10)].join(""); + const repeated = subject.repeat({ + valueOf() { + callbackRuns++; + churn(); + return 3; + }, + } as unknown as number); + if (repeated !== subject + subject + subject) badRepeat++; +} +console.log("repeat under reentrant count:", badRepeat === 0, callbackRuns); + +// `Symbol.toPrimitive` takes precedence over `valueOf` and is the same window. +let badPrimitive = 0; +for (let i = 0; i < 3; i++) { + const subject = ["uvw", "xyz", String(i % 10)].join(""); + const repeated = subject.repeat({ + [Symbol.toPrimitive]() { + churn(); + return 2; + }, + } as unknown as number); + if (repeated !== subject + subject) badPrimitive++; +} +console.log("repeat under toPrimitive count:", badPrimitive === 0); + +// A count of 0 returns "" without ever reading the receiver — the callback +// still runs, and still allocates, exactly once per call. +let zeroRuns = 0; +const zeroSubject = ["mno", "pqr"].join(""); +const zeroResult = zeroSubject.repeat({ + valueOf() { + zeroRuns++; + churn(); + return 0; + }, +} as unknown as number); +console.log("zero count:", JSON.stringify(zeroResult), zeroRuns); + +// ToIntegerOrInfinity is observable even when the receiver is empty, and a +// negative count must throw before repeat's empty-string return. +let emptyCoercions = 0; +try { + "".repeat({ + valueOf() { + emptyCoercions++; + churn(); + throw new Error("empty-count"); + }, + } as unknown as number); +} catch (error) { + console.log("empty throw:", emptyCoercions, (error as Error).message); +} + +try { + "".repeat({ + valueOf() { + emptyCoercions++; + churn(); + return -1; + }, + } as unknown as number); +} catch (error) { + console.log("empty negative:", emptyCoercions, error instanceof RangeError); +} + +// padStart/padEnd take the same shape of reentrant arguments, but their +// coercions (`js_number_coerce` for maxLength, `js_string_pad_fill` for the +// fill) are emitted by codegen BEFORE the receiver handle is re-read, so no +// user code runs inside the runtime helper while its payload is borrowed. +// Pin that ordering here so a future move of either coercion into the helper +// reintroduces #8427's window with a test already watching. +let badPadStart = 0; +let badPadEnd = 0; +for (let i = 0; i < 3; i++) { + const subject = ["pad", "me", String(i % 10)].join(""); + const started = subject.padStart( + { + valueOf() { + churn(); + return 12; + }, + } as unknown as number, + { + toString() { + churn(); + return "-"; + }, + } as unknown as string, + ); + const ended = subject.padEnd( + { + valueOf() { + churn(); + return 12; + }, + } as unknown as number, + { + toString() { + churn(); + return "+"; + }, + } as unknown as string, + ); + // Literal expectations: the receiver is 6 units, so 6 fill units are added. + // (Spelling these out keeps the pad assertions independent of `repeat`.) + if (started !== "------" + subject) badPadStart++; + if (ended !== subject + "++++++") badPadEnd++; +} +console.log("padStart under reentrant args:", badPadStart === 0); +console.log("padEnd under reentrant args:", badPadEnd === 0);