fix crash with GC occurring during class attribute initialization

[davidhewitt]

Fixes #6390

This fixes a horrible bug lurking in the abi3t code paths for PyVariableClassObject. The crash formed as follows:

• When #[classattr] values are being created in #[pyclass] type object dicts, the arbitrary code execution can lead the interpreter to detach from the thread
• This can potentially cause other threads to begin to access the partially-initialized type object, such as during GC
• (The root cause bug) is that PyVariableClassObject would then use T::type_object_raw to look up #[pyclass] type, initializing it if needed (which can be concurrent with the original thread, see also Disallow races in initializing types #5211)
  • ... and if this look up occurs during GC traversal, we can't execute Python code, so boom.

I've ended up having to add several layers to this fix:

• We need to add _during_gc variants to several APIs to avoid possibly entering T::type_object_raw (which is only safe to call when attached to the interpreter
• This required introducing a PyClassTraverseGuard type, similar to PyClassGuard, but which avoids using T::type_object_raw and only goes via gc-safe pathways
• For Python 3.15+ I use the new _DuringGC APIs to properly look up type data during the GC pass
  • This requires use of Py_tp_token slot, so I had to populate that in create_type_object with help of the #[pyclass] macro

I started passing a lot of NonNull<ffi::PyObject> all over the place while implementing these functions, but that felt pretty painful especially with borrowed data flying around. In the end, I also introduced a crate-private PyBorrowedUnbound<'a, T> helper, basically Borrowed<'a, 'py, T> but without attachment to the interpreter. This helped me write "safer" functions by requiring objects of the right type and connecting input & output lifetimes, (e.g. on T::Layout::contents_during_gc)

Some notes:

• I think I might want to add a DuringGc ZST to pass around to these _during_gc functions - I think otherwise they are typically unsafe to call because they do things like borrowed references, and I don't think I've marked them all properly.
• I 100% needed AI to figure out the interaction causing the crash and it wrote me the test case; from that point forward the fix is me just going loose on the codebase 😂
• We could probably split the tp_token based lookup into a separate PR and just have a more targeted fix related to the GC soundness, but I started with the tp_token stuff as there was a TODO literally inside PyVariableClassObject::get_contents_of_obj right where the GC traversal was entering T::type_object_raw. We need this machinery anyway to eventually support module isolation / subinterpreters.

[codspeed-hq]

Merging this PR will degrade performance by 12.78%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 3 regressed benchmarks
✅ 138 untouched benchmarks
⏩ 6 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	into_u128_small	876.1 ns	1,041.4 ns	-15.87%
❌	into_u128_zero	816 ns	925.7 ns	-11.85%
❌	into_i128_small_pos	932.8 ns	1,042.5 ns	-10.52%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing davidhewitt:py3.15rc2-macos (51dd216) with main (ee629fb)}

Footnotes

1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[ngoldbaum]

Maybe this is the simpler version you were getting at? What about:

diff --git a/pyo3-ffi/src/object.rs b/pyo3-ffi/src/object.rs
index ee1508e97..f7befb936 100644
--- a/pyo3-ffi/src/object.rs
+++ b/pyo3-ffi/src/object.rs
@@ -392,6 +392,9 @@ extern_libpython! {
     #[cfg(Py_3_12)]
     pub fn PyObject_GetTypeData(obj: *mut PyObject, cls: *mut PyTypeObject) -> *mut c_void;
 
+    #[cfg(Py_3_15)]
+    pub fn PyObject_GetTypeData_DuringGC(obj: *mut PyObject, cls: *mut PyTypeObject) -> *mut c_void;
+
     #[cfg(Py_3_12)]
     #[cfg_attr(PyPy, link_name = "PyPyType_GetTypeDataSize")]
     pub fn PyType_GetTypeDataSize(cls: *mut PyTypeObject) -> Py_ssize_t;
diff --git a/src/impl_/pyclass/lazy_type_object.rs b/src/impl_/pyclass/lazy_type_object.rs
index 22b083fe6..88c6b4b32 100644
--- a/src/impl_/pyclass/lazy_type_object.rs
+++ b/src/impl_/pyclass/lazy_type_object.rs
@@ -56,6 +56,18 @@ impl<T> LazyTypeObject<T> {
 }
 
 impl<T: PyClass> LazyTypeObject<T> {
+    /// Gets an already-created type without initializing its class attributes.
+    /// This can also be used during GC traversal, without interpreter attachment.
+    #[cfg(Py_3_12)]
+    pub(crate) fn get_cached(&self) -> &Py<PyType> {
+        &self
+            .0
+            .value
+            .get_unbound()
+            .expect("the type must have been created before accessing an instance")
+            .type_object
+    }
+
     /// Gets the type object contained by this `LazyTypeObject`, initializing it if needed.
     #[inline]
     pub fn get_or_try_init<'py>(&self, py: Python<'py>) -> PyResult<&Bound<'py, PyType>> {
diff --git a/src/internal/state.rs b/src/internal/state.rs
index 44acc9edf..2f43cc815 100644
--- a/src/internal/state.rs
+++ b/src/internal/state.rs
@@ -39,6 +39,15 @@ pub(crate) fn thread_is_attached() -> bool {
     ATTACH_COUNT.try_with(|c| c.get() > 0).unwrap_or(false)
 }
 
+/// Checks whether PyO3 is currently executing a `tp_traverse` handler.
+#[cfg(Py_3_15)]
+#[inline]
+pub(crate) fn thread_is_traversing() -> bool {
+    ATTACH_COUNT
+        .try_with(|c| c.get() == ATTACH_FORBIDDEN_DURING_TRAVERSE)
+        .unwrap_or(false)
+}
+
 /// RAII type that represents thread attachment to the interpreter.
 pub(crate) enum AttachGuard {
     /// Indicates the thread was already attached when this AttachGuard was acquired.
diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs
index 59b4e2fa1..7bc823e91 100644
--- a/src/pycell/impl_.rs
+++ b/src/pycell/impl_.rs
@@ -523,14 +523,22 @@ pub struct PyVariableClassObject<T: PyClassImpl> {
 impl<T: PyClass<Layout = Self>> PyVariableClassObject<T> {
     /// # Safety
     /// - `obj` must have the layout that the implementation is expecting
-    /// - thread must be attached to the interpreter
+    /// - the type object for `T` must already have been created
     unsafe fn get_contents_of_obj(
         obj: *mut ffi::PyObject,
     ) -> *mut MaybeUninit<PyClassObjectContents<T>> {
-        // TODO: it would be nice to eventually avoid coupling to the PyO3 statics here, maybe using
-        // 3.14's PyType_GetBaseByToken, to support PEP 587 / multiple interpreters better
-        // SAFETY: caller guarantees attached to the interpreter
-        let type_obj = T::type_object_raw(unsafe { Python::assume_attached() });
+        // An instance can be traversed while its class attributes are still being initialized
+        // on another thread. Reading its layout must not run that initialization again.
+        let type_obj = T::lazy_type_object().get_cached().as_ptr().cast();
+
+        #[cfg(Py_3_15)]
+        if crate::internal::state::thread_is_traversing() {
+            // SAFETY: `obj` and its cached type are valid, and the traversal guard is active.
+            return unsafe { ffi::PyObject_GetTypeData_DuringGC(obj, type_obj).cast() };
+        }
+
+        // SAFETY: `obj` and its cached type are valid. Before Python 3.15, this API is also
+        // used during traversal: it only computes the address of the instance's type data.
         let pointer = unsafe { ffi::PyObject_GetTypeData(obj, type_obj) };
         pointer.cast()
     }
diff --git a/src/sync.rs b/src/sync.rs
index 1b4a002e9..58036df02 100644
--- a/src/sync.rs
+++ b/src/sync.rs
@@ -114,6 +114,12 @@ impl<T> GILOnceCell<T> {
     /// Get a reference to the contained value, or `None` if the cell has not yet been written.
     #[inline]
     pub fn get(&self, _py: Python<'_>) -> Option<&T> {
+        self.get_unbound()
+    }
+
+    /// Reads the cell without initializing it or requiring interpreter attachment.
+    #[inline]
+    pub(crate) fn get_unbound(&self) -> Option<&T> {
         if self.once.is_completed() {
             // SAFETY: the cell has been written.
             Some(unsafe { (*self.data.get()).assume_init_ref() })

This seems to also make the test you added pass. Instead of forcing realization of the type, this adds an API to get data in a manner that is safe to do during GC. It also uses the DuringGC C API to get type data.

[davidhewitt]

Yes, something approximately like that.

I think I'd prefer not introduce the runtime GC check, after #5663 is landed and traversal is made unsafe I'd quite like to remove the guard which forbids attaching. That would imply having during_gc variants of functions similar to this PR.

Also we definitely want to eventually kill off the GILOnceCell which this PR and your proposed short patch expands (that is definitely still a follow up for another day).

The token stuff is probably overkill for now, as it'll only be relevant once we properly support reloadable modules. But killing the static type objects will be a much harder problem than just the isolated fix in this PR.

I think there's probably grounds for:

• splitting the FFI definition additions into a precursor PR
• rolling back the tp_token stuff from this PR and deferring that to future module isolation work
• keeping the _during_gc variants added in this PR

... I'll update accordingly in the next few days