Keys-array elements copied at a hand-computed header + 8 in four places — SIGSEGV during collection on a grow-forward or reserved-floor array

Found while building dictionary mode (PR #10938); it cost three build cycles there and is latent for everyone else today.

The bug

Four sites read or copy an ordered keys array's elements at a hand-computed offset:

site	what it does
`crates/perry-runtime/src/object/object_ops/keys_array.rs:301-302`	clone-before-mutate for `defineProperty`
`crates/perry-runtime/src/object/field_set_by_name/tail.rs:801-802`	clone-before-push, `[[Set]]` growth (site 1)
`crates/perry-runtime/src/object/field_set_by_name/tail.rs:1014-1015`	clone-before-push, `[[Set]]` growth (site 2)
`crates/perry-runtime/src/object/field_get_set/ic_miss.rs:1124`	key scan on the miss path (read only)

let src_data = (keys as *const u8).add(8) as *const f64;
let dst_data = (cloned as *mut u8).add(8) as *mut f64;

header + 8 is not where a keys array's elements necessarily start. The canonical accessor is object::keys_array_dense_slots (object/keys_lookup.rs:26), and it does two things the hand-computed offset does not:

crate::array::clean_arr_ptr(keys) — resolves a grow-forward pointer;
crate::array::array_elements_ptr(arr) — adds array_front_offset, which is array_physical_capacity - capacity (array/storage.rs:31). That is nonzero for any array with a front reserve — GC_ARRAY_NAMED_PROPS arrays, and for…of over a Set iterator with a patched .next() segfaults (pre-existing, not #9017) #9019's reserved-floor keys arrays, which are born with leading holes.

Why it is worse than an off-by-N read

The copy destination comes from js_array_alloc_pointer_elements, which declares GC_LAYOUT_ALL_POINTERS on the clone (gc/layout.rs:575). So the clone tells the collector every live slot is a pointer. Copying from the wrong base puts ArrayHeader words and front-reserve slack into it, and the collector then traces those as pointers.

Two symptoms, in this order:

before the next collection: a missing property — the key bytes don't match, so the lookup misses;
during it: SIGSEGV, with a backtrace that names something entirely unrelated. Mine landed in a URLSearchParams shape probe:

#0  core::str::lossy::{impl#4}::next
#1  alloc::string::String::from_utf8_lossy
#2  perry_runtime::url::get_string_content
#3  perry_runtime::url::search_params::shape_is_url_search_params
#4  perry_runtime::object::field_get_set::get_field_by_name_tail::get_field_by_name_object_tail

shape_is_url_search_params (url/search_params.rs:1026) does js_array_get_f64(keys_arr, 0) then get_string_content(key0). It validates the array's GC header carefully — the comment at :1008 records a previous SIGSEGV there — but it cannot validate an element that is a header word.

Why it has not fired yet

The existing four sites happen to be reached only with arrays whose front offset is 0 and that are not grow-forward. That is a property of today's callers, not of the type: nothing at any of the four sites checks it, and array_front_offset is derived from the GC header's size, so a size-class round-up alone can make it nonzero.

Fix

Use object::keys_array_dense_slots for the source and array::array_elements_ptr for the destination, as object/dictionary.rs::copy_key_list_into now does (PR #10938). The dense-slot accessor already exists precisely for this — its doc comment is about cost, but correctness is the stronger reason.

Owner note: object_ops/keys_array.rs and the tail.rs sites sit in step 2.5's area (#10868).

Closing #10940 as a duplicate of this — four sites is the right scope, and I
had only keys_array.rs:301. Moving the parts of that analysis that aren't
already here.

A correction to my own filing, which this issue's finding supersedes. I
described it as latent, reachable only on a dense-shifted or reserved-floor
(#9019) source array. That is too weak: if array_front_offset is derived
from the GC header's size, a size-class round-up alone makes it nonzero,
so the wrong offset is the ordinary case rather than an exotic one. That also
explains a backtrace naming a URLSearchParams probe with nothing near the
bug — the corruption and the crash have no call-path relationship.

Why this is worse than a bad read, at keys_array.rs:301 specifically.
The destination is js_array_alloc_pointer_elements, and the copy is
followed by:

(*cloned).length = key_count as u32;

Before that store the clone is unpublished and its garbage is untraced; after
it, exactly the first key_count slots become a region the collector walks as
heap pointers. So the bug converts a mis-offset read into a promise to the
GC that arbitrary header//capacity words are pointers. The failure is a
SIGSEGV inside collection, arbitrarily later, with a stack that has nothing to
do with the writer — which is the shape that cost three build cycles to trace.

Fix shape for that site: resolve the source through
keys_lookup::keys_array_dense_slots_resolved(keys), which exists precisely
to resolve the front offset and returns (slots, slot_len), and clamp the
copy to min(key_count, slot_len) rather than trusting key_count. The
destination should go through the accessor too instead of a second
hand-computed + 8, so no site in the function carries the constant.

One note for whoever fixes all four: object_ops/keys_array.rs is being
rewritten by #10868 step 2.5 (canonical keys arrays). The fix should land on
its own ahead of that — it is memory safety and should not wait — and I will
rebase step 2.5 onto it rather than the reverse.

lint: satisfy the addr-class ratchet for #10925 and #10939

fix(runtime): a keys array's elements are not at `header + 8` (fixes #…

lint: satisfy the addr-class ratchet for #10925 and #10939

fix(runtime): a keys array's elements are not at `header + 8` (fixes #…

lint: satisfy the addr-class ratchet for #10925 and #10939

Review finding on the landed fix — the fourth site has no witness, and the coverage that looks like one cannot fail.

Routing this to the issue of record rather than PR #10948: that PR closed unmerged, and the fix reached main as c7c7eed836. There is no open PR to comment on, and this is lane 16b's file, so this is report-only — I have pushed nothing.

CodeRabbit flagged keys_front_offset_tests.rs:90 as missing IC-miss coverage. Verified against upstream/main, and it is valid, but understated in a way worth naming.

The commit message over-reaches. c7c7eed836 states: "Reverting the four sites reddens it by name." That holds only because two of the four are copy sites reachable from js_object_set_field_by_name. The regression test's entire call surface is js_object_alloc, js_object_set_field_by_name, object_keys_array, and the array builders — it never performs a field read. get_field_ic_miss_impl's only callers are inside ic_miss.rs and ic_slow.rs. Reverting the ic_miss.rs hunk alone leaves the suite green.

The worse half: the ic_miss keys scan is already covered, and the coverage is blind. ladder_skip_tests::an_object_miss_does_not_consult_the_typed_array_registry calls get_field_ic_miss_impl and, by its own comment, "resolves in the keys scan and returns before the handler's by-name tail" — the exact loop the fix changed. But it builds its receiver with js_object_alloc(0, 4) plus set_field_by_name, so the keys array is fresh, array_front_offset is 0, and header + 8 and the resolved base are the same address. It passes identically on both arms.

That is the shape this very test file already guards against everywhere else. Line 70 asserts the premise explicitly — "or header + 8 and the element accessor name the same address and this test proves nothing." The ic_miss path is the one place that assertion was never made, and it is false there.

Absent coverage announces itself. Coverage that runs the right line in the one configuration where the bug is invisible reads as green and is worse.

Recommended fix, unchanged in substance from the review: after the js_array_shift_f64, read a surviving key back through js_object_get_field_ic_miss (ic_miss/packed_get.rs:8) and assert the value. Two things beyond what the review asked:

Carry the same premise assertion as line 70, so the new read cannot silently degrade into the zero-offset case.
Assert the symptom, not just inequality. The fix comment says an own property "misses here and is answered from the prototype chain instead," so the failure is a surviving own key returning undefined (or a prototype value), not a wrong number. Pin that.

Method, stated plainly: this is a source-level determination — call-graph and test-construction reads on upstream/main — not a measured revert. I did not build. The tree-wide check behind "no other test covers this" is that of every file mentioning js_array_shift / shift_dense / GC_ARRAY_NAMED_PROPS / array_front_offset, only ic_miss.rs also names an ic_miss entry point, and its sole mention is the fix's own comment at line 1132. Anyone taking this should redden it by reverting that hunk before fixing it.

fix(runtime): a keys array's elements are not at `header + 8` (fixes P…

Navigation Menu

Repository navigation

Keys-array elements copied at a hand-computed `header + 8` in four places — SIGSEGV during collection on a grow-forward or reserved-floor array #10939

The bug

Why it is worse than an off-by-N read

Why it has not fired yet

Fix

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Keys-array elements copied at a hand-computed header + 8 in four places — SIGSEGV during collection on a grow-forward or reserved-floor array #10939

Description

The bug

Why it is worse than an off-by-N read

Why it has not fired yet

Fix

Activity

proggeramlug commented on Sep 22, 2026

proggeramlug commented on Sep 22, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Keys-array elements copied at a hand-computed `header + 8` in four places — SIGSEGV during collection on a grow-forward or reserved-floor array #10939