fix(runtime): arr[Symbol.iterator] read an array's capacity as a class_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).

Author: Ralph Küpper

changelog.d/7569-array-iterator-class-id-confusion.md

@@ -0,0 +1,86 @@
+### Bug fixes
+
+**`arr[Symbol.iterator]` read an array's `capacity` as a `class_id`, so an
+array literal could resolve its iterator to an unrelated user class's `values`
+method — self-recursively, and fatally (#7563).**
+
+Reported as "a `class X extends Map` that overrides `values()` SIGSEGVs when
+the override is iterated". `Map` turns out to be incidental, and so does the
+iteration: the same crash reproduces with **no `Map` anywhere in the program**
+and with **no `for-of` on the path**.
+
+```ts
+class Plain {
+  values(): IterableIterator<number> {
+    return [777][Symbol.iterator]();   // <-- SIGSEGV
+  }
+}
+new Plain().values();
+```
+
+#### Root cause
+
+`ObjectHeader` is `{ object_type: u32, class_id: u32, … }`; `ArrayHeader` is
+`{ length: u32, capacity: u32 }`. The two `u32`s at offset 4 alias, so 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")`
+(`symbol/get.rs`, the #321 arm that makes `typeof arr[Symbol.iterator] ===
+"function"` hold). That builder's receiver→class step,
+`class_id_from_method_receiver` in `object/native_module.rs`, read the field
+with a bare `(*obj).class_id` — guarded only against closures and against the
+handle band, 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_id` found it and the canonical class method was returned as
+the array's iterator.
+
+The default array capacity is `MIN_ARRAY_CAPACITY`-clamped, and class ids are
+handed out from 1 in declaration order, so the collision is not exotic — it is
+the common case for small programs. When the colliding class was the *calling*
+class, `values` re-entered `values` once per step until the stack guard page:
+
+```
+perry_method_repro_ts__MyMap__values
+  → js_native_call_method_value → js_object_get_symbol_property   [the array read]
+  → js_native_call_value → dispatch_bound_method → call_vtable_method
+  → perry_method_repro_ts__MyMap__values                          [× ~26 000]
+```
+
+`EXC_BAD_ACCESS (code=2)` at `str xzr, [sp], #-0x50` — a stack-overflow guard
+fault, not a stale or null pointer.
+
+#### Fix
+
+`class_id_from_method_receiver` now uses `js_object_get_class_id`, the guarded
+accessor that already existed for exactly this read: it rejects the handle band,
+the `std::alloc`'d `Map`/`Set`/`Regex` headers (which have no `GcHeader` to
+probe), and any allocation whose `GcHeader.obj_type` is not `GC_TYPE_OBJECT`.
+The bare read bypassed all three. The sibling symbol-method arm in
+`object/native_call_method.rs` already routed through that accessor and was
+never affected — verified, not assumed.
+
+One line of behaviour change; the guard is deliberately not narrower than the
+invariant it protects, so a genuine class instance still resolves to its id
+(asserted in the same test).
+
+#### Not #7561
+
+The #7561 `for (… of m.values())` rewrite is **not** implicated, and neither is
+any Map fast path. `rewrite_collection_view_for_of` declines a subclass receiver
+exactly as its doc comment claims, and the crash needs neither a `for-of` nor a
+`Map`: calling `m.values()` and discarding the result is enough, and a plain
+class reproduces it. The offending line predates #7561 by hundreds of commits
+(it traces back through #5631's file split to #4630), matching the issue's
+report that it reproduces at `969b447cc`.
+
+#### Also fixed by the same line
+
+The related shapes in the issue's table now match node as well — a `values` /
+`keys` / `entries` / `[Symbol.iterator]` override on a `Map`, `Set` or `Array`
+subclass, an indirect subclass, and a class-expression subclass, with
+`super.values()` from inside an override still reaching the native base.
+
+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).

crates/perry-runtime/src/object/native_module.rs

@@ -1301,7 +1301,12 @@ pub(crate) fn canonical_bound_method_receiver(captured: f64) -> f64 {
     captured
 }
 
-fn class_id_from_method_receiver(instance: f64) -> Option<u32> {
+/// The `class_id` of `instance`, when `instance` really is a class instance.
+///
+/// `pub(super)` so `object::tests` can assert the #7563 invariant directly: a
+/// non-object allocation (an array, above all) must resolve to `None` rather
+/// than to whatever its bytes happen to hold at the `class_id` offset.
+pub(super) fn class_id_from_method_receiver(instance: f64) -> Option<u32> {
     if let Some(cid) = class_ref_id(instance) {
         return Some(cid);
     }
@@ -1323,7 +1328,27 @@ fn class_id_from_method_receiver(instance: f64) -> Option<u32> {
             if crate::closure::is_closure_ptr(obj as usize) {
                 return None;
             }
-            let cid = unsafe { (*obj).class_id };
+            // #7563: the closure guard above fixed ONE instance of that type
+            // confusion; a bare `(*obj).class_id` read has it for every other
+            // non-object allocation too. `ObjectHeader` is `{ object_type: u32,
+            // class_id: u32, … }` while `ArrayHeader` is `{ length: u32,
+            // capacity: u32 }`, so the `class_id` slot of an ARRAY overlays its
+            // **capacity** — an N-capacity array literal was read back as
+            // "class id N". Reached from `arr[Symbol.iterator]`, which resolves via
+            // `js_class_method_bind(arr, "values")` (`symbol/get.rs`): whenever
+            // class id N happened to own a `values` method, the array's
+            // iterator resolved to THAT class's method. With `class Plain {
+            // values() { return [777][Symbol.iterator](); } }` the one-element
+            // literal read back as class id 1 — `Plain` itself — so `values`
+            // called `values` until the stack guard page: a SIGSEGV with no
+            // `Map` anywhere in the program.
+            //
+            // `js_object_get_class_id` is the guarded accessor for exactly this
+            // read: it rejects the handle band, the std::alloc'd Map/Set/Regex
+            // headers (which have no `GcHeader` to probe), and — the part that
+            // matters here — any allocation whose `GcHeader.obj_type` is not
+            // `GC_TYPE_OBJECT`. Reading the field directly bypassed all three.
+            let cid = crate::object::js_object_get_class_id(obj);
             if cid != 0 {
                 return Some(cid);
             }

crates/perry-runtime/src/object/tests.rs

@@ -1379,3 +1379,48 @@ fn stale_pre_grow_array_pointer_reads_the_real_length_in_object_ops() {
         "the freeze walk must not run past the array's real length"
     );
 }
+
+/// #7563: an ARRAY receiver must never be read back as a class instance.
+///
+/// `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 read as an `ObjectHeader` reports its **capacity** as a `class_id`.
+///
+/// That mattered because `arr[Symbol.iterator]` resolves through
+/// `js_class_method_bind(arr, "values")`, whose receiver→class step used a bare
+/// `(*obj).class_id` read instead of the guarded `js_object_get_class_id`. Any
+/// class whose id equalled the array's capacity and which owned a `values`
+/// method therefore captured the array's iterator. When that class was the
+/// *calling* class — `class C { values() { return [x][Symbol.iterator](); } }`
+/// — `values` re-entered `values` until the stack guard page, i.e. a SIGSEGV
+/// with no `Map` anywhere in the program.
+#[test]
+fn array_receiver_is_never_read_as_a_class_id() {
+    let arr = crate::array::js_array_alloc(3);
+    assert!(!arr.is_null());
+    // Impersonate exactly the class id this array's bytes would have yielded.
+    let impersonated = unsafe { (*arr).capacity };
+    assert_ne!(
+        impersonated, 0,
+        "the test is vacuous unless the capacity is a non-zero (i.e. lookup-able) class id"
+    );
+
+    let arr_value = crate::value::js_nanbox_pointer(arr as i64);
+    assert_eq!(
+        super::native_module::class_id_from_method_receiver(arr_value),
+        None,
+        "an array is not a class instance: its capacity must not be read as a class id"
+    );
+
+    // The guard must not over-narrow. A genuine class instance carrying the
+    // very same id still resolves, so the bound-method identity path (#446)
+    // keeps working.
+    let obj = js_object_alloc(impersonated, 0);
+    assert!(!obj.is_null());
+    let obj_value = crate::value::js_nanbox_pointer(obj as i64);
+    assert_eq!(
+        super::native_module::class_id_from_method_receiver(obj_value),
+        Some(impersonated),
+        "a real class instance must still resolve to its class id"
+    );
+}

test-files/test_gap_7563_array_iterator_class_id_confusion.ts

@@ -0,0 +1,180 @@
+// #7563: `arr[Symbol.iterator]` read an ARRAY's `capacity` field as a `class_id`.
+//
+// `ObjectHeader` is `{ object_type: u32, class_id: u32, … }` and `ArrayHeader`
+// is `{ length: u32, capacity: u32 }`, so the two u32s at offset 4 alias: an
+// N-capacity array read as an `ObjectHeader` reports "class id N".
+//
+// `arr[Symbol.iterator]` resolves through `js_class_method_bind(arr, "values")`
+// (`symbol/get.rs`), and that bound-method builder read `class_id` off the
+// receiver with a BARE `(*obj).class_id` instead of the guarded
+// `js_object_get_class_id` accessor (which rejects any allocation whose
+// `GcHeader.obj_type` is not `GC_TYPE_OBJECT`). So whenever the class whose id
+// equalled the array's capacity happened to own a `values` method, the array's
+// iterator resolved to THAT class's method.
+//
+// The issue was reported as a `class X extends Map` bug, but Map is incidental:
+// the only thing that matters is a class owning a method named `values`. When
+// that class is also the one whose `values` body builds the array literal, the
+// method calls itself until the stack guard page — a SIGSEGV.
+
+// ── the issue's exact reproducer ──
+class MyMap<K, V> extends Map<K, V> {
+  values(): IterableIterator<V> {
+    return [777 as unknown as V][Symbol.iterator]();
+  }
+}
+
+const m = new MyMap<string, number>();
+m.set("q", 5);
+console.log("size:", m.size);
+console.log("get:", m.get("q"));
+const out: number[] = [];
+for (const v of m.values()) out.push(v);
+console.log("values:", out.join(","));
+
+// ── the same crash with NO `Map` anywhere: a plain class whose method is named
+//    `values` and whose body iterates an array literal. Before the fix the
+//    one-element literal read back as class id 1 — the class itself — so
+//    `values` called `values` until the stack overflowed. ──
+class Plain {
+  values(): IterableIterator<number> {
+    return [777][Symbol.iterator]();
+  }
+}
+console.log("plain:", [...new Plain().values()].join(","));
+
+// ── the non-recursive form of the same mis-dispatch: the array's capacity
+//    selects a DIFFERENT class that owns `values`. Pre-fix the 2-element
+//    literal resolved to `B.values` (a number), and the spread threw
+//    "value is not iterable". ──
+class A {
+  one(): IterableIterator<number> {
+    return [1][Symbol.iterator]();
+  }
+  two(): IterableIterator<number> {
+    return [1, 2][Symbol.iterator]();
+  }
+  three(): IterableIterator<number> {
+    return [1, 2, 3][Symbol.iterator]();
+  }
+}
+class B {
+  values(): number {
+    return 42;
+  }
+}
+const a = new A();
+console.log("cap1:", [...a.one()].join(","));
+console.log("cap2:", [...a.two()].join(","));
+console.log("cap3:", [...a.three()].join(","));
+console.log("B.values:", new B().values());
+
+// ── a free function (no class in scope) always worked; keep it covered so a
+//    future narrowing of the guard cannot silently break the ordinary path. ──
+function free(): IterableIterator<number> {
+  return [888][Symbol.iterator]();
+}
+console.log("free:", [...free()].join(","));
+
+// ── the array `values`/`keys`/`entries` surface itself must stay intact ──
+const plainArr = [10, 20, 30];
+console.log("arr values:", [...plainArr.values()].join(","));
+console.log("arr keys:", [...plainArr.keys()].join(","));
+console.log("arr entries:", JSON.stringify([...plainArr.entries()]));
+console.log("arr @@iterator:", [...plainArr[Symbol.iterator]()].join(","));
+
+// ── native-base subclass overrides, the family the issue reported against ──
+class MapKeysOverride extends Map<string, number> {
+  keys(): IterableIterator<string> {
+    return ["kk"][Symbol.iterator]();
+  }
+}
+class MapEntriesOverride extends Map<string, number> {
+  entries(): IterableIterator<[string, number]> {
+    return ([["ee", 1]] as [string, number][])[Symbol.iterator]();
+  }
+}
+class MapIterOverride extends Map<string, number> {
+  *[Symbol.iterator](): IterableIterator<[string, number]> {
+    yield ["ii", 9];
+  }
+}
+const mk = new MapKeysOverride();
+mk.set("q", 5);
+console.log("map keys override:", [...mk.keys()].join(","));
+const me = new MapEntriesOverride();
+me.set("q", 5);
+console.log("map entries override:", JSON.stringify([...me.entries()]));
+const mi = new MapIterOverride();
+mi.set("q", 5);
+console.log("map @@iterator override:", JSON.stringify([...mi]));
+
+class SetValuesOverride extends Set<number> {
+  values(): IterableIterator<number> {
+    return [111][Symbol.iterator]();
+  }
+}
+class SetIterOverride extends Set<number> {
+  *[Symbol.iterator](): IterableIterator<number> {
+    yield 444;
+  }
+}
+const sv = new SetValuesOverride();
+sv.add(1);
+console.log("set values override:", [...sv.values()].join(","));
+const si = new SetIterOverride();
+si.add(1);
+console.log("set @@iterator override:", [...si].join(","));
+
+class ArrValuesOverride extends Array<number> {
+  values(): IterableIterator<number> {
+    return [555][Symbol.iterator]();
+  }
+}
+const av = new ArrValuesOverride();
+av.push(1);
+console.log("array values override:", [...av.values()].join(","));
+
+// ── INDIRECT subclass and a class EXPRESSION: the two shapes CLAUDE.md's
+//    "native base-class subclassing" note calls out as historically lossy. ──
+class MidMap extends Map<string, number> {}
+class LeafMap extends MidMap {
+  values(): IterableIterator<number> {
+    return [888][Symbol.iterator]();
+  }
+}
+const lm = new LeafMap();
+lm.set("z", 3);
+console.log("indirect override:", [...lm.values()].join(","));
+
+const ExprMap = class extends Map<string, number> {
+  values(): IterableIterator<number> {
+    return [999][Symbol.iterator]();
+  }
+};
+const em = new ExprMap();
+em.set("z", 3);
+console.log("class-expression override:", [...em.values()].join(","));
+
+// ── non-overriding subclasses keep the built-in surface ──
+class PlainMap extends Map<string, number> {}
+const pm = new PlainMap();
+pm.set("a", 1);
+pm.set("b", 2);
+console.log("no-override values:", [...pm.values()].join(","));
+console.log("no-override keys:", [...pm.keys()].join(","));
+console.log("no-override entries:", JSON.stringify([...pm.entries()]));
+console.log("no-override spread:", JSON.stringify([...pm]));
+
+// ── `super.<m>()` from inside an override still reaches the native base ──
+class SuperMap extends Map<string, number> {
+  values(): IterableIterator<number> {
+    return super.values();
+  }
+}
+const sm = new SuperMap();
+sm.set("a", 1);
+sm.set("b", 2);
+console.log("super.values():", [...sm.values()].join(","));
+
+console.log("done");