From 7988534651895c99598f972c52138cdd65669714 Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 14 Aug 2026 23:48:06 -0400 Subject: [PATCH 1/3] fix(array): a typed array is not concat-spreadable, and must not be dropped `[1,2].concat(new Uint8Array([3,4]))` returned `[1,2]` where node gives `[1,2,Uint8Array(2)]`. The argument disappeared silently. Two defects stacked. A typed array is NOT concat-spreadable -- IsConcatSpreadable falls back to IsArray, which is false for one -- but js_array_is_array answers true here, so append_concat_arg took the spread branch rather than appending a single element. That spread ran through js_array_concat, whose clean_arr_ptr nulls tracked typed arrays, so it contributed nothing. Neither was even reached: dense_concat_array_source cleaned the argument first, read null as 'empty dense source', and returned from the bulk path, so the spec-shaped flow never ran. Its own typed-array rejection sits BELOW that clean and was unreachable for the values it names -- the same ordering bug as #8090, and the same shape as the subclass hazard documented in the comment directly above it. The spread accumulator is deliberately untouched: [...typedArray] must keep materializing elements, and reordering there would have traded a dropped argument for a wrong element count. Verified against node 26.5.1 across u8/i32/f64 arguments, an empty typed array, a multi-argument mix, an empty receiver, plus controls for plain concat, nested arrays, strings, Set spread and typed-array spread. --- .../2879-concat-typed-array-not-spreadable.md | 41 +++++++++++++++++++ crates/perry-runtime/src/array/from_concat.rs | 39 +++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 changelog.d/2879-concat-typed-array-not-spreadable.md diff --git a/changelog.d/2879-concat-typed-array-not-spreadable.md b/changelog.d/2879-concat-typed-array-not-spreadable.md new file mode 100644 index 0000000000..8d75628fd7 --- /dev/null +++ b/changelog.d/2879-concat-typed-array-not-spreadable.md @@ -0,0 +1,41 @@ +### Fixed + +- **`Array.prototype.concat` no longer drops a typed-array argument.** + `[1, 2].concat(new Uint8Array([3, 4]))` returned `[1, 2]`; node returns + `[1, 2, Uint8Array(2)]`. The argument vanished with no error and no + diagnostic. + + Two defects stacked. A typed array is **not** concat-spreadable — the spec's + `IsConcatSpreadable` falls back to `IsArray`, which is false for a TypedArray + — but this runtime's `js_array_is_array` answers true for one, so + `append_concat_arg` took the spread branch instead of appending a single + element. That spread then ran through `js_array_concat`, whose + `clean_arr_ptr` nulls every tracked typed array, so it contributed nothing. + + Before either could be reached, the all-dense bulk path in + `dense_concat_array_source` cleaned the argument first: `clean_arr_ptr` + returned null, the `src.is_null()` arm reported "empty dense source", and the + bulk path returned early — so the spec-shaped flow never ran at all. The + typed-array rejection that function already carries sits BELOW that clean and + was unreachable for exactly the values it names. + + This is the same shape as the comment immediately above it, which describes + a `class X extends Array` argument being mis-classified as an empty dense + source and silently dropped. + + Affected files: + + - `crates/perry-runtime/src/array/from_concat.rs` — reject typed arrays and + registered buffers in `dense_concat_array_source` before the clean, and + append them as one element in `append_concat_arg`. + + The spread accumulator (`js_array_concat`) is deliberately untouched: + `[...new Uint8Array([5, 6])]` must keep materializing elements, and + "fixing" the ordering there instead would have traded a dropped argument for + a wrong element count. + + Validation: byte-compared against node 26.5.1 across `Uint8Array`, + `Int32Array` and `Float64Array` arguments, an empty typed array, a + multi-argument call mixing plain arrays and a typed array, an empty receiver, + and controls for plain-array concat, nested arrays, string elements, Set + spread, and `[...typedArray]` spread — all matching. diff --git a/crates/perry-runtime/src/array/from_concat.rs b/crates/perry-runtime/src/array/from_concat.rs index 43ee8035c8..210f980468 100644 --- a/crates/perry-runtime/src/array/from_concat.rs +++ b/crates/perry-runtime/src/array/from_concat.rs @@ -331,7 +331,26 @@ pub(crate) fn append_concat_arg(result: *mut ArrayHeader, value: f64) -> *mut Ar return append_spread_array(result, snap_ptr); } - // Arrays (and set/map/typed-array/buffer that concat treats array-like via + // #2879: a typed array is NOT concat-spreadable. `IsConcatSpreadable` + // (ECMA-262 §23.1.3.1 step 2) falls back to `IsArray`, and `IsArray` is + // FALSE for a TypedArray — node appends `new Uint8Array([3,4])` as one + // element, giving `[1,2,Uint8Array(2)]`. + // + // Checked before the `is_array` branch because this runtime's + // `js_array_is_array` answers true for a typed array, so it took the spread + // path — and that spread runs through `js_array_concat`, whose + // `clean_arr_ptr` nulls a tracked typed array. The argument contributed + // nothing and vanished: `[1,2].concat(u8)` returned `[1,2]`. An explicit + // `@@isConcatSpreadable === true` still opts in below, which is the one way + // the spec does spread one. + if spreadable != Some(true) + && (crate::typedarray::lookup_typed_array_kind(raw_addr).is_some() + || crate::buffer::is_registered_buffer(raw_addr)) + { + return js_array_push_f64(result, value); + } + + // Arrays (and set/map/buffer that concat treats array-like via // js_array_concat) spread by default, unless @@isConcatSpreadable === false. let is_array = js_array_is_array(value).to_bits() == 0x7FFC_0000_0000_0004; if is_array { @@ -857,6 +876,24 @@ unsafe fn dense_concat_array_source(src: *const ArrayHeader) -> Option<(*const A if crate::array::subclass::raw_receiver_is_heap_object(src) { return None; } + // #2879: the same silent-drop hazard the comment above describes, for a + // typed array rather than a subclass. `clean_arr_ptr` nulls every tracked + // typed array, the `src.is_null()` arm below then reports "empty dense + // source", and the bulk path returns `Some(out)` — so the spec-shaped + // `append_concat_arg` flow never runs and the argument disappears. + // `[1,2].concat(new Uint8Array([3,4]))` yielded `1,2`. + // + // The rejection further down covers exactly these registries; it is simply + // BELOW the clean, which makes it unreachable for the values it names. + // Asking first is what lets the caller fall through to the spec path, where + // a typed array is appended as ONE element because `IsArray` is false for + // it. + let raw_before_clean = crate::array::array_receiver_addr(src as *mut ArrayHeader); + if crate::typedarray::lookup_typed_array_kind(raw_before_clean).is_some() + || crate::buffer::is_registered_buffer(raw_before_clean) + { + return None; + } let src = clean_arr_ptr(src); if src.is_null() { return Some((src, 0)); From c5dd81bf926f11267179fdb4c7630316fa6246e1 Mon Sep 17 00:00:00 2001 From: jdalton Date: Sat, 15 Aug 2026 01:51:42 -0400 Subject: [PATCH 2/3] review: filter the handle band before the registry lookups array_receiver_addr only strips the NaN-box tag and neither registry helper validates what it is handed, so gate both lookups on crate::value::addr_class::is_plausible_heap_addr rather than open-coding a floor. Same canonical predicate the typed_array_receiver funnel uses. Re-verified after the change: the concat matrix still matches node 26.5.1 across u8/i32 arguments, an empty typed array, a multi-argument mix and an empty receiver, with plain-concat, nested-array and typed-array-spread controls unchanged. --- crates/perry-runtime/src/array/from_concat.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/perry-runtime/src/array/from_concat.rs b/crates/perry-runtime/src/array/from_concat.rs index 210f980468..f646c45898 100644 --- a/crates/perry-runtime/src/array/from_concat.rs +++ b/crates/perry-runtime/src/array/from_concat.rs @@ -889,8 +889,12 @@ unsafe fn dense_concat_array_source(src: *const ArrayHeader) -> Option<(*const A // a typed array is appended as ONE element because `IsArray` is false for // it. let raw_before_clean = crate::array::array_receiver_addr(src as *mut ArrayHeader); - if crate::typedarray::lookup_typed_array_kind(raw_before_clean).is_some() - || crate::buffer::is_registered_buffer(raw_before_clean) + // `array_receiver_addr` only strips the NaN-box tag, and neither registry + // helper validates what it is handed, so filter the handle band with the + // canonical predicate first rather than open-coding a floor here. + if crate::value::addr_class::is_plausible_heap_addr(raw_before_clean) + && (crate::typedarray::lookup_typed_array_kind(raw_before_clean).is_some() + || crate::buffer::is_registered_buffer(raw_before_clean)) { return None; } From 3b7e0168a97bbe59ba5098ded8b781f4d66d73a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 15 Aug 2026 10:06:49 +0200 Subject: [PATCH 3/3] fix(typedarray): a Symbol key must do OrdinarySet, not be read as an index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `u8[sym] = 5` was dropped in silence. ECMA-262 §10.4.5.5 routes a key that is not a CanonicalNumericIndexString to OrdinarySet, and a Symbol is definitionally not one — but `typed_array_set_numeric_index` could not tell the two apart. A Symbol arrives as a NaN-boxed pointer, which as an f64 is a NaN, so it took the "canonical-invalid index" arm and returned `true`: write handled. The property never existed; the same code on a plain object, a plain array and a Buffer all worked. This is what made the `@@isConcatSpreadable === true` opt-in this PR documents unreachable by assignment. `concat` already honours the flag correctly — `Object.defineProperty` gives node's answer — but the assignment form could not install it. Ask the key-kind question before either typed-array arm claims the receiver, and make the numeric arm decline a key it cannot classify. Note on test shape: the obvious end-to-end unit test PASSES WITHOUT THE FIX, because a direct call to the polymorphic helper reaches a different sub-arm than the compiled path. Measured, not assumed. The end-to-end coverage is therefore a gap test byte-compared against node; the unit test asserts the contract change, and fails without it. --- .../2879-concat-typed-array-not-spreadable.md | 37 ++++++++++ crates/perry-runtime/src/object/mod.rs | 2 + .../src/object/polymorphic_index.rs | 24 +++++++ .../object/polymorphic_index_symbol_tests.rs | 70 +++++++++++++++++++ crates/perry-runtime/src/typedarray_props.rs | 20 ++++++ test-files/test_gap_typed_array_symbol_key.ts | 42 +++++++++++ 6 files changed, 195 insertions(+) create mode 100644 crates/perry-runtime/src/object/polymorphic_index_symbol_tests.rs create mode 100644 test-files/test_gap_typed_array_symbol_key.ts diff --git a/changelog.d/2879-concat-typed-array-not-spreadable.md b/changelog.d/2879-concat-typed-array-not-spreadable.md index 8d75628fd7..eb48413c65 100644 --- a/changelog.d/2879-concat-typed-array-not-spreadable.md +++ b/changelog.d/2879-concat-typed-array-not-spreadable.md @@ -39,3 +39,40 @@ multi-argument call mixing plain arrays and a typed array, an empty receiver, and controls for plain-array concat, nested arrays, string elements, Set spread, and `[...typedArray]` spread — all matching. + +- **A `Symbol` key on a typed-array receiver was dropped in silence**, which is + why the `@@isConcatSpreadable` opt-in above could not be exercised by + assignment. ECMA-262 §10.4.5.5 routes a key that is not a + CanonicalNumericIndexString to OrdinarySet, and a `Symbol` is definitionally + not one — but `typed_array_set_numeric_index` could not tell the two apart. A + `Symbol` arrives as a NaN-boxed pointer, which AS AN `f64` is a NaN, so it + took the "canonical-invalid index" arm, coerced the value for side effects, + and returned `true` meaning "write handled". The store vanished: + `u8[sym] = 5` then read back `undefined` and + `Object.getOwnPropertySymbols(u8)` stayed empty, while the identical code on + a plain object, a plain array and a `Buffer` all worked. + + Same shape as #8090/#8109/#8119/#8120/#8141: a receiver-specific fast path + claims the operation before the key-kind question is asked. + + - `crates/perry-runtime/src/object/polymorphic_index.rs` — ask the key-kind + question before either typed-array arm claims the receiver, and route a + `Symbol` to the symbol side table, where `js_put_value_set` and + `js_array_set_index_or_string` already put it. Gated on the receiver, and + on BOTH typed-array registries, since either arm alone would still claim + the write. + - `crates/perry-runtime/src/typedarray_props.rs` — make the numeric-index + arm's contract honest: decline a key it cannot classify instead of + reporting it handled. Inert for this module's own callers, which reach it + only under `is_int32()` / `is_finite()`. + + This makes the `@@isConcatSpreadable === true` opt-in documented above + actually reachable by assignment: `[1].concat(u8)` with the flag set now + gives node's `[1,9,10]`. + + Validation: `test-files/test_gap_typed_array_symbol_key.ts` byte-compared + against node 26.5.1 across all four receiver kinds, the element-store + control, both opt-in forms and the default. Sabotage: with the routing + removed the compiled probe diverges from node (`ta set/get: undefined | + ownSyms: 0`); with the numeric-arm guard removed the unit test fails. + `perry-runtime --lib` 2385 passed / 0 failed. diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 2041ac1405..a7b6a0c5d5 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -119,6 +119,8 @@ mod object_ops; pub(crate) use object_ops::{ensure_key_in_keys_array, install_builtin_getter}; mod object_ops_frozen; mod polymorphic_index; +#[cfg(test)] +mod polymorphic_index_symbol_tests; mod primitive_proto_thunks; mod property_key; pub(crate) mod prototype_chain; diff --git a/crates/perry-runtime/src/object/polymorphic_index.rs b/crates/perry-runtime/src/object/polymorphic_index.rs index 08f0f54246..1c5c6e9bc7 100644 --- a/crates/perry-runtime/src/object/polymorphic_index.rs +++ b/crates/perry-runtime/src/object/polymorphic_index.rs @@ -316,6 +316,30 @@ pub extern "C" fn js_object_set_index_polymorphic(obj_handle: i64, idx: f64, val if raw < 0x1000 { return; } + // Ask the KEY-KIND question before either typed-array arm claims the + // receiver. A Symbol is definitionally not a CanonicalNumericIndexString, + // so ECMA-262 §10.4.5.5 requires OrdinarySet — the symbol side table, the + // same place `js_put_value_set` and `js_array_set_index_or_string` already + // put it. Without this, `typed_array_set_numeric_index` read the NaN-boxed + // symbol pointer as a non-finite f64, classified it "canonical-invalid + // index", and returned "handled" — so `u8[sym] = v` was dropped silently: + // the store never landed, `u8[sym]` read back `undefined`, and + // `Object.getOwnPropertySymbols(u8)` stayed empty while the same code on a + // plain object, a plain array and a Buffer all worked. + // + // The visible consequence was `@@isConcatSpreadable`: `concat` honours the + // opt-in correctly (`Object.defineProperty` proves it), but the assignment + // form could never install the property, so a typed array could not opt in. + // + // Gated on the receiver so only the broken case changes: BOTH registries + // are consulted, because either arm alone would still claim the write. + if unsafe { crate::symbol::js_is_symbol(idx) } != 0 + && (crate::typedarray::lookup_typed_array_kind(raw as usize).is_some() + || crate::typedarray_props::is_typed_array_owner(raw as usize)) + { + unsafe { crate::symbol::js_object_set_symbol_property(boxed, idx, value) }; + return; + } // #5525 fast path: a cached typed-array kind lookup + inline store, before // the thread-local `typed_array_set_numeric_index` registry dispatch // (`typed_array_owner_*` → `_tlv_get_addr`) that dominated the bcrypt diff --git a/crates/perry-runtime/src/object/polymorphic_index_symbol_tests.rs b/crates/perry-runtime/src/object/polymorphic_index_symbol_tests.rs new file mode 100644 index 0000000000..532b5af09d --- /dev/null +++ b/crates/perry-runtime/src/object/polymorphic_index_symbol_tests.rs @@ -0,0 +1,70 @@ +//! A Symbol key on a typed-array receiver must do OrdinarySet — it must not be +//! swallowed by the numeric-index arms of the computed-store path. +//! +//! ECMA-262 §10.4.5.5: an Integer-Indexed exotic object routes a key that is +//! NOT a CanonicalNumericIndexString to OrdinarySet. A Symbol is definitionally +//! not one. `typed_array_set_numeric_index` could not tell the two apart — a +//! Symbol arrives as a NaN-boxed pointer, which AS AN f64 is a NaN, so it took +//! the "canonical-invalid index" arm, coerced for side effects, and returned +//! `true` meaning "write handled". The store was dropped in silence. +//! +//! # Why this test is shaped as a contract assertion +//! +//! The obvious end-to-end shape — allocate a typed array, call +//! `js_object_set_index_polymorphic` with a symbol key, read it back — PASSES +//! WITHOUT THE FIX and is therefore worthless. Measured: with the routing +//! removed, the direct call still stored the property, while the same source +//! compiled and run diverged from node. The direct call reaches a different +//! sub-arm than the compiled path does, so it cannot witness the bug. +//! +//! The end-to-end coverage therefore lives in +//! `test-files/test_gap_typed_array_symbol_key.ts`, which is byte-compared +//! against node and does fail without the fix. What is left here is the piece a +//! unit test CAN witness: that the numeric-index arm no longer claims a key it +//! cannot classify. + +use crate::typedarray::{typed_array_alloc, KIND_UINT8}; + +/// The numeric-index arm must decline a Symbol key rather than report it +/// handled. Pre-fix this returned `true` and the write vanished. +#[test] +fn the_numeric_index_arm_does_not_claim_a_symbol_key() { + let _serialized = crate::array::test_serialize(); + let ta = typed_array_alloc(KIND_UINT8, 2); + crate::typedarray::js_typed_array_set(ta, 0, 1.0); + crate::typedarray::js_typed_array_set(ta, 1, 2.0); + + let sym = unsafe { crate::symbol::js_symbol_new_empty() }; + assert_ne!( + unsafe { crate::symbol::js_is_symbol(sym) }, + 0, + "precondition: the key under test must actually be a Symbol" + ); + + let claimed = + unsafe { crate::typedarray_props::typed_array_set_numeric_index(ta as usize, sym, 5.0) }; + assert!( + !claimed, + "a Symbol is not a CanonicalNumericIndexString, so the numeric-index \ + arm must decline it and let the caller route it to OrdinarySet; \ + pre-fix it read the NaN-boxed symbol as a non-finite f64, classified \ + it a canonical-invalid index, and reported the write handled" + ); +} + +/// The control that keeps the guard honest: a real out-of-bounds numeric index +/// must STILL be claimed and dropped per spec. Without this, making the +/// function decline everything would satisfy the test above. +#[test] +fn the_numeric_index_arm_still_claims_an_out_of_bounds_numeric_key() { + let _serialized = crate::array::test_serialize(); + let ta = typed_array_alloc(KIND_UINT8, 2); + + let claimed = + unsafe { crate::typedarray_props::typed_array_set_numeric_index(ta as usize, 99.0, 5.0) }; + assert!( + claimed, + "an out-of-bounds CanonicalNumericIndexString is still the numeric \ + arm's to handle — it is dropped per spec, not routed to OrdinarySet" + ); +} diff --git a/crates/perry-runtime/src/typedarray_props.rs b/crates/perry-runtime/src/typedarray_props.rs index 24c0c0f223..2dfd63e13c 100644 --- a/crates/perry-runtime/src/typedarray_props.rs +++ b/crates/perry-runtime/src/typedarray_props.rs @@ -566,10 +566,30 @@ unsafe fn typed_array_coerce_element_for_side_effects(owner: usize, value: f64) } } +/// True when `owner` is a typed-array receiver by EITHER registry — the +/// cached-kind lookup used by the inline fast path, or the thread-local owner +/// registry this module's slow dispatch gates on. Callers routing a key AWAY +/// from the numeric-index arms need both, because either one alone leaves the +/// other arm free to claim the write. +pub(crate) fn is_typed_array_owner(owner: usize) -> bool { + typed_array_owner_kind(owner).is_some() +} + pub(crate) unsafe fn typed_array_set_numeric_index(owner: usize, index: f64, value: f64) -> bool { if typed_array_owner_kind(owner).is_none() { return false; } + // A Symbol key is NOT a CanonicalNumericIndexString, so ECMA-262 + // §10.4.5.5 sends it to OrdinarySet — it is not an invalid index to be + // dropped. The `is_finite` test below cannot tell the two apart: a Symbol + // arrives as a NaN-boxed pointer, which AS AN f64 is a NaN, so it took the + // "canonical-invalid index" arm and returned `true` (write handled), and + // `u8[sym] = v` vanished with no error. Say "not mine" instead, so the + // caller can route it. Inert for this module's own callers, which reach + // here only under `is_int32()` / `is_finite()`. + if crate::symbol::js_is_symbol(index) != 0 { + return false; + } if !index.is_finite() || index.fract() != 0.0 || index < 0.0 || index > u32::MAX as f64 { // Canonical-invalid index: coerce the value for side effects, then drop. typed_array_coerce_element_for_side_effects(owner, value); diff --git a/test-files/test_gap_typed_array_symbol_key.ts b/test-files/test_gap_typed_array_symbol_key.ts new file mode 100644 index 0000000000..9cf84972c7 --- /dev/null +++ b/test-files/test_gap_typed_array_symbol_key.ts @@ -0,0 +1,42 @@ +// A Symbol key on a typed-array receiver must do OrdinarySet (ECMA-262 +// §10.4.5.5: a key that is not a CanonicalNumericIndexString is not an index). +// Perry's computed-store path let the numeric-index arm claim the write: a +// Symbol is a NaN-boxed pointer, which as an f64 is a NaN, so it was +// classified a "canonical-invalid index" and dropped in silence. + +const s: any = Symbol("x"); + +// Every receiver kind must behave the same. Only the typed array was broken. +const o: any = {}; +o[s] = 5; +console.log("obj:", o[s], Object.getOwnPropertySymbols(o).length); + +const arr: any = [1, 2]; +arr[s] = 5; +console.log("arr:", arr[s], Object.getOwnPropertySymbols(arr).length); + +const buf: any = Buffer.alloc(2); +buf[s] = 5; +console.log("buf:", buf[s], Object.getOwnPropertySymbols(buf).length); + +const u8: any = new Uint8Array([1, 2]); +u8[s] = 5; +console.log("u8:", u8[s], Object.getOwnPropertySymbols(u8).length); + +// The element store through the same helper must be undisturbed. +u8[1] = 9; +console.log("elements:", u8[0], u8[1]); + +// The user-visible consequence: a typed array could not opt in to +// @@isConcatSpreadable by assignment, though defineProperty worked. +const a: any = new Uint8Array([9, 10]); +a[Symbol.isConcatSpreadable] = true; +console.log("optin readback:", a[Symbol.isConcatSpreadable]); +console.log("optin concat:", JSON.stringify([1].concat(a))); + +const b: any = new Uint8Array([9, 10]); +Object.defineProperty(b, Symbol.isConcatSpreadable, { value: true, configurable: true }); +console.log("defineProperty concat:", JSON.stringify([1].concat(b))); + +// Default (no opt-in): a typed array is NOT concat-spreadable. +console.log("default concat:", JSON.stringify([1, 2].concat(new Uint8Array([3, 4]))));