GC-manage immortal iterator wrappers that hold managed children - #693
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit fc75e32). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/dd61a4dc6e5301aa951c47ef7c040a59d0fed979/pyre-object/src/interp_itertools.rs#L208
This makes W_TakeWhile GC-managed, but its baseobjspace::next arm still keeps only a raw &mut W_TakeWhile while calling the inner iterator, predicate, and is_true. If that Python code drops the last external reference to the takewhile object and runs gc.collect(), the receiver is not on the shadow stack and can be swept before the later it.stopped = true write; root obj and re-read the fields around those calls, as the compress/starmap branches do. The same pattern needs checking for the adjacent newly managed dropwhile/filterfalse arms.
https://github.com/youknowone/pyre/blob/dd61a4dc6e5301aa951c47ef7c040a59d0fed979/pyre-interpreter/src/baseobjspace.rs#L11299
The barrier records the new w_prev child, but after this commit W_Pairwise is collectible and this branch still holds only the raw it pointer across next(it.w_iterator). If the inner iterator's __next__ drops the last Python reference to the pairwise object and triggers gc.collect(), the managed receiver can be reclaimed before the subsequent field write/barrier; pin obj and reload it around both inner next calls.
https://github.com/youknowone/pyre/blob/dd61a4dc6e5301aa951c47ef7c040a59d0fed979/pyre-object/src/operation.rs#L49
Making _CallableIterator managed means its next implementation can now be swept while it invokes the user callable: baseobjspace::next calls call_function_impl_result(callable, &[]) without pinning obj, so a callable that clears the last Python reference to this iterator and runs gc.collect() leaves only the untraced Rust local before the later re-check/set on obj. Please root the receiver across the callable call and equality check before switching this type to allocate_stable.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Convert the `#[pyre_class]` iterator / sequence wrappers whose managed children are held solely through the wrapper from immortal (`allocate`) to GC-managed (`allocate_stable`), so the marker scans them and forwards their inline `PyObjectRef` children on both the interpreter and JIT paths. As immortals the marker skipped them and only the interpreter-side immortal-root walker forwarded the children; over jitframe slots (no immortal walk) a child reachable solely through the wrapper was dropped on a moving collection. pyre-object: the seq / list-reverse / tuple / set iterators, reversed, map / filter / zip, count / repeat / compress / starmap / accumulate / zip_longest / cycle / chain, the SRE scanner and match result, range, the long-range iterator, takewhile / dropwhile / filterfalse / pairwise, and the callable-sentinel iterator. pyre-interpreter: struct.Struct and its unpack iterator, the two deque iterators, and the tokenizer iterator. For the wrappers that were registered only through the immortal-root offset loop (range, long-range, takewhile, dropwhile, filterfalse, pairwise, _CallableIterator, struct/unpack-iter, the deque iterators, the tokenizer iterator): register them through register_pyre_class at the tail of the GC type-id chain (tids 118..129) and add the matching subclass-range hierarchy + alias census entries. The offset loop is removed. Most drop the vestigial `type_id = N` for an auto-assigned tail tid; range keeps an explicit `type_id = 118` pinned to its tail slot because RANGE_DESCR_GROUP bakes `W_RANGE_GC_TYPE_ID` at compile time to virtualize range GET_ITER/FOR_ITER. The SRE match result and scanner were already registered through register_pyre_class, so converting them needs no census change. Add a write barrier to the managed-field setters / next paths that store a child after construction: the long-range index, the count value, the list-iter / list-reverse / tuple / set seq, the callable, the struct format, and the pairwise prev. W_IntRangeIterator stays immortal — it holds only scalar counters. Assisted-by: Claude
1 participant
Problem
The precise collector roots interpreter frames through
walk_pyframe_roots+walk_raw_immortal_roots, which recursively forwards the inlinePyObjectRefchildren of immortal (
allocate)#[pyre_class]wrappers. JIT frames arerooted through
jitframe_trace(the gcmap), which has no immortal walk. Sofor an iterator / sequence wrapper allocated immortal, a managed child reachable
solely through that wrapper (e.g. an enumerate's inner iterator, a deque
iterator's backing deque) was scanned on the interpreter path but dropped on a
moving collection while the wrapper sat in a jitframe slot — a use-after-free
under the JIT.
Fix
Convert every
#[pyre_class]iterator / sequence wrapper whose managed childrenare held solely through the wrapper from immortal (
allocate) to GC-managed(
allocate_stable). Managed wrappers are scanned by the marker via their tid'sptr_offsets, so their children are forwarded on both paths.reversed,map/filter/zip,count/repeat/compress/starmap/accumulate/zip_longest/cycle/chain, the SRE scanner and matchresult,
range, the long-range iterator,takewhile/dropwhile/filterfalse/pairwise, and the callable-sentinel iterator.struct.Structand its unpack iterator, the two dequeiterators, and the tokenizer iterator.
The wrappers that were previously registered only through the immortal-root
offset loop (
range, long-range,takewhile,dropwhile,filterfalse,pairwise,_CallableIterator, struct/unpack-iter, deque iterators, tokenizeriterator) are moved into
register_pyre_classat the tail of the GC type-idchain (tids 118..129), with matching subclass-range hierarchy + alias census
entries; the offset loop is removed. Most drop the vestigial
type_id = Nforan auto-assigned tail tid;
rangekeeps an explicittype_id = 118pinnedto its tail slot, because
RANGE_DESCR_GROUP(added by #683) bakesW_RANGE_GC_TYPE_IDat compile time to virtualize rangeGET_ITER/FOR_ITER.The SRE match result and scanner were already registered through
register_pyre_class, so converting them needs no census change.Write barriers are added to the managed-field setters / next paths that store a
child after construction: the long-range index, the count value, the list-iter /
list-reverse / tuple / set seq, the callable, the struct format, and the
pairwise prev.
W_IntRangeIteratorstays immortal — it holds only scalar counters.Completeness audit
An adversarial audit (four independent lenses — missed conversion, missing
barrier, census consistency, descr-group tid / Rust-heap leak — each finding
verified by a skeptic) swept every
#[pyre_class]struct for the same latentclass. Results:
scanner, holds
w_string/w_bufferreachable solely through the match).Folded into this PR.
(
GET_AWAITABLE,SEND) permanently abort the JIT tracer, so it never landsin a jitframe slot; the immortal-with-registered-offsets gap is inert. Left
immortal (correct).
iterator owns Rust heap (source
String, tokenVecs) with no destructor, soits buffers leak on collection. Pre-existing in nature (as an immortal it was
never freed either) and a separate mechanism (destructor plumbing); deferred
to a follow-up, not a UAF.
Validation
pytype_to_tid↔ subclass-range aliases) + subclass-rangehierarchy + GC-root completeness oracle all green.
converted_smoke.py: every converted type under 200×gc.collect()): JIT output identical to NO_JIT, and identical under aforced 16 KB nursery.
sre_match_gc.py: str + bytes match whose subject is solelyreachable through the match,
gc.collect()between): JIT == NO_JIT == tinynursery.
range_virt_gc.py+ a big-intrangetid check holdingthe range live across
gc.collect()): correct under JIT, GC-stress, and tinynursery — confirming the real managed tid equals the descr-baked
W_RANGE_GC_TYPE_ID.🤖 Generated with Claude Code