From ef5e913c399702086f7780c1706e12141847fb10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 27 Jul 2026 05:56:30 +0200 Subject: [PATCH] fix(string): stop mask-casting SSO values to StringHeader* (#6887) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Indexing or iterating a SHORT string produced by concatenation segfaulted: const s = "ab" + "c"; s.length // 3 fine s[0] // SIGSEGV for (const ch of s) {} // SIGSEGV Array.from(s) // SIGSEGV A short string is an inline SHORT_STRING_TAG (SSO) JSValue whose payload IS the characters, not a heap address. Two places masked its low 48 bits and dereferenced the result as a StringHeader*: - codegen's `s[i]` fast path (expr/index_get.rs) `unbox_to_i64`'d the receiver before calling js_string_index_get, and - js_array_from_value did the same mask itself, behind a `(bits >> 48) >= 0x7FF8` test that an SSO value passes. String literals, join() results, and long concatenations were all fine — literals are heap-allocated at init and a long concat exceeds the SSO threshold. That is the clean discriminator: a 3-char concat crashes, a 64-char concat does not. `typeof`, `.length` and printing the value work either way, so nothing looks wrong until something indexes it. Codegen now passes the receiver STILL BOXED to a new js_string_index_get_boxed, which decides by tag; an SSO receiver is materialized to a heap StringHeader and delegated, so the CanonicalNumericIndexString key semantics stay in exactly one place. js_array_from_value normalizes an SSO receiver up front. The materialization costs a small arena allocation per index on an SSO receiver — worth revisiting if it shows up hot, but the alternative was a crash. Test: test_gap_sso_concat_string_index.ts covers indexed reads (in range, out of range, negative), charAt/charCodeAt/codePointAt/at, an index-accumulation loop, for-of, spread, Array.from, split, the `join("") + "\n"` shape this was found on, empty and single-char concatenations, non-ASCII where UTF-16 indexing differs from bytes, and a long heap-backed concatenation so the fix cannot regress that path. Byte-identical to node. Regression: 120-file test_gap_* differential vs node is 115 pass / 2 fail / 3 skip, both failures already in known_failures.json. Found compiling the Milo compiler, whose codegen builds a printf format with `partFmts.join("") + "\n"` (5 chars, SSO) and then iterates it, so every Milo program importing std/platform, std/os, std/sync, std/runtime or std/event died. With this fix Milo's example suite goes from 4/23 to 22/23 byte-identical against bun. --- changelog.d/6887-sso-concat-string-index.md | 1 + crates/perry-codegen/src/expr/index_get.rs | 11 +++- .../src/runtime_decls/strings.rs | 4 ++ crates/perry-runtime/src/array/from_concat.rs | 13 ++++ crates/perry-runtime/src/string/char_ops.rs | 36 ++++++++++ .../test_gap_sso_concat_string_index.ts | 65 +++++++++++++++++++ 6 files changed, 127 insertions(+), 3 deletions(-) create mode 100644 changelog.d/6887-sso-concat-string-index.md create mode 100644 test-files/test_gap_sso_concat_string_index.ts diff --git a/changelog.d/6887-sso-concat-string-index.md b/changelog.d/6887-sso-concat-string-index.md new file mode 100644 index 0000000000..188d33ed0c --- /dev/null +++ b/changelog.d/6887-sso-concat-string-index.md @@ -0,0 +1 @@ +**Fix SIGSEGV indexing or iterating a short concatenated string (#6887):** `("ab" + "c")[0]`, `for (const ch of a + b)`, `Array.from(a + b)`, `[...s]` and `s.split("")` all segfaulted when the concatenation was short enough to be small-string-optimized. A short string is an inline `SHORT_STRING_TAG` JSValue whose payload *is* the characters, but codegen's `s[i]` fast path mask-unboxed the receiver to a `StringHeader*`, and `js_array_from_value` performed the same mask itself behind a `(bits >> 48) >= 0x7FF8` test that an SSO value passes — both then dereferenced the packed characters as an address. String literals, `join()` results and long (heap-backed) concatenations were unaffected, which is why this survived: a 3-char concat crashed while a 64-char one did not, and `typeof`, `.length` and printing the value all worked first. Codegen now passes the receiver still boxed to a new `js_string_index_get_boxed`, which decides by tag, and `js_array_from_value` materializes an SSO receiver before extracting pointers. This was the blocker behind #6872 and the last defect stopping the Milo compiler from building and running Milo programs under Perry. diff --git a/crates/perry-codegen/src/expr/index_get.rs b/crates/perry-codegen/src/expr/index_get.rs index 9251185e4e..13eacc051c 100644 --- a/crates/perry-codegen/src/expr/index_get.rs +++ b/crates/perry-codegen/src/expr/index_get.rs @@ -1596,17 +1596,22 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let s_box = lower_expr(ctx, object)?; let idx_d = lower_expr(ctx, index)?; let blk = ctx.block(); - let s_handle = unbox_to_i64(blk, &s_box); // #3987: route through the canonical-index runtime helper (it // takes the raw NaN-boxed key, not an `fptosi`'d i32) so a valid // array index returns its char and every non-canonical key // (`NaN`, `1.5`, negatives, OOB, `"01"`, non-numeric strings) // returns `undefined` — matching ECMAScript / Node — instead of // truncating the index and returning `""` for OOB. + // Pass the receiver STILL BOXED. Unboxing here masked off the + // low 48 bits, which is only a pointer for a heap STRING_TAG + // value — an inline SHORT_STRING_TAG (SSO) value's payload is + // the characters themselves, so the mask produced a garbage + // pointer and `(a + b)[0]` segfaulted on any short + // concatenation. The boxed entry point decides by tag. return Ok(blk.call( DOUBLE, - "js_string_index_get", - &[(I64, &s_handle), (DOUBLE, &idx_d)], + "js_string_index_get_boxed", + &[(DOUBLE, &s_box), (DOUBLE, &idx_d)], )); } // #6750 follow-up: a masked-window fact (dense range-loop or diff --git a/crates/perry-codegen/src/runtime_decls/strings.rs b/crates/perry-codegen/src/runtime_decls/strings.rs index f9fc5768af..c6b937c41a 100644 --- a/crates/perry-codegen/src/runtime_decls/strings.rs +++ b/crates/perry-codegen/src/runtime_decls/strings.rs @@ -630,6 +630,10 @@ pub fn declare_phase_b_strings(module: &mut LlModule) { // #3987: `s[key]` canonical-index read — returns the char (NaN-boxed string) // for a valid array index, else NaN-boxed `undefined`. Takes the raw key. module.declare_function("js_string_index_get", DOUBLE, &[I64, DOUBLE]); + // SSO-safe variant: takes the receiver NaN-boxed so an inline + // SHORT_STRING_TAG value is decoded by tag instead of being mask-cast into + // a bogus pointer (which segfaulted on `(a + b)[0]`). + module.declare_function("js_string_index_get_boxed", DOUBLE, &[DOUBLE, DOUBLE]); // #2787: NaN-safe JS index coercion (undefined/NaN -> 0, trunc, clamp) for // the char-access methods, replacing a raw `fptosi` that is UB on a NaN. module.declare_function("js_string_index_to_i32", I32, &[DOUBLE]); diff --git a/crates/perry-runtime/src/array/from_concat.rs b/crates/perry-runtime/src/array/from_concat.rs index d3ed84bfd1..ce0884edca 100644 --- a/crates/perry-runtime/src/array/from_concat.rs +++ b/crates/perry-runtime/src/array/from_concat.rs @@ -81,6 +81,19 @@ pub extern "C" fn js_array_from_value(boxed: f64) -> *mut ArrayHeader { if bits == TAG_NULL { throw_not_iterable("object null"); } + // An inline SHORT_STRING_TAG (SSO) value's payload is the characters + // themselves, not an address, but it passes the `>= 0x7FF8` test below and + // the mask then yields a bogus pointer — `Array.from("ab" + "c")` + // segfaulted. Materialize to a heap StringHeader so every pointer + // extraction downstream is valid; the per-codepoint path in + // `js_array_clone` then behaves exactly as it does for a literal. + let jsval = crate::value::JSValue::from_bits(bits); + if jsval.is_short_string() { + let hdr = crate::string::js_string_materialize_to_heap(boxed); + if !hdr.is_null() { + return js_array_from_value(crate::value::js_nanbox_string(hdr as i64)); + } + } // #6454: `Array.from(SomeClass)` where the class DECLARATION (an // INT32-tagged ClassRef) carries a — possibly inherited, #36/#321 — // `[Symbol.iterator]`: drive it. A class WITHOUT one falls through to the diff --git a/crates/perry-runtime/src/string/char_ops.rs b/crates/perry-runtime/src/string/char_ops.rs index 00c2b009ff..9bdf5cbba0 100644 --- a/crates/perry-runtime/src/string/char_ops.rs +++ b/crates/perry-runtime/src/string/char_ops.rs @@ -118,6 +118,42 @@ fn utf16_unit_at(s: *const StringHeader, idx: usize) -> Option { None } +/// SSO-safe `s[key]`: takes the receiver as a **NaN-boxed JSValue** rather than +/// an already-unboxed `StringHeader*`. +/// +/// Codegen's `s[i]` fast path used to `unbox_to_i64` the receiver — masking off +/// the low 48 bits — and hand that to [`js_string_index_get`]. That is only +/// correct for a heap `STRING_TAG` value. A short string is an inline +/// `SHORT_STRING_TAG` value whose payload IS the characters, not an address, so +/// masking produced a garbage pointer and the read segfaulted. Concatenation is +/// the common way to produce one (`"ab" + "c"`), which made plain +/// `for (const ch of a + b)` / `(a + b)[0]` crash while the same operations on a +/// string literal or a `join()` result were fine. +/// +/// Short receivers are materialized to a heap `StringHeader` and delegated, so +/// the CanonicalNumericIndexString key semantics below stay in exactly one +/// place. That costs a small arena allocation per index on an SSO receiver; +/// worth revisiting if it shows up hot, but the alternative was a crash. +#[no_mangle] +pub extern "C" fn js_string_index_get_boxed(value: f64, key: f64) -> f64 { + const UNDEFINED: f64 = f64::from_bits(crate::value::TAG_UNDEFINED); + let jsval = crate::value::JSValue::from_bits(value.to_bits()); + if jsval.is_short_string() { + let hdr = crate::string::js_string_materialize_to_heap(value); + if hdr.is_null() { + return UNDEFINED; + } + return js_string_index_get(hdr, key); + } + // Heap strings and every non-string receiver keep the existing behavior: + // `js_string_index_get` already guards invalid pointers and delegates + // non-string heap objects to the polymorphic index path. + js_string_index_get( + (value.to_bits() & crate::value::POINTER_MASK) as *const StringHeader, + key, + ) +} + /// `s[key]` indexed read with ECMAScript CanonicalNumericIndexString semantics /// (#3987): returns the single-UTF-16-code-unit string at `key` only when `key` /// is a canonical array index — a non-negative integer (or a numeric string diff --git a/test-files/test_gap_sso_concat_string_index.ts b/test-files/test_gap_sso_concat_string_index.ts new file mode 100644 index 0000000000..cf18467472 --- /dev/null +++ b/test-files/test_gap_sso_concat_string_index.ts @@ -0,0 +1,65 @@ +// A short string built by concatenation is an inline SHORT_STRING_TAG (SSO) +// JSValue whose payload IS the characters, not a heap address. Codegen's `s[i]` +// fast path mask-unboxed the receiver to a `StringHeader*`, and +// `js_array_from_value` did the same mask itself, so both produced a bogus +// pointer and segfaulted. `"ab" + "c"` is the ordinary way to make one, which +// made `(a + b)[0]`, `for (const ch of a + b)` and `Array.from(a + b)` crash +// while the identical operations on a literal or a `join()` result were fine. +// +// Long concatenations exceed the SSO threshold and were always heap-backed — +// they are covered here so the fix cannot regress the heap path. + +const a = "ab"; +const b = "c"; +const short = a + b; + +console.log(typeof short, short.length, short); + +// --- indexed reads on a short concatenation ------------------------------- +console.log(short[0], short[1], short[2]); +console.log(String(short[3])); // undefined, out of range +console.log(String(short[-1])); // undefined, negative +console.log(short.charAt(1), short.charCodeAt(1), short.codePointAt(1)); +console.log(short.at(0), short.at(-1)); + +// index-loop accumulation (the shape milo's codegen uses) +let viaIndex = ""; +for (let i = 0; i < short.length; i++) viaIndex += short[i]; +console.log(viaIndex); + +// --- iteration protocols -------------------------------------------------- +let viaForOf = 0; +for (const ch of short) viaForOf++; +console.log(viaForOf); +console.log([...short].join("-")); +console.log(Array.from(short).length, Array.from(short).join("|")); +console.log(short.split("").join("+")); + +// --- concatenation of a join result, exactly milo's shape ----------------- +const parts: string[] = []; +parts.push("%.*s"); +const fmt = parts.join("") + "\n"; +console.log(fmt.length); +let fmtChars = 0; +for (const ch of fmt) fmtChars++; +console.log(fmtChars); +console.log(fmt[0], fmt[1], JSON.stringify(fmt[4])); + +// --- empty and single-char concatenations --------------------------------- +const empty = "" + ""; +console.log(empty.length, String(empty[0]), Array.from(empty).length); +const one = "" + "x"; +console.log(one.length, one[0], Array.from(one).length); + +// --- non-ASCII, where UTF-16 indexing differs from bytes ------------------ +const uni = "é" + "ü"; +console.log(uni.length, uni[0], uni[1], Array.from(uni).length); + +// --- long (heap-backed) concatenation must still work --------------------- +const long = "a".repeat(40) + "b".repeat(40); +console.log(long.length, long[0], long[79], Array.from(long).length); + +// --- concatenation built in a loop ---------------------------------------- +let acc = ""; +for (const p of ["x", "y", "z"]) acc += p; +console.log(acc.length, acc[0], acc[2], Array.from(acc).join(""));