diff --git a/changelog.d/9797-array-hole-inherited-setters.md b/changelog.d/9797-array-hole-inherited-setters.md new file mode 100644 index 0000000000..6f0b92eb2c --- /dev/null +++ b/changelog.d/9797-array-hole-inherited-setters.md @@ -0,0 +1 @@ +Fix numeric writes into array holes bypassing inherited setters and read-only properties on `Array.prototype` and `Object.prototype`. Existing own elements and arrays with unmodified prototype chains keep their fast path. diff --git a/crates/perry-runtime/src/array/indexing.rs b/crates/perry-runtime/src/array/indexing.rs index 6d128ebb52..530ac70e32 100644 --- a/crates/perry-runtime/src/array/indexing.rs +++ b/crates/perry-runtime/src/array/indexing.rs @@ -1027,14 +1027,15 @@ pub(crate) unsafe fn try_strict_dense_number_store( // proves there are no holes and keeps its bit-for-bit old hot path; every // other admitted layout proves ownership with the slot Perry is about to // overwrite. - // The process latch leads: an array can only have an inherited index when - // SOME array has been retargeted, so a program that never calls - // `Object.setPrototypeOf` on an array keeps this lane bit-for-bit as it was - // (one relaxed load of a static bool, and the slot is never read here). - // `new Array(n)` fills are holey and would otherwise all fall off the lane. + // #9787: the shared invalidation byte covers indexed properties on both + // default prototypes as well as retargeted arrays. Checking only whether + // an array was retargeted misses Array.prototype / Object.prototype + // descriptors and lets this lane create an own element over a setter. + // Ordinary `new Array(n)` fills still pay one relaxed load and never read + // the old slot while all three prototype conditions remain clear. let may_have_holes = flags & crate::gc::GC_ARRAY_RAW_F64_LAYOUT == 0 || flags & crate::gc::GC_ARRAY_RAW_F64_HOLES != 0; - if crate::object::prototype_chain::array_static_proto_recorded() + if PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED.load(Ordering::Relaxed) != 0 && may_have_holes && ptr::read(slot) == crate::value::TAG_HOLE { diff --git a/crates/perry-runtime/src/array/strict_store_tests.rs b/crates/perry-runtime/src/array/strict_store_tests.rs index 96f753086d..97ef10dbf2 100644 --- a/crates/perry-runtime/src/array/strict_store_tests.rs +++ b/crates/perry-runtime/src/array/strict_store_tests.rs @@ -130,10 +130,10 @@ fn strict_dense_number_store_fast_lane_matches_the_general_path() { // #9220: an in-bounds hole is not an own property. The number lane // must decline it so the strict entry can consult an inherited index - // setter / non-writable data descriptor before creating an element — - // but ONLY once some array has been retargeted. With the process latch - // clear (the overwhelmingly common case, including every `new Array(n)` - // fill) the lane keeps filling holes exactly as it did before #9220. + // setter / non-writable data descriptor before creating an element. + // #9787: a default-prototype descriptor invalidates this lane even + // without a retargeted array. With the summary byte clear, ordinary + // `new Array(n)` fills retain their fast path. // // Indices 4 and 5 are the SAME shape — two in-bounds holes on one // array — so the latch is the only variable between the two arms. @@ -142,15 +142,27 @@ fn strict_dense_number_store_fast_lane_matches_the_general_path() { assert!(!array_has_own_index(out, 5)); let latch_was = crate::object::prototype_chain::test_swap_array_static_proto_recorded(false); + let summary_was = super::test_swap_array_index_fast_path_invalidated(0); + struct RestorePrototypeFlags(bool, u8); + impl Drop for RestorePrototypeFlags { + fn drop(&mut self) { + crate::object::prototype_chain::test_swap_array_static_proto_recorded(self.0); + super::test_swap_array_index_fast_path_invalidated(self.1); + } + } + let _restore = RestorePrototypeFlags(latch_was, summary_was); assert!( lane(out, 4, 8.0), - "no recorded array prototype: the hole fill stays on the fast lane" + "unmodified prototype chains: the hole fill stays on the fast lane" ); assert!(array_has_own_index(out, 4)); - crate::object::prototype_chain::test_swap_array_static_proto_recorded(true); + super::test_swap_array_index_fast_path_invalidated(1); assert!(!lane(out, 5, 8.0), "hole slot requires the [[Set]] walk"); assert!(!array_has_own_index(out, 5)); - crate::object::prototype_chain::test_swap_array_static_proto_recorded(latch_was); + assert!( + lane(out, 4, 9.0), + "an existing own element still bypasses inherited descriptors" + ); } } diff --git a/crates/perry/tests/issue_9249_array_prototype_define_property.rs b/crates/perry/tests/issue_9249_array_prototype_define_property.rs index 9aa254290d..5a977e95e6 100644 --- a/crates/perry/tests/issue_9249_array_prototype_define_property.rs +++ b/crates/perry/tests/issue_9249_array_prototype_define_property.rs @@ -68,6 +68,30 @@ console.log(hits, nums.length, nums[7]); ); } +#[test] +fn default_array_prototype_setter_intercepts_in_bounds_holes() { + compile_and_run( + include_str!("../../../test-files/test_gap_9787_array_hole_inherited_setter.ts"), + "true:31 false array-proto-three 1\n\ + true:31,true:37 false array-proto-three 5\n\ + own 2 true 41 4\n\ + deleted true:31,true:37,true:43 false array-proto-three 4\n\ + removed true 47 5\n", + "array_prototype_holes", + ); +} + +#[test] +fn default_object_prototype_descriptors_intercept_in_bounds_holes() { + compile_and_run( + include_str!("../../../test-files/test_gap_9787_object_prototype_hole_setter.ts"), + "1 true 53 false object-proto-eight 10\n\ + TypeError false getter-only 10\n\ + TypeError false locked 10\n", + "object_prototype_holes", + ); +} + #[test] fn define_properties_array_prototype_index_setter_intercepts_boolean_store() { compile_and_run( diff --git a/test-files/test_gap_9787_array_hole_inherited_setter.ts b/test-files/test_gap_9787_array_hole_inherited_setter.ts new file mode 100644 index 0000000000..33f25dd423 --- /dev/null +++ b/test-files/test_gap_9787_array_hole_inherited_setter.ts @@ -0,0 +1,36 @@ +// An in-bounds hole has no own property: it must consult inherited setters. +"use strict"; + +const calls: string[] = []; +Object.defineProperty(Array.prototype, "3", { + configurable: true, + get() { return "array-proto-three"; }, + set(this: any, value: any) { calls.push(`${Array.isArray(this)}:${value}`); }, +}); + +try { + const outOfBounds: any[] = [0]; + outOfBounds[3] = 31; + console.log(calls.join(","), Object.hasOwn(outOfBounds, 3), outOfBounds[3], outOfBounds.length); + + const inBoundsHole: any[] = new Array(5); + inBoundsHole[3] = 37; + console.log(calls.join(","), Object.hasOwn(inBoundsHole, 3), inBoundsHole[3], inBoundsHole.length); + + // An own undefined value is not a hole and must bypass the inherited setter. + const own: any[] = [0, 1, 2, undefined]; + own[3] = 41; + console.log("own", calls.length, Object.hasOwn(own, 3), own[3], own.length); + + // Deletion creates the same obligation as new Array(n)'s initial holes. + delete own[3]; + own[3] = 43; + console.log("deleted", calls.join(","), Object.hasOwn(own, 3), own[3], own.length); +} finally { + delete (Array.prototype as any)[3]; +} + +// The invalidation latch stays set after deletion, but no setter remains. +const plain: any[] = new Array(5); +plain[3] = 47; +console.log("removed", Object.hasOwn(plain, 3), plain[3], plain.length); diff --git a/test-files/test_gap_9787_object_prototype_hole_setter.ts b/test-files/test_gap_9787_object_prototype_hole_setter.ts new file mode 100644 index 0000000000..84922be313 --- /dev/null +++ b/test-files/test_gap_9787_object_prototype_hole_setter.ts @@ -0,0 +1,54 @@ +// Object.prototype alone must invalidate the numeric hole-store fast path. +"use strict"; + +let calls = 0; +let receiverIsArray = false; +let assigned = 0; +Object.defineProperty(Object.prototype, "8", { + configurable: true, + get() { return "object-proto-eight"; }, + set(this: any, value: number) { + calls++; + receiverIsArray = Array.isArray(this); + assigned = value; + }, +}); + +let setterResult = ""; +try { + const holes: any[] = new Array(10); + holes[8] = 53; + setterResult = [calls, receiverIsArray, assigned, Object.hasOwn(holes, 8), holes[8], holes.length].join(" "); +} finally { + delete (Object.prototype as any)[8]; +} +console.log(setterResult); + +Object.defineProperty(Object.prototype, "8", { + configurable: true, + get() { return "getter-only"; }, +}); +let getterResult = ""; +try { + const holes: any[] = new Array(10); + let error = "none"; + try { holes[8] = 59; } catch (e) { error = (e as Error).constructor.name; } + getterResult = [error, Object.hasOwn(holes, 8), holes[8], holes.length].join(" "); +} finally { + delete (Object.prototype as any)[8]; +} +console.log(getterResult); + +Object.defineProperty(Object.prototype, "8", { + configurable: true, value: "locked", writable: false, +}); +let lockedResult = ""; +try { + const holes: any[] = new Array(10); + let error = "none"; + try { holes[8] = 61; } catch (e) { error = (e as Error).constructor.name; } + lockedResult = [error, Object.hasOwn(holes, 8), holes[8], holes.length].join(" "); +} finally { + delete (Object.prototype as any)[8]; +} +console.log(lockedResult);