Skip to content

json/stringify shape template holds raw obj/keys_array pointers across allocating toJSON recursion (SHAPE_CACHE has no GC root) #7268

Description

@proggeramlug

Raised by CodeRabbit on #7265 and deliberately not fixed there — it is pre-existing, it is not what #7264 was, and a partial mitigation would be worse than none. Writing it down so the decision is explicit rather than implicit.

The claim

json/stringify_shape_template.rs::try_emit_shape_element derives

let obj = elem_ptr as *const crate::ObjectHeader;
let fields_ptr = (elem_ptr as *const u8).add(size_of::<ObjectHeader>()) as *const f64;

once, then loops over the fields calling stringify_value_depth(...) for pointer-valued ones. That call can run a user toJSON, allocate, and therefore trigger a minor GC — which under the C4b evacuation policy is moving. elem_ptr is a bare Rust local, invisible to the collector (production resolves the conservative stack scan to SkipDisabled). After such a collection every later fields_ptr.add(f) read in the same loop is a read of a forwarded/stale address.

stringify_object_inner in json/stringify.rs already does this correctly: RuntimeHandleScope + root_raw_const_ptr, with cur_obj() re-derived on every access and the keys array re-derived through the object header. The template path never got the same treatment.

Why the naive fix is not enough

Rooting obj alone does not close it. The template machinery has two more raw-pointer dependencies with the same exposure:

  1. SHAPE_CACHE is Vec<(*mut ArrayHeader, Box<ShapeTemplate>)> — a raw keys_array pointer used as the shape identity key, held across the whole traversal and never registered with the GC's root-scanner side tables.
  2. try_emit_shape_element matches an element by (*obj).keys_array != template.keys_arr, and set_to_json_key_for_template_field reads key strings out of template.keys_arr.

If a moving minor can run mid-traversal, a stale cache key either misses (falls back — merely slow) or, worse, collides with a newly-allocated array at the recycled address and matches the wrong shape.

The actual question

SHAPE_CACHE's doc comment asserts the invariant that makes all of this safe:

within one top-level stringify call no GC runs over the user object graph (the buffer/result allocations don't move keys arrays), so pointer identity is a stable shape ID

If that invariant holds, there is nothing to fix and the comment should say why it holds (what prevents a toJSON allocation from triggering a moving minor?). If it does not hold, the fix is not a RuntimeHandleScope sprinkle — it is registering the cache with a mutable root scanner (gc/mod.rs's ~55 registered side-table scanners) or suppressing GC/evacuation for the traversal.

So this is a decide-then-implement item, not a patch.

Reproducing / gating

The GC knob kill-policy in CLAUDE.md applies: whatever is decided, the gate must assert its subject was live. A test here needs PERRY_GC_FORCE_EVACUATE=1 plus a toJSON that allocates enough to force a real copying minor mid-array, and must assert copied_objects > 0 — otherwise it proves nothing (see #6942/#6946, where PERRY_GC_FORCE_EVACUATE was inert for every gc()-driven test).

Suggested shape: a homogeneous array of ≥2 objects sharing a keys_array where element 0's fields are primitives (so the template's primitive_only path is taken) and a later element carries a pointer field whose toJSON allocates heavily. Then verify under PERRY_GC_VERIFY_EVACUATION=1.

Not urgent because

The array template path is only entered for class_id == 0 plain data objects with no descriptors in use, and build_shape_prefix_template bails when the first element could resolve a prototype toJSON. The exposure is a later element carrying an own/inherited toJSON or a nested object that does. Real, but narrow.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions