Skip to content

Fix memory leaks during interpreter and thread teardown #8966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
VladimirKras wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix memory leaks during interpreter and thread teardown #8966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/complex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ thread_local! {
impl PyPayload for PyComplex {
const MAX_FREELIST: usize = 100;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -53,7 +54,7 @@ impl PyPayload for PyComplex {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
9 changes: 8 additions & 1 deletion crates/vm/src/builtins/descriptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ impl PyMemberSpec {
}

// = PyMemberDescrObject
#[pyclass(name = "member_descriptor", module = false)]
#[pyclass(name = "member_descriptor", module = false, traverse = "manual")]
#[derive(Debug)]
pub struct PyMemberDescriptor {
#[pymember(name = "__objclass__", path = "typ")]
Expand All @@ -746,6 +746,13 @@ pub struct PyMemberDescriptor {
pub access: MemberAccess,
}

// SAFETY: common.typ is owned; the other fields contain no owned Python objects.
unsafe impl crate::object::Traverse for PyMemberDescriptor {
fn traverse(&self, tracer_fn: &mut crate::object::TraverseFn<'_>) {
tracer_fn(self.common.typ.as_object());
}
}

impl PyMemberDescriptor {
/// Byte offset of an object-pointer member. `None` for bool, float, and
/// struct-sequence indexes, which slot specialization must not treat as cells.
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/dict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ thread_local! {
impl PyPayload for PyDict {
const MAX_FREELIST: usize = 80;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -81,7 +82,7 @@ impl PyPayload for PyDict {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/float.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ thread_local! {
impl PyPayload for PyFloat {
const MAX_FREELIST: usize = 100;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -60,7 +61,7 @@ impl PyPayload for PyFloat {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
2 changes: 2 additions & 0 deletions crates/vm/src/builtins/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ fn next_func_version() -> u32 {
unsafe impl Traverse for PyFunction {
fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
self.globals.traverse(tracer_fn);
// Both fields own a reference, even when builtins aliases globals.
self.builtins.traverse(tracer_fn);
if let Some(closure) = self.closure.as_ref() {
// Visit the closure tuple itself as an edge, not its cells: the
// tuple is a tracked object that can join a reference cycle, and
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/int.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ thread_local! {
impl PyPayload for PyInt {
const MAX_FREELIST: usize = 100;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -74,7 +75,7 @@ impl PyPayload for PyInt {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ thread_local! {
impl PyPayload for PyList {
const MAX_FREELIST: usize = 80;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -102,7 +103,7 @@ impl PyPayload for PyList {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ thread_local! {
impl PyPayload for PyRange {
const MAX_FREELIST: usize = 6;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -92,7 +93,7 @@ impl PyPayload for PyRange {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
5 changes: 3 additions & 2 deletions crates/vm/src/builtins/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ unsafe impl crate::object::Traverse for PySlice {
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)
// (before freelist caching, or during deallocation)
if let Some(step) = self.step.take() {
out.push(step);
}
Expand All @@ -54,6 +54,7 @@ thread_local! {
impl PyPayload for PySlice {
const MAX_FREELIST: usize = 1;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -66,7 +67,7 @@ impl PyPayload for PySlice {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/builtins/tuple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ thread_local! {
impl PyPayload for PyTuple {
const MAX_FREELIST: usize = 2000;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;

#[inline]
fn class(ctx: &Context) -> &'static Py<PyType> {
Expand All @@ -130,7 +131,7 @@ impl PyPayload for PyTuple {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,6 +1615,7 @@ thread_local! {
impl PyPayload for FrameObject {
const MAX_FREELIST: usize = 200;
const HAS_FREELIST: bool = true;
const FREELIST_HAS_PAYLOAD: bool = false;
// Ordinary call frames are created untracked and only enter the GC when
// they escape (see `release_datastack_frame`); generator/coroutine frames
// are tracked explicitly at creation in `invoke_with_locals`.
Expand All @@ -1631,7 +1632,7 @@ impl PyPayload for FrameObject {
.try_with(|fl| {
let mut list = fl.take();
let stored = if list.len() < Self::MAX_FREELIST {
list.push(obj);
unsafe { list.push(obj) };
true
} else {
false
Expand Down
75 changes: 54 additions & 21 deletions crates/vm/src/object/core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,7 +1015,7 @@ unsafe impl Link for WeakLink {
}

// PyWeakReference: each weakref holds a direct pointer to its referent.
#[pyclass(name = "ReferenceType", module = "weakref")]
#[pyclass(name = "ReferenceType", module = "weakref", traverse = "manual")]
#[derive(Debug)]
pub struct PyWeak {
pointers: Pointers<Py<Self>>,
Expand All @@ -1035,6 +1035,21 @@ cfg_select! {
_ => {}
}

// SAFETY: the stripe lock protects the owned callback; wr_object is a weak pointer.
unsafe impl Traverse for PyWeak {
fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) {
let obj_ptr = self.wr_object.load(Ordering::Acquire);
if obj_ptr.is_null() {
return;
}
let _lock = weakref_lock::lock(obj_ptr as usize);
if !self.wr_object.load(Ordering::Relaxed).is_null() {
// SAFETY: the callback is read under the same lock used to consume it.
unsafe { (&*self.callback.get()).traverse(tracer_fn) };
}
}
}

impl PyWeak {
/// _PyWeakref_GET_REF: attempt to upgrade the weakref to a strong reference.
pub(crate) fn upgrade(&self) -> Option<PyObjectRef> {
Expand Down Expand Up @@ -1247,9 +1262,11 @@ impl<T: PyPayload> Py<T> {
/// object it points at until its own `Drop` unlinks it, and a thread
/// walking that list reads the class off every node it passes, so the
/// class has to outlive the payload.
unsafe fn drop_fields(ptr: *mut Self) {
unsafe fn drop_fields<const DROP_PAYLOAD: bool>(ptr: *mut Self) {
unsafe {
core::ptr::drop_in_place(&raw mut (*ptr).payload);
if DROP_PAYLOAD {
core::ptr::drop_in_place(&raw mut (*ptr).payload);
}
core::ptr::drop_in_place(&raw mut (*ptr).typ);
}
}
Expand All @@ -1260,8 +1277,15 @@ impl<T: PyPayload> Py<T> {
/// # Safety
/// `ptr` must be a valid pointer from `Py::new` and must not be used after this call.
unsafe fn dealloc(ptr: *mut Self) {
unsafe { Self::dealloc_inner::<true>(ptr) };
}

// Freelist entries have already had their payload destroyed.
unsafe fn dealloc_inner<const DROP_PAYLOAD: bool>(ptr: *mut Self) {
unsafe {
let (flags, member_count) = (*ptr).read_type_flags();
// Read the initialized header, including for cached allocations
// whose payload has already been destroyed.
let (flags, member_count) = (*(ptr as *const PyObject)).0.read_type_flags();
let has_ext = flags.contains(&crate::types::PyTypeFlags::HAS_DICT) || member_count > 0;
let has_weakref = flags.contains(&crate::types::PyTypeFlags::HAS_WEAKREF);
// Objects published to lock-free caches keep their memory mapped
Expand Down Expand Up @@ -1293,7 +1317,7 @@ impl<T: PyPayload> Py<T> {

let alloc_ptr = (ptr as *mut u8).sub(inner_offset);

Self::drop_fields(ptr);
Self::drop_fields::<DROP_PAYLOAD>(ptr);

// Drop member cells, then ObjExt. WeakRefList is in front of the cells.
let mut cursor = alloc_ptr;
Expand Down Expand Up @@ -1321,10 +1345,10 @@ impl<T: PyPayload> Py<T> {
}
} else if published {
let layout = core::alloc::Layout::new::<Self>();
Self::drop_fields(ptr);
Self::drop_fields::<DROP_PAYLOAD>(ptr);
crate::object::qsbr::free_delayed(ptr as *mut u8, layout);
} else {
Self::drop_fields(ptr);
Self::drop_fields::<DROP_PAYLOAD>(ptr);
// The fields are gone; the box is only here to free the memory
// the matching `Box::new` in `new` allocated.
drop(Box::from_raw(ptr.cast::<core::mem::MaybeUninit<Self>>()));
Expand Down Expand Up @@ -1459,15 +1483,28 @@ impl<T: PyPayload + core::fmt::Debug> Py<T> {

/// Thread-local freelist storage for reusing object allocations.
///
/// Wraps a `Vec<*mut PyObject>`. On thread teardown, `Drop` frees raw
/// `Py<T>` allocations without running payload destructors to avoid
/// accessing already-destroyed thread-local storage (GC state, other freelists).
/// Cached allocations retain their header, but not their payload. Payloads
/// are destroyed before caching, while thread-local deallocation state is
/// available. Thread teardown therefore only releases the allocation and its
/// reference to the static base class.
pub(crate) struct FreeList<T: PyPayload> {
items: Vec<*mut PyObject>,
_marker: core::marker::PhantomData<T>,
}

impl<T: PyPayload> FreeList<T> {
/// Cache a dead allocation after destroying its payload.
///
/// # Safety
/// `ptr` must point to an untracked, unpublished, exact base-type `Py<T>`
/// with no outstanding payload borrows. Its payload must be initialized.
pub(crate) unsafe fn push(&mut self, ptr: *mut PyObject) {
debug_assert!(!T::FREELIST_HAS_PAYLOAD);
// Destruction can reenter allocation; publish the husk only afterwards.
unsafe { core::ptr::drop_in_place(&raw mut (*(ptr as *mut Py<T>)).payload) };
self.items.push(ptr);
}

pub(crate) const fn new() -> Self {
Self {
items: Vec::new(),
Expand All @@ -1484,16 +1521,10 @@ impl<T: PyPayload> Default for FreeList<T> {

impl<T: PyPayload> Drop for FreeList<T> {
fn drop(&mut self) {
// During thread teardown, we cannot safely run destructors on cached
// objects because their Drop impls may access thread-local storage
// (GC state, other freelists) that is already destroyed.
// Instead, free just the raw allocation. The payload's heap fields
// (BigInt, PyObjectRef, etc.) are leaked, but this is bounded by
// MAX_FREELIST per type per thread.
for ptr in self.items.drain(..) {
unsafe {
alloc::alloc::dealloc(ptr as *mut u8, core::alloc::Layout::new::<Py<T>>());
}
// The payload is gone and the remaining class reference points to
// a static base type, so no Python destruction can run here.
unsafe { Py::<T>::dealloc_inner::<false>(ptr as *mut Py<T>) };
}
}
}
Expand Down Expand Up @@ -2827,8 +2858,10 @@ impl<T: PyPayload + crate::object::MaybeTraverse + core::fmt::Debug> PyRef<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);
if T::FREELIST_HAS_PAYLOAD {
core::ptr::drop_in_place(&raw mut (*inner).payload);
}
core::ptr::write(&raw mut (*inner).payload, payload);
// Freelist only stores exact base types (push-side filter),
// but subtypes sharing the same Rust payload (e.g. structseq)
// may pop entries. Update typ if it differs.
Expand Down
13 changes: 9 additions & 4 deletions crates/vm/src/object/payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,18 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static {
/// race conditions when the object is reused.
const HAS_FREELIST: bool = false;

/// Whether cached allocations retain an initialized payload.
/// `FreeList<Self>` destroys payloads before caching and requires false.
const FREELIST_HAS_PAYLOAD: bool = true;

/// Maximum number of objects to keep in the freelist.
const MAX_FREELIST: usize = 0;

/// Try to push a dead object onto this type's freelist for reuse.
/// Returns true if the object was stored (caller must NOT free the memory).
/// Called after tp_clear, so the payload is a cleared husk; implementations
/// must not rely on its pre-clear contents.
/// must not rely on its pre-clear contents. If `FREELIST_HAS_PAYLOAD` is
/// false, destroy the payload before publishing the allocation for reuse.
///
/// # Safety
/// `obj` must be a valid pointer to a `Py<Self>` with refcount 0
Expand All @@ -114,13 +119,13 @@ pub trait PyPayload: MaybeTraverse + PyThreadingConstraint + Sized + 'static {
}

/// Try to pop a pre-allocated object from this type's freelist.
/// The returned pointer still has the old payload; the caller must
/// The caller must
/// reinitialize `ref_count`, `gc_bits`, and `payload`.
///
/// # Safety
/// The returned pointer (if Some) must point to a valid `Py<Self>`
/// whose payload is still initialized from a previous allocation. The caller
/// will drop and overwrite `payload` before reuse.
/// whose payload is initialized if and only if `FREELIST_HAS_PAYLOAD` is
/// true. The caller will destroy it if initialized, then write a new payload.
#[inline]
unsafe fn freelist_pop(_payload: &Self) -> Option<NonNull<PyObject>> {
None
Expand Down
Loading
Loading