Skip to content

Sweep: ~20 unrooted js_implicit_this_set(prev) save/restores hold a bare local across allocating user code #9445

Description

@proggeramlug

PR #9444 fixes four instances of this shape in object/field_get_set/accessors.rs. Roughly twenty more remain, all latent, all with the same silent-wrong-answer failure mode.

The shape

let prev = js_implicit_this_set(receiver);
… call user code, which allocates …
js_implicit_this_set(prev);   // pre-collection address

prev is a bare Rust local held across a call that can allocate. An evacuating young-gen minor moves the object it points at, and the restore installs a retired from-space address as the caller's this.

The failure is silent and badly misattributed: nothing faults at the move. The stale pointer is read later, obj_type == GC_TYPE_OBJECT fails on the recycled cell, and the read answers undefined — so the crash surfaces as a property access on undefined several steps downstream, naming a property unrelated to the defect. In #9417 this presented as Cannot read properties of undefined (reading 'def') on cc's auth path, which is nowhere near the real site.

Sites

timer.rs ×3, node_stream.rs ×3, node_stream_readwrite.rs ×4, os_process_streams.rs ×3, property_key.rs ×4, descriptor_state.rs, dgram.rs, event_target.rs, util_promisify.rs, map.rs, set.rs, class_constructors.rs.

Each needs checking individually rather than a blanket rewrite: a site whose intervening call provably cannot allocate does not need rooting, and saying which ones those are is part of the work.

The correct idiom is already in-tree

iterator_helpers.rs:157 and prototype_chain.rs:551/572 root theirs. PR #9444 uses RuntimeHandleScope with a re-read at point of use, which is longjmp-safe because exception.rs saves and restores the handle stack at trap boundaries. Follow that pattern.

Why this matters more than a typical latent bug

Moving young-gen scavenge is perry's default GC (PR #7019), so these are live, not theoretical — and the timer and stream sites sit on paths that every long-running program exercises. map.rs/set.rs are on hot collection paths.

Verification bar

This is the hard part, and a sweep that skips it is not worth landing. Each fix needs a test that can actually fail — a fixture that forces a minor collection inside the accessor/callback window and observes the caller's this afterwards. #9444's test_gap_9417_accessor_this_restore.ts does this deterministically without GC environment knobs and is the model to copy: knob-gated reproductions are too weak, because a fix that merely changes allocation timing will appear to work.

PERRY_GC_PROTECT_FROMSPACE=1 is the tool that makes a stale use fault immediately with the owning object's type and size, instead of failing silently downstream.

Found while root-causing #9417.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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