fix(runtime): arr[Symbol.iterator] read an array's capacity as a class_id (#7563) - #7569
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 (2)
📝 WalkthroughWalkthroughThe runtime now uses guarded class-ID access when resolving method receivers. Tests verify that arrays are rejected as class instances while genuine class instances resolve correctly. Integration coverage exercises array iterators and related subclass behavior. ChangesArray iterator class-ID fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…s_id (#7563) ObjectHeader is { object_type: u32, class_id: u32, ... } and ArrayHeader is { length: u32, capacity: u32 }, so the two u32s at offset 4 alias: an array pointer read as an ObjectHeader reports its capacity as a class_id. arr[Symbol.iterator] resolves through js_class_method_bind(arr, "values"), and that builder's receiver->class step, class_id_from_method_receiver, read the field with a bare (*obj).class_id -- guarded against closures and the handle band, but never against the allocation's actual type. So whenever the class whose id equalled the array's capacity owned a method named `values`, the array's iterator resolved to THAT class's method. When it was the calling class, `values` re-entered `values` until the stack guard page: EXC_BAD_ACCESS at `str xzr, [sp], #-0x50`, ~26 000 frames deep. Reported as a `class X extends Map` values() override bug, but Map is incidental and so is the iteration -- the crash reproduces with no Map in the program and no for-of on the path: class Plain { values() { return [777][Symbol.iterator](); } } new Plain().values(); // SIGSEGV Use js_object_get_class_id, the guarded accessor that already existed for this read: it rejects the handle band, the std::alloc'd Map/Set/Regex headers (no GcHeader to probe), and any allocation whose GcHeader.obj_type is not GC_TYPE_OBJECT. The sibling symbol-method arm in native_call_method.rs already routed through it and was never affected -- verified, not assumed. Not #7561: rewrite_collection_view_for_of declines a subclass receiver exactly as documented, and the offending line predates it by hundreds of commits (traces through #5631's file split to #4630), matching the report that it reproduces at 969b447. Coverage: test-files/test_gap_7563_array_iterator_class_id_confusion.ts (byte-compared against node; SIGSEGVs at the parent commit) and object::tests::array_receiver_is_never_read_as_a_class_id (fails with Some(16), the array's capacity, before the fix).
3bfc255 to
3523355
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
While probing the native-base-subclass family for this fix I found a second, independent memory-safety bug in the same area and filed it as #7570 — it is not fixed here, and it survives this PR. class MyMap<K, V> extends Map<K, V> {}
const m: Map<string, number> = new MyMap<string, number>();
m.set("a", 1); // SIGBUS, before anything printsDifferent root cause: "is a Map" is decided from the declared type, so an annotated Dropping the annotation ( Kept out of this PR deliberately: it is a codegen/type-analysis fix in a different layer, and widening a one-line memory-safety fix to reach it would make both harder to review and to revert. |
1 participant
Fixes #7563.
What the crash actually is
Reported as "a
class X extends Mapthat overridesvalues()SIGSEGVs when the override is iterated".Mapturns out to be incidental, and so does the iteration. The same crash reproduces with noMapanywhere in the program and with nofor-ofon the path:It is a stack overflow from infinite recursion, not a stale or null pointer.
EXC_BAD_ACCESS (code=2, address=0x16f603fe0)atstr xzr, [sp], #-0x50— the guard page — with a ~26 000-frame cycle:Root cause
ObjectHeaderis{ object_type: u32, class_id: u32, … };ArrayHeaderis{ length: u32, capacity: u32 }. The twou32s at offset 4 alias, so an array pointer read as anObjectHeaderreports its capacity as aclass_id.arr[Symbol.iterator]resolves throughjs_class_method_bind(arr, "values")(symbol/get.rs, the #321 arm that makestypeof arr[Symbol.iterator] === "function"hold). That builder's receiver→class step —class_id_from_method_receiverincrates/perry-runtime/src/object/native_module.rs— read the field with a bare(*obj).class_id, guarded against closures and against the handle band but never against the allocation's actual type.So whenever the class whose id equalled the array's capacity happened to own a method named
values,method_owner_class_idfound it and the canonical class method was returned as the array's iterator. Class ids are handed out from 1 in declaration order and the default capacity isMIN_ARRAY_CAPACITY-clamped, so the collision is the common case for small programs — and when the colliding class was the calling class,valuesre-enteredvaluesforever.The mis-dispatch is directly observable in its non-fatal form, and it is capacity-indexed exactly as predicted:
Fix
One line: use
js_object_get_class_id, the guarded accessor that already existed for exactly this read. It rejects the handle band, thestd::alloc'dMap/Set/Regexheaders (which have noGcHeaderto probe), and any allocation whoseGcHeader.obj_typeis notGC_TYPE_OBJECT. The bare read bypassed all three.The sibling symbol-method arm in
object/native_call_method.rsalready routed through that accessor — verified with a targeted probe, not assumed.The guard is deliberately not narrower than the invariant it protects: a genuine class instance still resolves to its id, asserted in the same test.
Not #7561
rewrite_collection_view_for_ofdeclines a subclass receiver exactly as its doc comment claims. The crash needs neither afor-ofnor aMap— callingm.values()and discarding the result is enough — and the offending line predates #7561 by hundreds of commits (it traces back through #5631's file split to #4630). That matches the issue's report that it reproduces at969b447cc.Verification (local; both directions)
test_gap_7563_…tsobject::tests::array_receiver_is_never_read_as_a_class_idSome(16)— the capacity)Native-base-subclass family sweep, all byte-identical to node:
values/keys/entries/[Symbol.iterator]overrides on Map, Set and Array subclasses; an indirect subclass (class Leaf extends Mid extends Map); a class-expression subclass; non-overriding subclasses keeping the built-in surface; andsuper.values()from inside an override still reaching the native base.Gates:
cargo test -p perry-runtime --no-fail-fast1812 passed / 0 failed;raw_handle_debt.py998 (baseline 998);check_file_size.shOK;cargo fmt --all -- --checkclean.No rooting change, so the GC instruments are not implicated.
Out of scope
The issue's "Related shapes" table (
Map.prototype.values = …,Map.prototype[Symbol.iterator] = …, own-instancem.values = …) still diverges — Perry uses the original, node honours the patch. Confirmed unchanged by this PR; that is the pre-existing statically-typed-collection fast-path family (#7542), and the issue itself files it as context rather than as the actionable part.#7564 (
make_iter_result's five allocations per.next()) is untouched — this fix does not go near that code, and widening a memory-safety fix into a performance refactor was not warranted.Summary by CodeRabbit
Bug Fixes
Tests