Merge #3176

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 <adam.reichold@t-online.de>

newsfragments/3176.fixed.md

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

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/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(feature = "macros")]
+use pyo3::prelude::*;
+
 #[macro_export]
 macro_rules! py_assert {
     ($py:expr, $($val:ident)+, $assertion:literal) => {
@@ -41,3 +44,49 @@ macro_rules! py_expect_exception {
         err
     }};
 }
+
+#[cfg(feature = "macros")]
+#[pyclass]
+pub struct UnraisableCapture {
+    pub capture: Option<(PyErr, PyObject)>,
+    old_hook: Option<PyObject>,
+}
+
+#[cfg(feature = "macros")]
+#[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(feature = "macros")]
+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);
     });
 }
 

tests/test_class_basics.rs

@@ -228,31 +228,40 @@ impl UnsendableChild {
 }
 
 fn test_unsendable<T: PyClass + 'static>() -> PyResult<()> {
-    let obj = std::thread::spawn(|| -> PyResult<_> {
+    let obj = Python::with_gil(|py| -> PyResult<_> {
+        let obj: Py<T> = PyType::new::<T>(py).call1((5,))?.extract()?;
+
+        // Accessing the value inside this thread should not panic
+        let caught_panic =
+            std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| -> PyResult<_> {
+                assert_eq!(obj.as_ref(py).getattr("value")?.extract::<usize>()?, 5);
+                Ok(())
+            }))
+            .is_err();
+
+        assert!(!caught_panic);
+        Ok(obj)
+    })?;
+
+    let keep_obj_here = obj.clone();
+
+    let caught_panic = std::thread::spawn(move || {
+        // This access must panic
         Python::with_gil(|py| {
-            let obj: Py<T> = PyType::new::<T>(py).call1((5,))?.extract()?;
-
-            // Accessing the value inside this thread should not panic
-            let caught_panic =
-                std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| -> PyResult<_> {
-                    assert_eq!(obj.as_ref(py).getattr("value")?.extract::<usize>()?, 5);
-                    Ok(())
-                }))
-                .is_err();
-
-            assert!(!caught_panic);
-            Ok(obj)
-        })
+            obj.borrow(py);
+        });
     })
-    .join()
-    .unwrap()?;
+    .join();
 
-    // This access must panic
-    Python::with_gil(|py| {
-        obj.borrow(py);
-    });
+    Python::with_gil(|_py| drop(keep_obj_here));
 
-    panic!("Borrowing unsendable from receiving thread did not panic.");
+    if let Err(err) = caught_panic {
+        if let Some(msg) = err.downcast_ref::<String>() {
+            panic!("{}", msg);
+        }
+    }
+
+    Ok(())
 }
 
 /// If a class is marked as `unsendable`, it panics when accessed by another thread.
@@ -526,3 +535,56 @@ fn access_frozen_class_without_gil() {
 
     assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1);
 }
+
+#[test]
+fn drop_unsendable_elsewhere() {
+    use common::UnraisableCapture;
+    use std::sync::{
+        atomic::{AtomicBool, Ordering},
+        Arc,
+    };
+    use std::thread::spawn;
+
+    #[pyclass(unsendable)]
+    struct Unsendable {
+        dropped: Arc<AtomicBool>,
+    }
+
+    impl Drop for Unsendable {
+        fn drop(&mut self) {
+            self.dropped.store(true, Ordering::SeqCst);
+        }
+    }
+
+    Python::with_gil(|py| {
+        let capture = UnraisableCapture::install(py);
+
+        let dropped = Arc::new(AtomicBool::new(false));
+
+        let unsendable = Py::new(
+            py,
+            Unsendable {
+                dropped: dropped.clone(),
+            },
+        )
+        .unwrap();
+
+        py.allow_threads(|| {
+            spawn(move || {
+                Python::with_gil(move |_py| {
+                    drop(unsendable);
+                });
+            })
+            .join()
+            .unwrap();
+        });
+
+        assert!(!dropped.load(Ordering::SeqCst));
+
+        let (err, object) = capture.borrow_mut(py).capture.take().unwrap();
+        assert_eq!(err.to_string(), "RuntimeError: test_class_basics::drop_unsendable_elsewhere::Unsendable is unsendbale, but is dropped on another thread!");
+        assert!(object.is_none(py));
+
+        capture.borrow_mut(py).uninstall(py);
+    });
+}

tests/test_exceptions.rs

@@ -100,29 +100,11 @@ fn test_exception_nosegfault() {
 #[test]
 #[cfg(Py_3_8)]
 fn test_write_unraisable() {
+    use common::UnraisableCapture;
     use pyo3::{exceptions::PyRuntimeError, ffi, AsPyPointer};
 
-    #[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);
 
         assert!(capture.borrow(py).capture.is_none());
 
@@ -140,6 +122,6 @@ fn test_write_unraisable() {
         assert_eq!(err.to_string(), "RuntimeError: bar");
         assert!(object.as_ptr() == unsafe { ffi::Py_NotImplemented() });
 
-        sys.setattr("unraisablehook", old_hook).unwrap();
+        capture.borrow_mut(py).uninstall(py);
     });
 }