Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cspell.dict/cpython.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ finalizers
firsttraceable
flowgraph
formatfloat
freelist
freevar
freevars
fromlist
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ use crate::{
vm::VirtualMachine,
};
use alloc::fmt;
use core::cell::Cell;
use core::ptr::NonNull;
use rustpython_common::lock::PyMutex;
use rustpython_common::wtf8::Wtf8Buf;

Expand Down Expand Up @@ -60,11 +62,43 @@ impl fmt::Debug for PyDict {
}
}

thread_local! {
static DICT_FREELIST: Cell<crate::object::FreeList<PyDict>> = const { Cell::new(crate::object::FreeList::new()) };
}

impl PyPayload for PyDict {
const MAX_FREELIST: usize = 80;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.dict_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
DICT_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
DICT_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
}

impl PyDict {
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/float.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ use crate::{
protocol::PyNumberMethods,
types::{AsNumber, Callable, Comparable, Constructor, Hashable, PyComparisonOp, Representable},
};
use core::cell::Cell;
use core::ptr::NonNull;
use malachite_bigint::{BigInt, ToBigInt};
use num_complex::Complex64;
use num_traits::{Signed, ToPrimitive, Zero};
Expand All @@ -32,11 +34,43 @@ impl PyFloat {
}
}

thread_local! {
static FLOAT_FREELIST: Cell<crate::object::FreeList<PyFloat>> = const { Cell::new(crate::object::FreeList::new()) };
}

impl PyPayload for PyFloat {
const MAX_FREELIST: usize = 100;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.float_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
FLOAT_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
FLOAT_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
}

impl ToPyObject for f64 {
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/builtins/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ use crate::{
use rustpython_common::wtf8::Wtf8Buf;

use alloc::fmt;
use core::cell::Cell;
use core::ops::DerefMut;
use core::ptr::NonNull;

#[pyclass(module = false, name = "list", unhashable = true, traverse = "manual")]
#[derive(Default)]
Expand Down Expand Up @@ -72,11 +74,43 @@ unsafe impl Traverse for PyList {
}
}

thread_local! {
static LIST_FREELIST: Cell<crate::object::FreeList<PyList>> = const { Cell::new(crate::object::FreeList::new()) };
}

impl PyPayload for PyList {
const MAX_FREELIST: usize = 80;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.list_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
LIST_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
LIST_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
}

impl ToPyObject for Vec<PyObjectRef> {
Expand Down
56 changes: 55 additions & 1 deletion crates/vm/src/builtins/slice.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// sliceobject.{h,c} in CPython
// spell-checker:ignore sliceobject
use core::cell::Cell;
use core::ptr::NonNull;
use rustpython_common::wtf8::{Wtf8Buf, wtf8_concat};

use super::{PyGenericAlias, PyStrRef, PyTupleRef, PyType, PyTypeRef};
Expand All @@ -15,19 +17,71 @@ use crate::{
use malachite_bigint::{BigInt, ToBigInt};
use num_traits::{One, Signed, Zero};

#[pyclass(module = false, name = "slice", unhashable = true, traverse)]
#[pyclass(module = false, name = "slice", unhashable = true, traverse = "manual")]
#[derive(Debug)]
pub struct PySlice {
pub start: Option<PyObjectRef>,
pub stop: PyObjectRef,
pub step: Option<PyObjectRef>,
}

// SAFETY: Traverse properly visits all owned PyObjectRefs
unsafe impl crate::object::Traverse for PySlice {
fn traverse(&self, traverse_fn: &mut crate::object::TraverseFn<'_>) {
self.start.traverse(traverse_fn);
self.stop.traverse(traverse_fn);
self.step.traverse(traverse_fn);
}

fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
if let Some(start) = self.start.take() {
out.push(start);
}
// stop is not Option, so it will be freed when payload is dropped
// (via drop_in_place on freelist pop, or Box::from_raw on dealloc)
if let Some(step) = self.step.take() {
out.push(step);
}
}
}

thread_local! {
static SLICE_FREELIST: Cell<crate::object::FreeList<PySlice>> = const { Cell::new(crate::object::FreeList::new()) };
}

impl PyPayload for PySlice {
const MAX_FREELIST: usize = 1;
const HAS_FREELIST: bool = true;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
ctx.types.slice_type
}

#[inline]
unsafe fn freelist_push(obj: *mut PyObject) -> bool {
SLICE_FREELIST.with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
true
} else {
false
};
fl.set(list);
stored
})
}

#[inline]
unsafe fn freelist_pop() -> Option<NonNull<PyObject>> {
SLICE_FREELIST.with(|fl| {
let mut list = fl.take();
let result = list.pop().map(|p| unsafe { NonNull::new_unchecked(p) });
fl.set(list);
result
})
}
Comment on lines +48 to +84
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if PySlice has a proper clear/tp_clear implementation

# Search for Traverse impl on PySlice
echo "=== PySlice Traverse implementation ==="
rg -nA 15 'impl Traverse for PySlice' crates/vm/src/builtins/slice.rs

# Check the pyclass attribute for traverse behavior
echo -e "\n=== PySlice pyclass attributes ==="
rg -nB 2 'pub struct PySlice' crates/vm/src/builtins/slice.rs

# Check if there's a manual clear implementation
echo -e "\n=== Manual clear implementations in slice.rs ==="
rg -n 'fn clear' crates/vm/src/builtins/slice.rs

# Also check the full struct definition to see field types
echo -e "\n=== Full PySlice struct definition ==="
rg -nA 10 'pub struct PySlice' crates/vm/src/builtins/slice.rs

Repository: RustPython/RustPython

Length of output: 673


🏁 Script executed:

#!/bin/bash
# Look at other PyPayload implementations with both traverse and freelist
echo "=== PyPayload implementations with freelist ==="
rg -l 'impl PyPayload' crates/vm/src/builtins/ | head -5

# Check one of them to see if they have explicit clear impls
echo -e "\n=== Example: PyTuple with traverse and freelist ==="
rg -nB 5 'TUPLE_FREELIST' crates/vm/src/builtins/tuple.rs | head -30

# Look for Traverse trait definition to understand what it provides
echo -e "\n=== Traverse trait definition ==="
rg -nA 20 'pub trait Traverse' crates/vm/object/mod.rs

# Search for how traverse macro generates clear
echo -e "\n=== Search for clear generation in macro or docs ==="
rg -n 'fn clear' crates/vm/src/builtins/tuple.rs | head -10

Repository: RustPython/RustPython

Length of output: 556


🏁 Script executed:

#!/bin/bash
# Find where Traverse trait is defined
echo "=== Locate Traverse trait ==="
rg -n 'pub trait Traverse' crates/

# Look at PyTuple's clear implementation in detail
echo -e "\n=== PyTuple clear implementation ==="
rg -nB 3 -A 10 'fn clear\(&mut self' crates/vm/src/builtins/tuple.rs

# Check if PyTuple has freelist and traverse
echo -e "\n=== PyTuple pyclass and freelist setup ==="
rg -nB 2 'pub struct PyTuple' crates/vm/src/builtins/tuple.rs | head -20

# Check what the traverse attribute does in the macro
echo -e "\n=== Search for traverse attribute handling ==="
rg -n 'traverse' crates/vm/src/builtins/tuple.rs | head -5

Repository: RustPython/RustPython

Length of output: 1097


🏁 Script executed:

#!/bin/bash
# Search for Traverse trait more broadly
echo "=== Find Traverse trait definition ==="
fd -e rs -x rg -l 'trait Traverse' {} \;

# Look at the actual imports in slice.rs to understand what's being used
echo -e "\n=== Imports in slice.rs ==="
head -20 crates/vm/src/builtins/slice.rs

# Check if there's a Traverse impl somewhere for PySlice or if it's generated
echo -e "\n=== All Traverse impls in builtins ==="
rg -n 'impl.*Traverse' crates/vm/src/builtins/slice.rs crates/vm/src/builtins/tuple.rs

# Look at other traverse implementations to understand the pattern
echo -e "\n=== Other examples with 'traverse' attribute ==="
rg -B 2 'traverse\)' crates/vm/src/builtins/weakref.rs | head -20

Repository: RustPython/RustPython

Length of output: 1091


🏁 Script executed:

#!/bin/bash
# Look at Traverse trait definition
echo "=== Traverse trait definition ==="
cat crates/vm/src/object/traverse.rs | head -60

# Check if PySlice explicitly implements Traverse anywhere
echo -e "\n=== Explicit Traverse impl for PySlice ==="
rg -n 'impl.*Traverse.*PySlice' crates/vm/src/builtins/slice.rs

# Look at the macro to see what traverse attribute generates
echo -e "\n=== Search for pyclass macro handling of traverse ==="
fd -name '*.rs' -type f crates/pyclass/ | head -5

# Check if there's auto-generation documented
echo -e "\n=== Check if traverse generates Traverse impl automatically ==="
rg -n 'unsafe impl Traverse' crates/vm/src/builtins/ | head -10

Repository: RustPython/RustPython

Length of output: 3825


🏁 Script executed:

#!/bin/bash
# Look for pyclass macro documentation or implementation
echo "=== Search for pyclass macro procedural macro code ==="
find crates -name "*.rs" -path "*pyclass*" | grep -E "(proc|macro)" | head -10

# Check if there are examples where traverse attribute auto-generates clear
echo -e "\n=== Look for generated Traverse impl patterns in builtins ==="
rg -A 8 'unsafe impl Traverse' crates/vm/src/builtins/ | grep -E '(impl Traverse|fn clear)' | head -20

# Specifically check what traverse attribute value PySlice uses vs others
echo -e "\n=== Compare traverse attribute values ==="
rg '#\[pyclass.*traverse' crates/vm/src/builtins/ | head -10

# Check if there's a default/generated Traverse impl somewhere
echo -e "\n=== Look for MaybeTraverse impl on PySlice ==="
rg -A 15 'impl MaybeTraverse' crates/vm/src/builtins/slice.rs

Repository: RustPython/RustPython

Length of output: 2607


🏁 Script executed:

#!/bin/bash
# Check if there's auto-generated Traverse impl or if it relies on MaybeTraverse
echo "=== Check for any Traverse impl for PySlice or if it's missing ==="
rg -n 'Traverse' crates/vm/src/builtins/slice.rs

# Look at how freelist_push is used in other types to see the pattern
echo -e "\n=== Example: Look at PyTuple freelist (even though commented) ==="
rg -B 5 -A 15 'freelist_push' crates/vm/src/builtins/tuple.rs | head -30

# Check how freelist_pop/push interact with Traverse in other payload types
echo -e "\n=== Look at list.rs freelist with Traverse ==="
rg -B 3 -A 15 'freelist_push' crates/vm/src/builtins/list.rs | head -35

# Check if there's any documentation about freelist and Traverse interaction
echo -e "\n=== Search for freelist documentation or comments ==="
rg -B 2 -A 2 'freelist' crates/vm/src/builtins/list.rs | head -20

Repository: RustPython/RustPython

Length of output: 1057


🏁 Script executed:

#!/bin/bash
# Check if there's any generated code or if traverse attribute is just declarative
echo "=== Search for how pyclass macro processes traverse attribute ==="
find crates -name "*.toml" | xargs grep -l "proc-macro\|proc_macro" | head -5

# Look at actual pyclass macro crate
echo -e "\n=== Find pyclass macro crate ==="
find . -type d -name "*pyclass*" | grep -v target

# Check if PySlice should have Traverse but doesn't
echo -e "\n=== Verify PySlice has NO Traverse impl anywhere ==="
rg 'unsafe impl Traverse for PySlice' crates/

# Check if there's a compile-time verification that Traverse is needed
echo -e "\n=== Look at PyPayload trait to understand freelist requirements ==="
rg -B 5 -A 25 'pub trait PyPayload' crates/vm/src/object/

# Check if there are any build/compile errors related to PySlice
echo -e "\n=== Check if PySlice Traverse impl exists in expanded macros ==="
grep -r 'impl.*Traverse.*PySlice' crates/ 2>/dev/null | head -10

Repository: RustPython/RustPython

Length of output: 2411


🏁 Script executed:

#!/bin/bash
# Check the pyclass macro implementation to understand traverse attribute handling
echo "=== Check derive crate for pyclass macro ==="
cat crates/derive/Cargo.toml | grep -A 5 "\[package\]"

# Look at the macro implementation
echo -e "\n=== Search for traverse handling in derive macro ==="
find crates/derive-impl -name "*.rs" -type f | head -10

# Check if there's code that generates Traverse impl
echo -e "\n=== Look for Traverse impl generation ==="
rg -l 'Traverse' crates/derive-impl/ | head -5

# Check whether types with just "traverse" attribute get auto-generated impl
echo -e "\n=== Look at simple types with traverse attribute ==="
rg -B 3 -A 20 '#\[pyclass.*traverse\]\]' crates/vm/src/builtins/iter.rs | head -40

# Check if iter types have Traverse impl
echo -e "\n=== Check if iter.rs types have explicit Traverse impl ==="
rg 'unsafe impl Traverse' crates/vm/src/builtins/iter.rs

Repository: RustPython/RustPython

Length of output: 969


🏁 Script executed:

#!/bin/bash
# Look at the pytraverse macro implementation
echo "=== Check pytraverse.rs which generates Traverse impls ==="
cat crates/derive-impl/src/pytraverse.rs

# Also check pyclass.rs to see how traverse attribute is handled
echo -e "\n=== Check pyclass.rs for traverse attribute handling ==="
rg -n 'traverse' crates/derive-impl/src/pyclass.rs | head -20

Repository: RustPython/RustPython

Length of output: 5653


🏁 Script executed:

#!/bin/bash
# Now check what actually happens when freelist_push is called
# Let's look at where freelist_push is called in the allocation/deallocation flow
echo "=== Search for where freelist_push is called ==="
rg -n 'freelist_push' crates/vm/src/ | head -10

# Check if tp_dealloc or similar calls clear before freelist_push
echo -e "\n=== Look at object deallocation logic ==="
rg -B 10 -A 10 'freelist_push' crates/vm/src/object/payload.rs | head -40

# Verify if the auto-generated Traverse impl includes clear or just traverse
echo -e "\n=== Check if derived Traverse includes clear method ==="
rg -A 20 'impl_pytraverse' crates/derive-impl/src/pytraverse.rs | grep -E '(clear|fn)' | head -10

Repository: RustPython/RustPython

Length of output: 1807


🏁 Script executed:

#!/bin/bash
# The key is the safety comment - let's find where tp_clear is actually called
echo "=== Search for tp_clear implementation ==="
rg -n 'tp_clear' crates/vm/src/object/ | head -20

# Look for where tp_clear is invoked before freelist_push
echo -e "\n=== Search for drop_slow_inner which is mentioned in the safety comment ==="
rg -B 5 -A 15 'drop_slow_inner' crates/vm/src/object/core.rs | head -50

# Check if Traverse::clear is explicitly called anywhere
echo -e "\n=== Check if Traverse::clear is called in deallocation ==="
rg -n 'Traverse::clear\|\.clear\(' crates/vm/src/object/core.rs | head -20

Repository: RustPython/RustPython

Length of output: 3659


🏁 Script executed:

#!/bin/bash
# Let's see the full deallocation flow to understand when clear is called
echo "=== Look at the full default_dealloc function ==="
rg -A 50 'pub\(super\) unsafe fn default_dealloc' crates/vm/src/object/core.rs | head -80

# Also check the traverse_object.rs for tp_clear implementation
echo -e "\n=== Check traverse_object.rs ==="
cat crates/vm/src/object/traverse_object.rs | head -100

Repository: RustPython/RustPython

Length of output: 5659


🏁 Script executed:

#!/bin/bash
# Check what the pyclass macro generates for try_clear
echo "=== Look at pyclass.rs MaybeTraverse generation more carefully ==="
rg -B 5 -A 30 'fn try_clear' crates/derive-impl/src/pyclass.rs

# Also check what the actual generated MaybeTraverse should look like
echo -e "\n=== Check if there's a default try_clear that calls Traverse::clear ==="
rg -n 'try_clear' crates/vm/src/object/core.rs

Repository: RustPython/RustPython

Length of output: 2069


🏁 Script executed:

#!/bin/bash
# Find the try_clear_body generation in pyclass.rs
echo "=== Find try_clear_body generation ==="
rg -B 10 'try_clear_body' crates/derive-impl/src/pyclass.rs | head -40

# Check if there's a template for what try_clear_body should be
echo -e "\n=== Search for all try_clear mentions in pyclass.rs ==="
rg -n 'try_clear' crates/derive-impl/src/pyclass.rs

Repository: RustPython/RustPython

Length of output: 1230


🏁 Script executed:

#!/bin/bash
# Get the actual try_clear_body content
echo "=== Get try_clear_body content ==="
rg -A 3 'let try_clear_body = if has_clear' crates/derive-impl/src/pyclass.rs

Repository: RustPython/RustPython

Length of output: 247


Implement explicit Traverse::clear() for PySlice to extract child references before freelist caching.

PySlice contains PyObjectRef fields (start, stop, step) and has HAS_FREELIST = true. The #[pyclass(traverse)] attribute auto-generates only the traverse() method; the default clear() implementation does nothing. Before a PySlice is pushed to the freelist, tp_clear (called during deallocation) must extract these child references to avoid caching objects with stale references. Add an explicit unsafe impl Traverse for PySlice that implements a clear() method to extract all three fields into the output vector, similar to how PyTuple, PyList, and PyDict handle this.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/builtins/slice.rs` around lines 28 - 64, PySlice currently has
HAS_FREELIST = true but relies on the auto-generated traverse() only, so
tp_clear won't extract its PyObjectRef fields before freelist caching; add an
explicit unsafe impl Traverse for PySlice that implements clear(&mut self, out:
&mut Vec<PyObjectRef>) and pushes self.start, self.stop, and self.step (if
present) into out (mirroring PyTuple/PyList/PyDict patterns) so child references
are removed prior to freelist_push; ensure the impl coexists with the existing
traverse() and freelist_* methods (freelist_push, freelist_pop) so tp_clear
behavior is correct before objects are cached.

}

#[pyclass(with(Comparable, Representable, Hashable))]
Expand Down
3 changes: 3 additions & 0 deletions crates/vm/src/builtins/tuple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ unsafe impl Traverse for PyTuple {
}
}

// No freelist for PyTuple: structseq types (stat_result, struct_time, etc.)
// are static subtypes sharing the same Rust payload, making type-safe reuse
// impractical without a type-pointer comparison at push time.
impl PyPayload for PyTuple {
#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand Down
106 changes: 95 additions & 11 deletions crates/vm/src/object/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,19 @@ pub(super) unsafe fn default_dealloc<T: PyPayload>(obj: *mut PyObject) {
// Untrack from GC BEFORE deallocation.
if obj_ref.is_gc_tracked() {
let ptr = unsafe { NonNull::new_unchecked(obj) };
rustpython_common::refcount::try_defer_drop(move || {
// untrack_object only removes the pointer address from a HashSet.
// It does NOT dereference the pointer, so it's safe even after deallocation.
unsafe {
crate::gc_state::gc_state().untrack_object(ptr);
}
});
if T::HAS_FREELIST {
// Freelist types must untrack immediately to avoid race conditions:
// a deferred untrack could remove a re-tracked entry after reuse.
unsafe { crate::gc_state::gc_state().untrack_object(ptr) };
} else {
rustpython_common::refcount::try_defer_drop(move || {
// untrack_object only removes the pointer address from a HashSet.
// It does NOT dereference the pointer, so it's safe even after deallocation.
unsafe {
crate::gc_state::gc_state().untrack_object(ptr);
}
});
}
}

// Extract child references before deallocation to break circular refs (tp_clear)
Expand All @@ -186,8 +192,17 @@ pub(super) unsafe fn default_dealloc<T: PyPayload>(obj: *mut PyObject) {
unsafe { clear_fn(obj, &mut edges) };
}

// Deallocate the object memory
drop(unsafe { Box::from_raw(obj as *mut PyInner<T>) });
// Try to store in freelist for reuse; otherwise deallocate.
// Only exact types (not heaptype subclasses) go into the freelist,
// because the pop site assumes the cached typ matches the base type.
let pushed = if T::HAS_FREELIST && obj_ref.class().heaptype_ext.is_none() {
unsafe { T::freelist_push(obj) }
} else {
false
};
if !pushed {
drop(unsafe { Box::from_raw(obj as *mut PyInner<T>) });
}

// Drop child references - may trigger recursive destruction.
// The object is already deallocated, so circular refs are broken.
Expand Down Expand Up @@ -807,6 +822,52 @@ impl<T: PyPayload + core::fmt::Debug> PyInner<T> {
}
}

/// Thread-local freelist storage that properly deallocates cached objects
/// on thread teardown.
///
/// Wraps a `Vec<*mut PyObject>` and implements `Drop` to convert each
/// raw pointer back into `Box<PyInner<T>>` for proper deallocation.
pub(crate) struct FreeList<T: PyPayload> {
items: Vec<*mut PyObject>,
_marker: core::marker::PhantomData<T>,
}

impl<T: PyPayload> FreeList<T> {
pub(crate) const fn new() -> Self {
Self {
items: Vec::new(),
_marker: core::marker::PhantomData,
}
}
}

impl<T: PyPayload> Default for FreeList<T> {
fn default() -> Self {
Self::new()
}
}

impl<T: PyPayload> Drop for FreeList<T> {
fn drop(&mut self) {
for ptr in self.items.drain(..) {
drop(unsafe { Box::from_raw(ptr as *mut PyInner<T>) });
}
}
}

impl<T: PyPayload> core::ops::Deref for FreeList<T> {
type Target = Vec<*mut PyObject>;
fn deref(&self) -> &Self::Target {
&self.items
}
}

impl<T: PyPayload> core::ops::DerefMut for FreeList<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.items
}
}

/// The `PyObjectRef` is one of the most used types. It is a reference to a
/// python object. A single python object can have multiple references, and
/// this reference counting is accounted for by this type. Use the `.clone()`
Expand Down Expand Up @@ -1720,8 +1781,31 @@ impl<T: PyPayload + crate::object::MaybeTraverse + core::fmt::Debug> PyRef<T> {
pub fn new_ref(payload: T, typ: crate::builtins::PyTypeRef, dict: Option<PyDictRef>) -> Self {
let has_dict = dict.is_some();
let is_heaptype = typ.heaptype_ext.is_some();
let inner = Box::into_raw(PyInner::new(payload, typ, dict));
let ptr = unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) };

// Try to reuse from freelist (exact type only, no dict, no heaptype)
let cached = if !has_dict && !is_heaptype {
unsafe { T::freelist_pop() }
} else {
None
};

let ptr = if let Some(cached) = cached {
let inner = cached.as_ptr() as *mut PyInner<T>;
unsafe {
core::ptr::write(&mut (*inner).ref_count, RefCount::new());
(*inner).gc_bits.store(0, Ordering::Relaxed);
core::ptr::drop_in_place(&mut (*inner).payload);
core::ptr::write(&mut (*inner).payload, payload);
// typ, vtable, slots are preserved; dict is None, weak_list was
// cleared by drop_slow_inner before freelist push
}
// Drop the caller's typ since the cached object already holds one
drop(typ);
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
Comment on lines +1792 to +1804
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reused freelist objects keep the old typ, which can return objects with stale class identity.

In the cached-object branch, the incoming typ is dropped and the cached typ is preserved. Because freelists are keyed by payload type T (not by typ instance), reuse across context lifetimes on the same thread can bind a new object to an old typ.

Suggested fix
 let ptr = if let Some(cached) = cached {
     let inner = cached.as_ptr() as *mut PyInner<T>;
     unsafe {
         core::ptr::write(&mut (*inner).ref_count, RefCount::new());
         (*inner).gc_bits.store(0, Ordering::Relaxed);
+        core::ptr::drop_in_place(&mut (*inner).typ);
+        core::ptr::write(&mut (*inner).typ, PyAtomicRef::from(typ));
         core::ptr::drop_in_place(&mut (*inner).payload);
         core::ptr::write(&mut (*inner).payload, payload);
-        // typ, vtable, slots are preserved; dict is None, weak_list was
+        // vtable, slots are preserved; dict is None, weak_list was
         // cleared by drop_slow_inner before freelist push
     }
-    // Drop the caller's typ since the cached object already holds one
-    drop(typ);
     unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
 } else {

As per coding guidelines: "Follow Rust best practices for error handling and memory management".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let ptr = if let Some(cached) = cached {
let inner = cached.as_ptr() as *mut PyInner<T>;
unsafe {
core::ptr::write(&mut (*inner).ref_count, RefCount::new());
(*inner).gc_bits.store(0, Ordering::Relaxed);
core::ptr::drop_in_place(&mut (*inner).payload);
core::ptr::write(&mut (*inner).payload, payload);
// typ, vtable, slots are preserved; dict is None, weak_list was
// cleared by drop_slow_inner before freelist push
}
// Drop the caller's typ since the cached object already holds one
drop(typ);
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
let ptr = if let Some(cached) = cached {
let inner = cached.as_ptr() as *mut PyInner<T>;
unsafe {
core::ptr::write(&mut (*inner).ref_count, RefCount::new());
(*inner).gc_bits.store(0, Ordering::Relaxed);
core::ptr::drop_in_place(&mut (*inner).typ);
core::ptr::write(&mut (*inner).typ, PyAtomicRef::from(typ));
core::ptr::drop_in_place(&mut (*inner).payload);
core::ptr::write(&mut (*inner).payload, payload);
// vtable, slots are preserved; dict is None, weak_list was
// cleared by drop_slow_inner before freelist push
}
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
🤖 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 1792 - 1804, The cached-object
branch currently drops the incoming typ and leaves the cached object's typ
unchanged, causing stale class identity; instead, in the cached branch (inside
the unsafe block handling inner: *mut PyInner<T>) replace the stored typ with
the incoming typ: drop the old typ in-place (core::ptr::drop_in_place(&mut
(*inner).typ)) and core::ptr::write(&mut (*inner).typ, typ) (or equivalently
move typ into (*inner).typ) before leaving the unsafe block, then do not call
drop(typ) afterwards; this ensures PyInner<T>::typ is updated when reusing
freelist objects while preserving the existing payload/vtable handling.

} else {
let inner = Box::into_raw(PyInner::new(payload, typ, dict));
unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
};

// Track object if:
// - HAS_TRAVERSE is true (Rust payload implements Traverse), OR
Expand Down
Loading
Loading