From 501ff8a17dfa6d426c6f8d8b30ad2e6b20c90863 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Mon, 22 May 2023 09:26:07 +0200 Subject: [PATCH 1/2] Prevent dropping unsendable classes on other threads. 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.) --- newsfragments/3176.fixed.md | 1 + src/impl_/pyclass.rs | 24 ++++++++++++++++++++++- src/pycell.rs | 4 +++- tests/test_class_basics.rs | 38 +++++++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 newsfragments/3176.fixed.md diff --git a/newsfragments/3176.fixed.md b/newsfragments/3176.fixed.md new file mode 100644 index 00000000000..9c7decf7f04 --- /dev/null +++ b/newsfragments/3176.fixed.md @@ -0,0 +1 @@ +Avoid running the `Drop` implementations of unsendable classes on other threads diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 56472e3e2a1..312144a69c0 100644 --- a/src/impl_/pyclass.rs +++ b/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 PyClassNewTextSignature for &'_ PyClassImplCollector { #[doc(hidden)] pub trait PyClassThreadChecker: Sized { fn ensure(&self); + fn can_drop(&self, py: Python<'_>) -> bool; fn new() -> Self; private_decl! {} } @@ -894,6 +895,9 @@ pub struct ThreadCheckerStub(PhantomData); impl PyClassThreadChecker for ThreadCheckerStub { fn ensure(&self) {} + fn can_drop(&self, _py: Python<'_>) -> bool { + true + } #[inline] fn new() -> Self { ThreadCheckerStub(PhantomData) @@ -903,6 +907,9 @@ impl PyClassThreadChecker for ThreadCheckerStub { impl PyClassThreadChecker for ThreadCheckerStub { fn ensure(&self) {} + fn can_drop(&self, _py: Python<'_>) -> bool { + true + } #[inline] fn new() -> Self { ThreadCheckerStub(PhantomData) @@ -924,6 +931,18 @@ impl PyClassThreadChecker for ThreadCheckerImpl { std::any::type_name::() ); } + 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::() + )) + .write_unraisable(py, None); + return false; + } + + true + } fn new() -> Self { ThreadCheckerImpl(thread::current().id(), PhantomData) } @@ -944,6 +963,9 @@ impl PyClassThreadChecker 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()) } diff --git a/src/pycell.rs b/src/pycell.rs index 7e301c3e2d7..44ee822fffb 100644 --- a/src/pycell.rs +++ b/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); - 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); ::LayoutAsBase::tp_dealloc(py, slf) diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index ac3c98d0844..deff52dc98b 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -526,3 +526,41 @@ fn access_frozen_class_without_gil() { assert_eq!(py_counter.get().value.load(Ordering::Relaxed), 1); } + +#[test] +fn drop_unsendable_elsewhere() { + use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, + }; + use std::thread::spawn; + + #[pyclass(unsendable)] + struct Unsendable { + dropped: Arc, + } + + impl Drop for Unsendable { + fn drop(&mut self) { + self.dropped.store(true, Ordering::SeqCst); + } + } + + let dropped = Arc::new(AtomicBool::new(false)); + + let unsendable = Python::with_gil(|py| { + let dropped = dropped.clone(); + + Py::new(py, Unsendable { dropped }).unwrap() + }); + + spawn(move || { + Python::with_gil(move |_py| { + drop(unsendable); + }); + }) + .join() + .unwrap(); + + assert!(!dropped.load(Ordering::SeqCst)); +} From e85bfcc3bf31fe3143bb8e03be99635f0796342f Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Wed, 24 May 2023 11:04:05 +0200 Subject: [PATCH 2/2] Factor out UnraisableCapture helper type and use it to check that dropping unsendable elsewhere calls into sys.unraisablehook --- tests/common.rs | 50 +++++++++++++++++++ tests/test_buffer_protocol.rs | 24 ++------- tests/test_class_basics.rs | 92 ++++++++++++++++++++++------------- tests/test_exceptions.rs | 24 ++------- 4 files changed, 115 insertions(+), 75 deletions(-) diff --git a/tests/common.rs b/tests/common.rs index 75a246a9e7d..1003d54658e 100644 --- a/tests/common.rs +++ b/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, +} + +#[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 { + 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(); + } +} diff --git a/tests/test_buffer_protocol.rs b/tests/test_buffer_protocol.rs index c3843909c8f..a9c0ff8ea17 100644 --- a/tests/test_buffer_protocol.rs +++ b/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); }); } diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index deff52dc98b..a0ec4b0c1b2 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -228,31 +228,40 @@ impl UnsendableChild { } fn test_unsendable() -> PyResult<()> { - let obj = std::thread::spawn(|| -> PyResult<_> { + let obj = Python::with_gil(|py| -> PyResult<_> { + let obj: Py = PyType::new::(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::()?, 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 = PyType::new::(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::()?, 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::() { + panic!("{}", msg); + } + } + + Ok(()) } /// If a class is marked as `unsendable`, it panics when accessed by another thread. @@ -528,7 +537,10 @@ fn access_frozen_class_without_gil() { } #[test] +#[cfg(Py_3_8)] // sys.unraisablehook not available until Python 3.8 +#[cfg_attr(target_arch = "wasm32", ignore)] fn drop_unsendable_elsewhere() { + use common::UnraisableCapture; use std::sync::{ atomic::{AtomicBool, Ordering}, Arc, @@ -546,21 +558,35 @@ fn drop_unsendable_elsewhere() { } } - let dropped = Arc::new(AtomicBool::new(false)); + Python::with_gil(|py| { + let capture = UnraisableCapture::install(py); - let unsendable = Python::with_gil(|py| { - let dropped = dropped.clone(); + let dropped = Arc::new(AtomicBool::new(false)); - Py::new(py, Unsendable { dropped }).unwrap() - }); + let unsendable = Py::new( + py, + Unsendable { + dropped: dropped.clone(), + }, + ) + .unwrap(); - spawn(move || { - Python::with_gil(move |_py| { - drop(unsendable); + py.allow_threads(|| { + spawn(move || { + Python::with_gil(move |_py| { + drop(unsendable); + }); + }) + .join() + .unwrap(); }); - }) - .join() - .unwrap(); - assert!(!dropped.load(Ordering::SeqCst)); + 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); + }); } diff --git a/tests/test_exceptions.rs b/tests/test_exceptions.rs index cbbbd6368d5..c0cf57a9148 100644 --- a/tests/test_exceptions.rs +++ b/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); }); }