crates/vm/src/builtins/type.rs (1)

1889-1927: Extract shared descriptor-injection logic for __dict__/__weakref__.

The two branches are nearly identical (presence checks + inherited check + getset insertion). A small helper would reduce drift risk and improve maintainability.

As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/builtins/type.rs` around lines 1889 - 1927, The two blocks that
inject a getset descriptor for "__dict__" and "__weakref__" duplicate the same
existence/inheritance checks and insertion logic; extract a small helper (e.g.,
inject_descriptor_if_missing) that takes the VM/context (vm or vm.ctx), the type
object (typ), the interned name (or name string), the getter and setter
functions (subtype_get_dict/subtype_set_dict or
subtype_get_weakref/subtype_set_weakref) and performs: check
typ.slots.flags.has_feature for the relevant PyTypeFlags value, check inherited
presence via typ.mro.read().iter().any(...), check
typ.attributes.read().contains_key(...), and if absent create the descriptor via
vm.ctx.new_getset and insert it into typ.attributes.write().insert(...); then
replace both branches with calls to this helper using the appropriate name,
getter and setter and flag test (PyTypeFlags::HAS_DICT /
PyTypeFlags::HAS_WEAKREF) to remove duplication while preserving behavior.

crates/vm/src/object/core.rs (1)

2123-2147: Extract common bootstrap-type initialization to reduce unsafe duplication

Line 2123-2147 repeats nearly identical unsafe initialization for type_type_ptr and object_type_ptr with only payload differences. A tiny helper would reduce drift risk in this memory-sensitive path.

As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 2123 - 2147, The two unsafe blocks
initializing type_type_ptr and object_type_ptr are duplicated; create a small
helper (e.g., init_type_with_ext or write_type_ptr) that calls
alloc_type_with_ext(), takes the differing payload (type_payload or
object_payload) and the Uninit { typ } marker, performs the unsafe
partially_init! write of a PyInner::<PyType> (setting ref_count, vtable via
PyObjVTable::of::<PyType>(), gc_bits, and payload) and returns the allocated
pointer; then replace the duplicated unsafe blocks by calling this helper twice
with the appropriate payloads (type_payload and object_payload) to centralize
the unsafe logic and avoid drift.

crates/vm/src/object/core.rs (1)

1062-1070: ⚠️ Potential issue | 🔴 Critical

Take the weakref stripe lock before dereferencing head_ptr.

Line 1064 loads and Line 1068 dereferences wrl.head without synchronization, which can race with list mutation/unlink and read freed memory.

🔒 Suggested fix

 pub(crate) fn get_weakrefs(&self) -> Option<PyObjectRef> {
     let wrl = self.weak_ref_list()?;
+    let _lock = weakref_lock::lock(self as *const PyObject as usize);
     let head_ptr = wrl.head.load(Ordering::Relaxed);
     if head_ptr.is_null() {
         None
     } else {
         let head = unsafe { &*head_ptr };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 1062 - 1070, get_weakrefs
currently loads wrl.head into head_ptr and then dereferences it without
synchronization, risking a race with list mutation/unlink; fix by acquiring the
weakref stripe lock used for list mutations before reading/dereferencing
wrl.head (use the same stripe-lock API on the wrl returned by weak_ref_list()),
re-check head_ptr/is_null after acquiring the lock, and only then call unsafe {
&*head_ptr } and PyRef::from_raw(head_ptr); ensure the lock is released after
these checks so you hold the same lock that protects unlinking/mutation during
the deref.

crates/vm/src/object/core.rs (1)

281-323: ⚠️ Potential issue | 🟠 Major

Derive ObjExt offset from Layout::extend inside ext_ref() instead of hardcoding EXT_OFFSET.

Line 285 assumes zero inter-padding universally, but Line 289 only proves alignment against PyInner<()>. If any PyInner<T> has stricter alignment, ext_ref() can compute the wrong prefix address.

Suggested fix

-const EXT_OFFSET: usize = core::mem::size_of::<ObjExt>();
-// Guarantee: ObjExt size is a multiple of its alignment, and its alignment
-// is >= any PyInner alignment, so Layout::extend produces no inter-padding.
-const _: () = assert!(core::mem::size_of::<ObjExt>() % core::mem::align_of::<ObjExt>() == 0);
-const _: () = assert!(core::mem::align_of::<ObjExt>() >= core::mem::align_of::<PyInner<()>>());
+const _: () = assert!(core::mem::size_of::<ObjExt>() % core::mem::align_of::<ObjExt>() == 0);

 impl<T> PyInner<T> {
@@
     pub(super) fn ext_ref(&self) -> Option<&ObjExt> {
         if !GcBits::from_bits_retain(self.gc_bits.load(Ordering::Relaxed)).contains(GcBits::HAS_EXT)
         {
             return None;
         }
+        let inner_layout = core::alloc::Layout::new::<Self>();
+        let ext_layout = core::alloc::Layout::new::<ObjExt>();
+        let (_, inner_offset) = ext_layout
+            .extend(inner_layout)
+            .expect("ObjExt + PyInner layout overflow");
         let self_addr = (self as *const Self as *const u8).addr();
-        let ext_ptr =
-            core::ptr::with_exposed_provenance::<ObjExt>(self_addr.wrapping_sub(EXT_OFFSET));
+        let ext_ptr = core::ptr::with_exposed_provenance::<ObjExt>(
+            self_addr.wrapping_sub(inner_offset),
+        );
         Some(unsafe { &*ext_ptr })
     }
 }

#!/bin/bash
set -euo pipefail

echo "== Potential >8-byte alignment payload indicators =="
rg -nP --type rust 'repr\s*\(\s*align\((?:1[6-9]|[2-9][0-9])\)\s*\)|\b(u128|i128|__m\d+)\b' crates/vm/src -C1 || true

echo
echo "== PyPayload impls to audit for effective alignment =="
rg -nP --type rust 'impl\s+.*\bPyPayload\b\s+for\s+' crates/vm/src -C1

echo
echo "== Current ext-offset derivation sites =="
rg -nP --type rust 'EXT_OFFSET|with_exposed_provenance|Layout::extend' crates/vm/src/object/core.rs -C2

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 281 - 323, The ext_ref() method
must compute the ObjExt prefix offset using Layout::extend at runtime for the
actual PyInner<T> instead of using the hardcoded EXT_OFFSET; change ext_ref() to
compute let ( _objext_layout, offset) =
core::alloc::Layout::new::<ObjExt>().extend(core::alloc::Layout::new::<PyInner<T>>()).unwrap();
then use self_addr.wrapping_sub(offset) with
core::ptr::with_exposed_provenance::<ObjExt>(...) to get ext_ptr; update
references to EXT_OFFSET in ext_ref() (and remove or keep the const as unused)
so alignment/padding for the concrete T is respected.

crates/vm/src/object/core.rs (2)

890-897: ⚠️ Potential issue | 🟠 Major

Enforce HAS_DICT allocation invariant at the constructor boundary.

needs_ext is currently derived from dict.is_some(), so a direct PyRef::new_ref(..., dict=None) for a HAS_DICT type can allocate without dict/ext and make instance_dict()/set_dict() fail unexpectedly. Add a debug assertion in PyInner<T>::new.

Proposed fix

     fn new(payload: T, typ: PyTypeRef, dict: Option<PyDictRef>) -> *mut Self {
         let member_count = typ.slots.member_count;
+        let has_dict_flag = typ
+            .slots
+            .flags
+            .has_feature(crate::types::PyTypeFlags::HAS_DICT);
+        debug_assert!(
+            !has_dict_flag || dict.is_some(),
+            "HAS_DICT types must pass an initialized instance dict when allocating"
+        );
         let needs_ext = dict.is_some()
             || typ
                 .slots
                 .flags
                 .has_feature(crate::types::PyTypeFlags::HAS_WEAKREF)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 890 - 897, The constructor
currently derives needs_ext from dict.is_some(), which allows creating a PyInner
for a type with PyTypeFlags::HAS_DICT without providing a dict; add a debug
assertion inside PyInner<T>::new that enforces the allocation invariant: if
typ.slots.flags.has_feature(crate::types::PyTypeFlags::HAS_DICT) then
dict.is_some() (or equivalent) must be true; update the assertion location in
PyInner::new so it runs before using needs_ext/allocating extensions and
reference the existing typ.slots.flags and dict parameters to locate where to
add the check.

---

281-285: ⚠️ Potential issue | 🔴 Critical

Hardcoded EXT_OFFSET can misaddress ObjExt for stricter PyInner<T> alignment.

PyInner<T>::new/PyInner<T>::dealloc correctly use Layout::extend, but ext_ref() subtracts size_of::<ObjExt>() directly. That offset is only guaranteed when no inter-layout padding is inserted; this can silently break pointer arithmetic if a payload raises PyInner<T> alignment.

Proposed fix

-const EXT_OFFSET: usize = core::mem::size_of::<ObjExt>();
-
 impl<T> PyInner<T> {
@@
     pub(super) fn ext_ref(&self) -> Option<&ObjExt> {
         if !GcBits::from_bits_retain(self.gc_bits.load(Ordering::Relaxed)).contains(GcBits::HAS_EXT)
         {
             return None;
         }
         let self_addr = (self as *const Self as *const u8).addr();
-        let ext_ptr =
-            core::ptr::with_exposed_provenance::<ObjExt>(self_addr.wrapping_sub(EXT_OFFSET));
+        let (_, inner_offset) = core::alloc::Layout::new::<ObjExt>()
+            .extend(core::alloc::Layout::new::<Self>())
+            .expect("ObjExt + PyInner layout overflow");
+        let ext_ptr = core::ptr::with_exposed_provenance::<ObjExt>(
+            self_addr.wrapping_sub(inner_offset),
+        );
         Some(unsafe { &*ext_ptr })
     }
 }

Does Rust `core::alloc::Layout::extend` insert padding before the second layout when it has stricter alignment than the first (so offset may be greater than `size_of::<prefix>()`)?

Also applies to: 316-324

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 281 - 285, EXT_OFFSET is hardcoded
to size_of::<ObjExt>(), which can be wrong if PyInner<T> has stricter alignment
than ObjExt; update ext_ref() (and the analogous code at 316-324) to compute the
prefix->payload offset using Layout::extend rather than assuming no padding.
Specifically, replace uses of EXT_OFFSET (and the const) with a computed offset
obtained by creating Layout::new::<ObjExt>() and calling
.extend(Layout::new::<PyInner<T>>()) (or otherwise using Layout::extend for the
actual payload type) and use the returned .0.size()/.1 or the computed offset to
do the pointer subtraction so pointer arithmetic matches what PyInner::new and
PyInner::dealloc use.

crates/vm/src/object/core.rs (1)

279-289: ⚠️ Potential issue | 🟠 Major

EXT_OFFSET still assumes no extra Layout::extend padding for all payload alignments.

Line 288 only proves the constraint for PyInner<()>, not for all PyInner<T>. If any payload has alignment > 8, alloc/dealloc offsets (computed via Layout::extend) can diverge from Line 339’s fixed subtraction. This was already flagged earlier and still applies here.

#!/bin/bash
set -euo pipefail

echo "== Candidate high-alignment declarations in vm =="
rg -nP --type rust '#\s*\[\s*repr\s*\(\s*align\((?:[9-9]|[1-9][0-9]+)\)\s*\)\s*\]' crates/vm/src || true

echo
echo "== Payload candidates that often imply >8 alignment =="
rg -nP --type rust '\b(u128|i128|core::arch::|std::arch::|__m\d+)\b' crates/vm/src || true

echo
echo "== PyPayload implementations =="
rg -nP --type rust 'impl\s+.*\bPyPayload\b\s+for\s+' crates/vm/src -C1

Expected verification result: either (1) a hard guarantee that all payload alignments are <= align_of::<ObjExt>(), or (2) ext pointer recovery that does not rely on a fixed constant offset.

Also applies to: 333-340