Merge #3168 #3176

3168: Do not apply deferred ref count updates and prevent the GIL from being acquired inside of __traverse__ implementations. r=davidhewitt a=adamreichold

Closes #2301
Closes #3165


3176: Prevent dropping unsendable classes on other threads. r=davidhewitt a=adamreichold

Continuing the discussed from #3169 (comment) and #3169 (comment):

We already have checks in place to avoid borrowing these classes on other threads but it was still possible to send them to another thread and drop them there (while holding the GIL).
    
This change avoids running the `Drop` implementation in such a case even though Python will still free the underlying memory. This might leak resources owned by the object, but it avoids undefined behaviour due to access the unsendable type from another thread.
    
This does assume that the object was not unsafely integrated into an intrusive data structures which still point to the now freed memory. In that case, the only recourse would be to abort the process as freeing the memory is unavoidable when the tp_dealloc slot is called. (And moving it elsewhere into a new allocation would still break any existing pointers.)

Co-authored-by: Adam Reichold <[email protected]>

guide/src/class/protocols.md

@@ -407,6 +407,10 @@ impl ClassWithGCSupport {
 }
 ```
 
+Usually, an implementation of `__traverse__` should do nothing but calls to `visit.call`.
+Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`,
+i.e. `Python::with_gil` will panic.
+
 > Note: these methods are part of the C API, PyPy does not necessarily honor them. If you are building for PyPy you should measure memory consumption to make sure you do not have runaway memory growth. See [this issue on the PyPy bug tracker](https://foss.heptapod.net/pypy/pypy/-/issues/3899).
 
 [`IterNextOutput`]: {{#PYO3_DOCS_URL}}/pyo3/pyclass/enum.IterNextOutput.html

newsfragments/3168.changed.md

@@ -0,0 +1 @@
+Safe access to the GIL, for example via `Python::with_gil`, is now locked inside of implementations of the `__traverse__` slot.

newsfragments/3168.fixed.md

@@ -0,0 +1 @@
+Do not apply deferred reference count updates when entering a `__traverse__` implementation is it cannot alter any reference counts while the garbage collector is running.

newsfragments/3176.fixed.md

@@ -0,0 +1 @@
+Avoid running the `Drop` implementations of unsendable classes on other threads

pyo3-macros-backend/src/pymethod.rs

@@ -406,22 +406,8 @@ fn impl_traverse_slot(cls: &syn::Type, rust_fn_ident: &syn::Ident) -> MethodAndS
             slf: *mut _pyo3::ffi::PyObject,
             visit: _pyo3::ffi::visitproc,
             arg: *mut ::std::os::raw::c_void,
-        ) -> ::std::os::raw::c_int
-        {
-            let trap = _pyo3::impl_::panic::PanicTrap::new("uncaught panic inside __traverse__ handler");
-            let pool = _pyo3::GILPool::new();
-            let py = pool.python();
-            let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf);
-
-            let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
-            let borrow = slf.try_borrow();
-            let retval = if let ::std::result::Result::Ok(borrow) = borrow {
-                _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#rust_fn_ident(visit))
-            } else {
-                0
-            };
-            trap.disarm();
-            retval
+        ) -> ::std::os::raw::c_int {
+            _pyo3::impl_::pymethods::call_traverse_impl::<#cls>(slf, #cls::#rust_fn_ident, visit, arg)
         }
     };
     let slot_def = quote! {

src/gil.rs

@@ -29,12 +29,17 @@ thread_local_const_init! {
     /// they are dropped.
     ///
     /// As a result, if this thread has the GIL, GIL_COUNT is greater than zero.
-    static GIL_COUNT: Cell<usize> = const { Cell::new(0) };
+    ///
+    /// Additionally, we sometimes need to prevent safe access to the GIL,
+    /// e.g. when implementing `__traverse__`, which is represented by a negative value.
+    static GIL_COUNT: Cell<isize> = const { Cell::new(0) };
 
     /// Temporarily hold objects that will be released when the GILPool drops.
     static OWNED_OBJECTS: RefCell<Vec<NonNull<ffi::PyObject>>> = const { RefCell::new(Vec::new()) };
 }
 
+const GIL_LOCKED_DURING_TRAVERSE: isize = -1;
+
 /// Checks whether the GIL is acquired.
 ///
 /// Note: This uses pyo3's internal count rather than PyGILState_Check for two reasons:
@@ -286,7 +291,7 @@ static POOL: ReferencePool = ReferencePool::new();
 
 /// A guard which can be used to temporarily release the GIL and restore on `Drop`.
 pub(crate) struct SuspendGIL {
-    count: usize,
+    count: isize,
     tstate: *mut ffi::PyThreadState,
 }
 
@@ -311,6 +316,40 @@ impl Drop for SuspendGIL {
     }
 }
 
+/// Used to lock safe access to the GIL
+pub(crate) struct LockGIL {
+    count: isize,
+}
+
+impl LockGIL {
+    /// Lock access to the GIL while an implementation of `__traverse__` is running
+    pub fn during_traverse() -> Self {
+        Self::new(GIL_LOCKED_DURING_TRAVERSE)
+    }
+
+    fn new(reason: isize) -> Self {
+        let count = GIL_COUNT.with(|c| c.replace(reason));
+
+        Self { count }
+    }
+
+    #[cold]
+    fn bail(current: isize) {
+        match current {
+            GIL_LOCKED_DURING_TRAVERSE => panic!(
+                "Access to the GIL is prohibited while a __traverse__ implmentation is running."
+            ),
+            _ => panic!("Access to the GIL is currently prohibited."),
+        }
+    }
+}
+
+impl Drop for LockGIL {
+    fn drop(&mut self) {
+        GIL_COUNT.with(|c| c.set(self.count));
+    }
+}
+
 /// A RAII pool which PyO3 uses to store owned Python references.
 ///
 /// See the [Memory Management] chapter of the guide for more information about how PyO3 uses
@@ -421,7 +460,13 @@ pub unsafe fn register_owned(_py: Python<'_>, obj: NonNull<ffi::PyObject>) {
 #[inline(always)]
 fn increment_gil_count() {
     // Ignores the error in case this function called from `atexit`.
-    let _ = GIL_COUNT.try_with(|c| c.set(c.get() + 1));
+    let _ = GIL_COUNT.try_with(|c| {
+        let current = c.get();
+        if current < 0 {
+            LockGIL::bail(current);
+        }
+        c.set(current + 1);
+    });
 }
 
 /// Decrements pyo3's internal GIL count - to be called whenever GILPool or GILGuard is dropped.

src/impl_/pyclass.rs

@@ -1,5 +1,5 @@
 use crate::{
-    exceptions::{PyAttributeError, PyNotImplementedError, PyValueError},
+    exceptions::{PyAttributeError, PyNotImplementedError, PyRuntimeError, PyValueError},
     ffi,
     impl_::freelist::FreeList,
     impl_::pycell::{GetBorrowChecker, PyClassMutability},
@@ -884,6 +884,7 @@ impl<T> PyClassNewTextSignature<T> for &'_ PyClassImplCollector<T> {
 #[doc(hidden)]
 pub trait PyClassThreadChecker<T>: Sized {
     fn ensure(&self);
+    fn can_drop(&self, py: Python<'_>) -> bool;
     fn new() -> Self;
     private_decl! {}
 }
@@ -894,6 +895,9 @@ pub struct ThreadCheckerStub<T: Send>(PhantomData<T>);
 
 impl<T: Send> PyClassThreadChecker<T> for ThreadCheckerStub<T> {
     fn ensure(&self) {}
+    fn can_drop(&self, _py: Python<'_>) -> bool {
+        true
+    }
     #[inline]
     fn new() -> Self {
         ThreadCheckerStub(PhantomData)
@@ -903,6 +907,9 @@ impl<T: Send> PyClassThreadChecker<T> for ThreadCheckerStub<T> {
 
 impl<T: PyNativeType> PyClassThreadChecker<T> for ThreadCheckerStub<crate::PyObject> {
     fn ensure(&self) {}
+    fn can_drop(&self, _py: Python<'_>) -> bool {
+        true
+    }
     #[inline]
     fn new() -> Self {
         ThreadCheckerStub(PhantomData)
@@ -924,6 +931,18 @@ impl<T> PyClassThreadChecker<T> for ThreadCheckerImpl<T> {
             std::any::type_name::<T>()
         );
     }
+    fn can_drop(&self, py: Python<'_>) -> bool {
+        if thread::current().id() != self.0 {
+            PyRuntimeError::new_err(format!(
+                "{} is unsendbale, but is dropped on another thread!",
+                std::any::type_name::<T>()
+            ))
+            .write_unraisable(py, None);
+            return false;
+        }
+
+        true
+    }
     fn new() -> Self {
         ThreadCheckerImpl(thread::current().id(), PhantomData)
     }
@@ -944,6 +963,9 @@ impl<T: PyClass + Send, U: PyClassBaseType> PyClassThreadChecker<T>
     fn ensure(&self) {
         self.1.ensure();
     }
+    fn can_drop(&self, py: Python<'_>) -> bool {
+        self.1.can_drop(py)
+    }
     fn new() -> Self {
         ThreadCheckerInherited(PhantomData, U::ThreadChecker::new())
     }

src/impl_/pymethods.rs

@@ -1,9 +1,15 @@
+use crate::gil::LockGIL;
+use crate::impl_::panic::PanicTrap;
 use crate::internal_tricks::extract_c_string;
-use crate::{ffi, IntoPy, Py, PyAny, PyErr, PyObject, PyResult, PyTraverseError, Python};
+use crate::{
+    ffi, IntoPy, Py, PyAny, PyCell, PyClass, PyErr, PyObject, PyResult, PyTraverseError, PyVisit,
+    Python,
+};
 use std::borrow::Cow;
 use std::ffi::CStr;
 use std::fmt;
-use std::os::raw::c_int;
+use std::os::raw::{c_int, c_void};
+use std::panic::{catch_unwind, AssertUnwindSafe};
 
 /// Python 3.8 and up - __ipow__ has modulo argument correctly populated.
 #[cfg(Py_3_8)]
@@ -239,14 +245,46 @@ impl PySetterDef {
     }
 }
 
-/// Unwraps the result of __traverse__ for tp_traverse
+/// Calls an implementation of __traverse__ for tp_traverse
 #[doc(hidden)]
-#[inline]
-pub fn unwrap_traverse_result(result: Result<(), PyTraverseError>) -> c_int {
-    match result {
-        Ok(()) => 0,
-        Err(PyTraverseError(value)) => value,
-    }
+pub unsafe fn call_traverse_impl<T>(
+    slf: *mut ffi::PyObject,
+    impl_: fn(&T, PyVisit<'_>) -> Result<(), PyTraverseError>,
+    visit: ffi::visitproc,
+    arg: *mut c_void,
+) -> c_int
+where
+    T: PyClass,
+{
+    // It is important the implementation of `__traverse__` cannot safely access the GIL,
+    // c.f. https://github.com/PyO3/pyo3/issues/3165, and hence we do not expose our GIL
+    // token to the user code and lock safe methods for acquiring the GIL.
+    // (This includes enforcing the `&self` method receiver as e.g. `PyRef<Self>` could
+    // reconstruct a GIL token via `PyRef::py`.)
+    // Since we do not create a `GILPool` at all, it is important that our usage of the GIL
+    // token does not produce any owned objects thereby calling into `register_owned`.
+    let trap = PanicTrap::new("uncaught panic inside __traverse__ handler");
+
+    let py = Python::assume_gil_acquired();
+    let slf = py.from_borrowed_ptr::<PyCell<T>>(slf);
+    let borrow = slf.try_borrow();
+    let visit = PyVisit::from_raw(visit, arg, py);
+
+    let retval = if let Ok(borrow) = borrow {
+        let _lock = LockGIL::during_traverse();
+
+        match catch_unwind(AssertUnwindSafe(move || impl_(&*borrow, visit))) {
+            Ok(res) => match res {
+                Ok(()) => 0,
+                Err(PyTraverseError(value)) => value,
+            },
+            Err(_err) => -1,
+        }
+    } else {
+        0
+    };
+    trap.disarm();
+    retval
 }
 
 pub(crate) struct PyMethodDefDestructor {

src/pycell.rs

@@ -937,7 +937,9 @@ where
     unsafe fn tp_dealloc(py: Python<'_>, slf: *mut ffi::PyObject) {
         // Safety: Python only calls tp_dealloc when no references to the object remain.
         let cell = &mut *(slf as *mut PyCell<T>);
-        ManuallyDrop::drop(&mut cell.contents.value);
+        if cell.contents.thread_checker.can_drop(py) {
+            ManuallyDrop::drop(&mut cell.contents.value);
+        }
         cell.contents.dict.clear_dict(py);
         cell.contents.weakref.clear_weakrefs(slf, py);
         <T::BaseType as PyClassBaseType>::LayoutAsBase::tp_dealloc(py, slf)

tests/common.rs

@@ -1,5 +1,8 @@
 //! Some common macros for tests
 
+#[cfg(all(feature = "macros", Py_3_8))]
+use pyo3::prelude::*;
+
 #[macro_export]
 macro_rules! py_assert {
     ($py:expr, $($val:ident)+, $assertion:literal) => {
@@ -41,3 +44,50 @@ macro_rules! py_expect_exception {
         err
     }};
 }
+
+// sys.unraisablehook not available until Python 3.8
+#[cfg(all(feature = "macros", Py_3_8))]
+#[pyclass]
+pub struct UnraisableCapture {
+    pub capture: Option<(PyErr, PyObject)>,
+    old_hook: Option<PyObject>,
+}
+
+#[cfg(all(feature = "macros", Py_3_8))]
+#[pymethods]
+impl UnraisableCapture {
+    pub fn hook(&mut self, unraisable: &PyAny) {
+        let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap());
+        let instance = unraisable.getattr("object").unwrap();
+        self.capture = Some((err, instance.into()));
+    }
+}
+
+#[cfg(all(feature = "macros", Py_3_8))]
+impl UnraisableCapture {
+    pub fn install(py: Python<'_>) -> Py<Self> {
+        let sys = py.import("sys").unwrap();
+        let old_hook = sys.getattr("unraisablehook").unwrap().into();
+
+        let capture = Py::new(
+            py,
+            UnraisableCapture {
+                capture: None,
+                old_hook: Some(old_hook),
+            },
+        )
+        .unwrap();
+
+        sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap())
+            .unwrap();
+
+        capture
+    }
+
+    pub fn uninstall(&mut self, py: Python<'_>) {
+        let old_hook = self.old_hook.take().unwrap();
+
+        let sys = py.import("sys").unwrap();
+        sys.setattr("unraisablehook", old_hook).unwrap();
+    }
+}

tests/test_buffer_protocol.rs

@@ -96,6 +96,7 @@ fn test_buffer_referenced() {
 #[test]
 #[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8
 fn test_releasebuffer_unraisable_error() {
+    use common::UnraisableCapture;
     use pyo3::exceptions::PyValueError;
 
     #[pyclass]
@@ -117,27 +118,8 @@ fn test_releasebuffer_unraisable_error() {
         }
     }
 
-    #[pyclass]
-    struct UnraisableCapture {
-        capture: Option<(PyErr, PyObject)>,
-    }
-
-    #[pymethods]
-    impl UnraisableCapture {
-        fn hook(&mut self, unraisable: &PyAny) {
-            let err = PyErr::from_value(unraisable.getattr("exc_value").unwrap());
-            let instance = unraisable.getattr("object").unwrap();
-            self.capture = Some((err, instance.into()));
-        }
-    }
-
     Python::with_gil(|py| {
-        let sys = py.import("sys").unwrap();
-        let old_hook = sys.getattr("unraisablehook").unwrap();
-        let capture = Py::new(py, UnraisableCapture { capture: None }).unwrap();
-
-        sys.setattr("unraisablehook", capture.getattr(py, "hook").unwrap())
-            .unwrap();
+        let capture = UnraisableCapture::install(py);
 
         let instance = Py::new(py, ReleaseBufferError {}).unwrap();
         let env = [("ob", instance.clone())].into_py_dict(py);
@@ -150,7 +132,7 @@ fn test_releasebuffer_unraisable_error() {
         assert_eq!(err.to_string(), "ValueError: oh dear");
         assert!(object.is(&instance));
 
-        sys.setattr("unraisablehook", old_hook).unwrap();
+        capture.borrow_mut(py).uninstall(py);
     });
 }