Fix __dict__ leak on Python 3.11 and 3.12#6234
Conversation
| // zeroes (a valid empty slot value) or a valid owned pointer stored by the base | ||
| // `tp_new` through the type's `tp_dictoffset`. | ||
| new_contents.dict = | ||
| unsafe { core::ptr::read(&raw const (*(*contents).as_mut_ptr()).dict) }; |
There was a problem hiding this comment.
Maybe this will be easier to read?
#[cfg(not(Py_3_13))]
let new_contents = {
let mut new_contents = new_contents;
// SAFETY: `tp_alloc` zero-initializes the object, so the slot contains either
// zeroes (a valid empty slot value) or a valid owned pointer stored by the base
// `tp_new` through the type's `tp_dictoffset`.
- new_contents.dict =
- unsafe { core::ptr::read(&raw const (*(*contents).as_mut_ptr()).dict) };
+ let contents_ptr = (*contents).as_mut_ptr();
+ let dict_ptr = core::ptr::addr_of!((*contents_ptr).dict);
+ new_contents.dict = unsafe { core::ptr::read(dict_ptr) };
new_contents
};
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, so many fixes for #[pyclass(dict)] all of a sudden!
davidhewitt
left a comment
There was a problem hiding this comment.
Actually just a couple of small comments, sorry.
| #[cfg(not(Py_3_13))] | ||
| let new_contents = { |
There was a problem hiding this comment.
For abi3 builds should we also consider doing a runtime gate? e.g. otherwise a 3.12 abi3 build still does this twiddling on 3.13+. It's probably harmless to have it not gated, but might be good to be thorough. (See bpo_45315_workaround as an example.)
Also I wonder if this can somehow be skipped for cases when T::Dict is PyClassDummySlot (i.e. no dict).
There was a problem hiding this comment.
Added a eagerly_created_dict_possible helper that does the limited API runtime python version check and and the check for a PyClassDummySlot.
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, one final suggestion particularly to cover the < 3.11 cases, and then let's merge.
| #[cfg(Py_LIMITED_API)] | ||
| { | ||
| use crate::sync::PyOnceLock; | ||
| static IS_PYTHON_3_13: PyOnceLock<bool> = PyOnceLock::new(); | ||
| !*IS_PYTHON_3_13.get_or_init(py, || py.version_info() >= (3, 13)) | ||
| } | ||
| #[cfg(not(Py_LIMITED_API))] | ||
| { | ||
| let _ = py; | ||
| true | ||
| } |
There was a problem hiding this comment.
Slight preference to use cfg_select! here for newer conditional code. Also now that we're differentiating abi3 and non-abi3 pathways we can more properly account for the < 3.11 cases, maybe as the following:
| #[cfg(Py_LIMITED_API)] | |
| { | |
| use crate::sync::PyOnceLock; | |
| static IS_PYTHON_3_13: PyOnceLock<bool> = PyOnceLock::new(); | |
| !*IS_PYTHON_3_13.get_or_init(py, || py.version_info() >= (3, 13)) | |
| } | |
| #[cfg(not(Py_LIMITED_API))] | |
| { | |
| let _ = py; | |
| true | |
| } | |
| cfg_select! { | |
| Py_LIMITED_API => { | |
| use crate::sync::PyOnceLock; | |
| static IS_PYTHON_3_11_OR_3_12: PyOnceLock<bool> = PyOnceLock::new(); | |
| IS_PYTHON_3_11_OR_3_12.get_or_init(py, || { | |
| let version_info = py.version_info(); | |
| version_info >= (3, 11) && version_info < (3, 13) | |
| }) | |
| }, | |
| not(Py_LIMITED_API) => { | |
| let _ = py; | |
| cfg!(Py_3_11) | |
| } | |
| } |
On CPython 3.11 and 3.12, every instance of a
#[pyclass(dict)]class leaks one empty dict object at instance creation. This is separate from #5953 / #6198, which fixes the leak of the dict reference held in the slot at deallocation: with #6198 applied,#[pyclass(dict)]classes still leak one dict per instance on 3.11/3.12.The other reference on 3.11 and 3.12 is due to Python eagerly instantiating
__dict__at class creation, which we clobber during pyclass setup. The eager instantiation was removed in Python 3.13.Marking as a draft for now because the test requires #6198.
I found this with an AI model reviewing #6198.