fix(runtime): an Array subclass in a base-typed binding was read as a raw header (#7574) - #7603
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughArray subclass instances stored in array-typed bindings now avoid forged ChangesArray subclass safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TypedBinding
participant ArrayRuntime
participant ArraySubclassObject
participant GenericArrayLikeEngine
TypedBinding->>ArrayRuntime: invoke array operation
ArrayRuntime->>ArrayRuntime: validate GC type
ArrayRuntime->>ArraySubclassObject: resolve subclass receiver
ArrayRuntime->>GenericArrayLikeEngine: dispatch array-like operation
GenericArrayLikeEngine->>ArraySubclassObject: access indexed properties
ArraySubclassObject-->>TypedBinding: return result
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
585d2eb to
3bbb402
Compare
3bbb402 to
0fc7d7e
Compare
0fc7d7e to
008e65d
Compare
Audit before merge — verified, merged as v0.5.1343Gap test byte-identical to node (39 lines, exit 0) on my own build, rebased Sabotage decisive in both directions: neutering just the new inline-push Fast-path preservation: Three things from the report worth keeping visible:
|
1 participant
Fixes #7574.
The bug
Reproduced on
main: exit 139, one line of (already wrong) output. Thefault address is
0x3ff0000000000000— the IEEE bit pattern of the double1.0, i.e. the value the first push stored, later dereferenced as a pointer.Sibling of #7570 (fixed by #7573) on a different family, and the same premise:
a declared TypeScript type is a hint, never a layout fact (CLAUDE.md,
Known Limitations). All five binding forms are affected —
const, parameter,class field, return type, and
as number[]cast.Verified root cause
An Array-subclass instance is a plain
ObjectHeader(
array/subclass.rs:1-7;js_array_subclass_initinstalls alengthownproperty and keeps the elements as ordinary indexed object properties — there
is no
ArrayHeaderbacking).ObjectHeaderandArrayHeaderoverlay fieldfor field:
ArrayHeaderObjectHeaderlength: u32object_type(= 1)capacity: u32class_idelements[0]parent_class_id‖field_countelements[1]keys_array(*mut ArrayHeader)elements[2]meta(*mut ObjectMeta)1 <= class_idsails throughclean_arr_ptr'slength <= capacity <= 100Msanity check, so the forged header is accepted.
a.push(1)stores1.0athandle + 8 + length*8=handle + 16— overkeys_array, a live GC childedge — and writes
length + 1overobject_type. The second push then walksthat forged pointer and faults. Element reads are the same hazard in the other
direction: they hand
keys_array/metaout to user code as doubles.The tiers, checked on current
mainThe issue named three; the sweep found two more, and one of its three turned
out to be a false positive:
mainexpr/array_push.rs:301— the inlineExpr::ArrayPushstore (fwd-flag test only, then raw slot store +lengthbump)expr/index_get.rs:531lower_bounded_array_index_get(testsGC_TYPE_LAZY_ARRAY,GC_FLAG_FORWARDED,OBJ_FLAG_ARRAY_DESCRIPTORS, then rawgep+load double)array/header.rs:511clean_arr_ptr— the funnel behind ~190js_array_*entriesobj_typetest, only length/capacity sanitytyped_feedback.rs:2413js_typed_feedback_array_index_set_fallback_boxedlengthstep. Not named in the issue.expr/property_get.rs:236— inlinearr.lengthobj_type == 1 || == 3, thenjs_value_length_f64, whoseGC_TYPE_OBJECTarm reads the ownlength). The issue's claim that it is "a baresafe_load_i32_from_ptr" does not hold on currentmain.expr/index.rs:79lower_index_set_fastinline tier, and both typed-feedback guardsobj_type == GC_TYPE_ARRAYexpr/ptr_numarray_access.rsguard-freePtr<NumArray>get/setnew Array(<static n>)or[], whichnew MyArr()is notexpr/index_get/guarded_array.rs:96Fix
Runtime funnel first, for memory safety.
clean_arr_ptrnow refuses aGC_TYPE_OBJECT/GC_TYPE_CLOSUREallocation, so all ~190 call sites becomefail-closed at once — each degrades through its existing null branch instead
of dereferencing a forged header. It reuses the
obj_typebyte the surroundingblock already loads for the forwarding/lazy checks, so a genuine array pays one
extra compare; the registry probes that rule out header-less buffers/typed
arrays are in the cold arm only. Same "resolve at the shared runtime funnel, not
at one codegen predicate at a time" shape as #7573.
Then correctness, at the entry points the declared-type tiers actually reach.
Unlike Map/Set there is nothing to redirect to — an Array subclass has no
hidden backing. But perry already has a complete spec-generic array-like engine
(
array/generic.rs,generic_object.rs) that operates on exactly thisrepresentation and is the path the unannotated form has always used through
js_native_call_method. So the entry points re-enter through their existingnull branch and run the operation there.
Per-entry-point inventory (everything reachable from the three tiers):
js_array_push_f64run_object_mutator(recv, "push"), returns the ORIGINAL receiver so codegen's realloc write-back keeps the binding (returning a fresh empty array is what made the push look silently dropped)js_array_set_f64_extend(…_strict)[[Set]]+ Array-exoticlengthjs_array_get_f64,js_array_get_f64_uncheckedal_getjs_array_set_length_strictSet(O,"length",n,true)(deletes truncated indices)js_array_pop_f64,js_array_shift_f64plain_object_value→ generic engine)js_array_lengthGC_TYPE_OBJECTarm reads the ownlength)js_array_map/filter/forEach/some/every/find*/reduce*/join/slice/indexOf/lastIndexOf/includes/at(21 sites)normalize_array_receiver, which materializes an array-like object into a dense snapshotjs_array_forEachjs_array_concat_variadicobj_typere-dispatch +append_concat_arg's subclass snapshot arm); its #6386 dense bulk path needed a guard — see belowjs_array_clone_for_spread,js_get_iterator,for…ofarray_from_spread_value/symbol/iterator.rssubclass arms)js_array_numeric_*,js_array_is_numeric_f64_layout,js_template_rawPtr<NumArray>tiersnew MyArr())Three sites needed more than the funnel, all analogues of #7573's extra four:
js_array_forEach's 3rd argument.normalize_array_receiverhands theloop a dense snapshot, so the callback saw the snapshot and
self === subwas
false. It now passes the original receiver through, gated on a one-loadGC_TYPE_OBJECTheader test (addr_class::try_read_gc_header) so a genuinearray never enters the registry probes, and rooted across the callbacks.
lengthis written, not just read.sub[3] = von a real Array runs theexotic
[[DefineOwnProperty]]and leaveslength == 4; a plain object's doesnot. Pre-fix
sub[0] = 10; sub.lengthread back0— on the unannotatedpath too — which then made the next
sub.push(v)append at index 0 andoverwrite the element. The step is applied after the store at the three
generic funnels a subclass index-write can reach (
js_put_value_set,js_object_set_index_polymorphic,js_typed_feedback_array_index_set_fallback_boxed), gated on the class chainreaching
Arrayso an object literal short-circuits onclass_id == 0.sub.length = nlikewise routes to the exotic setter, which deletes thetruncated indices.
clean_arr_ptrreturning null now MEANS something new at sites that readnull as "an empty array".
concat's [perf] DataView accessors, Array.concat, and regex match-with-groups are 4–30x slower than Hermes #6386 all-dense bulk path did exactlythat:
peek_plain_array_len/dense_concat_array_sourceansweredSome(0), the bulk path claimed the copy, and the spec-shapedappend_concat_argflow (which has the subclass snapshot arm) never ran —[1,2].concat(sub)yielded1,2. This was a regression my own funnelintroduced, caught by the family A/B below, not by the new test. Both now
classify the receiver before the null shortcut and answer
None("un-peekable"), restoring the pre-fix routing. I swept the other
null-means-empty sites (
js_template_raw,js_array_{mark,is}_numeric_f64_layout,js_array_numeric_set_f64_unboxed);for those, null is the correct answer.
Two codegen guards are unavoidable. Unlike #7573, two of the three tiers
emit no runtime call at all — they are inline LLVM IR — so no runtime funnel
can reach them. Both now test
obj_type == GC_TYPE_ARRAYand route a miss tothe slow call they already had (which then resolves through the funnel above).
Both are strictly more restrictive than what they replaced, so no receiver
that used to take the slow path now takes the fast one:
lower_bounded_array_index_get:icmp eq gc_type, 9(GC_TYPE_LAZY_ARRAY)→
icmp ne gc_type, 1. Lazy arrays are 9, so the new test subsumes the oldone — and it is one instruction cheaper (an
icmpreplacesicmp+or).Expr::ArrayPush: the existinggc_flags & FORWARDEDpredicate gains|| obj_type != GC_TYPE_ARRAY; a miss takes theapush.fwdarm, which isalready a
js_array_push_f64call.Why not give an Array subclass a hidden backing the way #7573 did for
Map/Set: its elements are its own indexed properties, and
Object.keys/for…in/JSON.stringify/ the whole generic engine read them there. A secondstorage would have to be kept in sync with every one of those paths — a far
larger and riskier change than making the raw entries resolve.
Validation (local; CI has a deep backlog, so local is what this rests on)
test-files/test_gap_7574_array_subclass_declared_base_type.ts—byte-identical to
node --experimental-strip-types(v26.5.1), exit 0, 39lines. Covers all five binding forms × index get/set,
.lengthread ANDwrite (including truncation), push/pop/shift, the bounded-index loop tier,
for…of/spread/Array.from/destructuring,map/filter/slice/join/indexOf/includes/reduce,forEachreceiver identity, an indirectsubclass, a subclass with its own constructor and fields, and non-subclass
controls (a real array in the same forms, and a plain object merely
annotated
number[]).crates/perry-runtime/andcrates/perry-codegen/reverted in full tomainand the test file untouched, the same file exits 139 after oneline of output (
push1 0; node printspush1 1). With both restored it exits0, byte-identical. Recorded separately: the runtime half alone does not
stop the crash — the inline push tier never calls into the runtime — which is
why the two codegen guards are in this PR.
LLVM IR is byte-identical apart from the guard predicate: after
normalizing SSA numbering,
probe/plain.tsdiffers by 15 lines (3 pushsites × the 4 new instructions + the
or), andprobe/plain2.ts— whichreaches the bounded-index tier — by exactly 3 lines, one
icmp eq …, 9→icmp ne …, 1per site, with identical instruction counts (3970/3970). Theapush.inboundsandbidx.fastblocks and their raw slot loads areunchanged, and both programs produce identical output on both arms. The unit
test
a_genuine_array_takes_the_fast_path_and_is_never_redirectedassertsclean_arr_ptrreturns a realArrayHeaderunchanged and that theredirect answers
Nonefor it, so the identity cannot have come from aredirect that happened to agree.
array/subclass_tests.rs: each firstasserts the bytes the pre-fix code misread are still sitting there
(
ArrayHeader.length == 1aliasingobject_type,capacity == class_id,both passing the old sanity check) and only then that the entry point refuses
or resolves — so a green run proves the brand check fired.
test-files/test_*{array,spread,iter,foreach,for_of, slice,splice,sort,concat,flat,push,pop,destructur}*.ts, run on both arms:the failure set is identical (2 pre-existing
COMPILE_FAIL, 8NODE_FAILwhere node itself cannot run the file, 1 pre-existingRUN_FAIL), except thattest_gap_7574_…fails 139 on the reverted armand passes on this one.
test_gap_6232_class_extends_arrayalso passes onboth — its
concatline is what caught the bulk-path regression above, whichis fixed here.
cargo test -p perry-runtime: 1850 passed, 0 failed.cargo test -p perry-codegen --lib: 671 passed, 0 failed.python3 scripts/raw_handle_debt.py: 998 (baseline 998), per-moduleceilings held.
python3 scripts/addr_class_inventory.py,python3 scripts/class_id_collisions.py,./scripts/check_file_size.sh,cargo fmt --all -- --check: all clean.Known gap left in place
ArraySpeciesCreateon a subclass: node'ssub.map(f)returns aMyArr,perry returns a plain
Array. That is pre-existing and identical on theunannotated path, so it is out of scope here; the gap test compares element
CONTENT (via
join) rather than the container's console formatting, and saysso inline. Worth its own issue.
No version bump (maintainer bumps at merge).
Summary by CodeRabbit
Bug Fixes
Arraysubclasses are used through array-typed bindings.push,concat, iteration, spread, stack operations, andlengthupdates.Tests
Documentation