Validate native downcasts against the allocated payload layout - #8963

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

sequenceDiagram
  participant PyObject
  participant PyObjVTable
  participant PyPayload
  PyObject->>PyObjVTable: Query native layout support
  PyObjVTable->>PyPayload: Call supports_native_layout(layout)
  PyPayload-->>PyObjVTable: Return layout support
  PyObjVTable-->>PyObject: Return layout support
  PyObject->>PyObject: Check Python class and payload compatibility

Merge Risk: 🟡 Moderate · up to dc78d

Dropping a sufficiently deep frame chain can overflow the native stack. Restore recursion protection for untracked objects before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 43e00

The change strengthens native casts by validating the allocated payload rather than trusting mutable Python class metadata. No newly enabled attack path was established. Residual risk remains because the contract governs interpreter memory safety, and downstream native implementations and concurrent type mutations are not fully verified.

Retained concerns
No architecture-level concerns identified.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 @coderabbitai help to get the list of available commands.}

Merging this PR will improve performance by 15.18%

⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks¹

Performance Changes

_{Comparing 1ndahous3:native_payload_layout (dc78dae) with main (54e47cd)}

@coderabbitai Both findings in your review are addressed in 43e006a9c:

• PySubclass is now an unsafe trait. Its contract requires a valid base prefix, matching payload offsets, compatible alignment, and validity independent of Python class or MRO. Both generators and all manual implementations now use unsafe impl; these guarantees belong to the implementor, while the conversion methods remain safe.
• upcast and upcast_ref now validate the allocation's payload root and native layout instead of requiring Python MRO membership. The owning conversion retains the original allocation pointer, and incompatible layouts are still rejected. Ordinary downcasts retain their Python-class checks.

#!/bin/bash
set -euo pipefail
gh api repos/RustPython/RustPython/pulls/8963/reviews/5407198764
gh api repos/RustPython/RustPython/pulls/8963/comments --paginate --jq '.[] | select(.pull_request_review_id == 5407198764) | {path, line, body}'
git show --stat 43e006a9c9c9c4aa86b17c3c1fb3cfaec4cc2e9d
ast-grep outline crates/vm/src/class.rs --match 'PySubclass' --view expanded
ast-grep outline crates/vm/src/object/core.rs --match 'upcast|into_base' --view expanded
git show 43e006a9c9c9c4aa86b17c3c1fb3cfaec4cc2e9d -- crates/vm/src/class.rs crates/vm/src/object/core.rs

#!/bin/bash
set -euo pipefail
git show 43e006a9c9c9c4aa86b17c3c1fb3cfaec4cc2e9d -- crates/derive-impl/src/pyclass.rs crates/derive-impl/src/pystructseq.rs crates/vm/src/exceptions.rs crates/vm/src/exception_group.rs
rg -n -C2 'impl.*PySubclass' --glob '*.rs' .
sed -n '2855,2925p' crates/vm/src/object/core.rs

@1ndahous3 I checked 43e006a9c. Both findings are addressed.

• PySubclass now places the physical-layout guarantees on unsafe implementors. Both generators and all manual implementations use unsafe impl. The conversion methods remain safe.
• upcast and upcast_ref now validate PAYLOAD_TYPE_ID and NATIVE_LAYOUT_ID without requiring Python MRO membership. The owning conversion preserves the allocation pointer.

The new regression test covers custom MROs and rejection of incompatible layouts. I inspected the test but did not run it.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

Thank you for contributing. about this patch, I investigated the topic and then I found #9004 approach looks better at the moment.

@youknowone in this PR, I was trying to address two things:

1. Make the PySubclass safety contract more explicit. My intention was to require authors of manual implementations to guarantee the base layout and alignment that safe native conversions rely on. I agree that this is not necessary for the specific fix in Give builtin_method its own type #9004, but it seemed to me like a more honest contract at the Rust/Python boundary. I hoped that drawing attention to these assumptions could help prevent future bugs or UB. The downside, of course, is the additional - and perhaps unfamiliar -placement of unsafe impl and // SAFETY: comments.
2. What seemed more important to me was checking casts against the actual allocation, rather than relying on the object's current Python class and its declared size. I also tried to restrict incompatible class/base assignments and reject invalid native upcast targets, since I saw these as related ways to arrive at an incorrect native representation.

For example, consider two macro-defined native classes, Base and Derived, where Derived embeds Base and adds another Rust field. Omitting their definitions:

let _ = Base::make_static_type();
let derived = Derived::make_static_type();

// Allocate only Base, but associate it with Derived's Python class.
let obj = PyRef::new_ref(Base { value: 5 }, derived, None);

assert!(!obj.as_object().downcastable::<Derived>());

With the current validation, this assertion fails: both payloads share the base's PAYLOAD_TYPE_ID, and the remaining checks consult the Python class, which claims to have Derived's layout. A subsequent safe downcast_ref::<Derived>() can therefore produce a reference to a representation that was never allocated.

The proposed allocation check rejects this case. My understanding is that #9004 resolves the specific function/method layout ambiguity, while this more general case remains. That is why I thought this part might still be useful independently.

Would you be open to a smaller follow-up focused on this allocation check? I would be happy to separate the unsafe PySubclass contract or adjust the approach if you think there is a simpler way to cover this.

you are right. I agree about unsafe PySubclass changes. if that part is splitted, it will be merged easy.

Could you please rebase this branch onto a fresh copy of main?

@youknowone @fanninpm I have updated this PR and its code, and moved the unsafe PySubclass changes into #9012.

for other parts, let me take another look later