From b73bb06ff4ded67ae6ab3fa1e12155681356aec0 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Wed, 8 Apr 2020 16:25:32 +0200 Subject: [PATCH 1/4] Fix panics caused by GC borrowing when class is already mutably borrowed --- src/class/gc.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/class/gc.rs b/src/class/gc.rs index b8638808e5a..e6fe3cf73c8 100644 --- a/src/class/gc.rs +++ b/src/class/gc.rs @@ -98,9 +98,14 @@ where arg, _py: py, }; - match slf.borrow().__traverse__(visit) { - Ok(()) => 0, - Err(PyTraverseError(code)) => code, + + if let Ok(borrow) = slf.try_borrow() { + match borrow.__traverse__(visit) { + Ok(()) => 0, + Err(PyTraverseError(code)) => code, + } + } else { + 0 } } From 049202c011f62b59637cc56fc609f6741d9e985e Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Wed, 8 Apr 2020 16:35:22 +0200 Subject: [PATCH 2/4] Add changelog entry [ci skip] --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1879d72de26..21606d6519d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added * `FromPyObject` implementations for `HashSet` and `BTreeSet`. [#842](https://github.com/PyO3/pyo3/pull/842) +### Fixed +- Garbage Collector causing random panics when traversing objects that were mutably borrowed. [#855](https://github.com/PyO3/pyo3/pull/855) + ## [0.9.1] ### Fixed From 5f2ec47beaed43b7a1c9b65386a45766579696a4 Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Thu, 9 Apr 2020 00:49:51 +0200 Subject: [PATCH 3/4] Add proper test for GC double borrow issue in `tp_traverse` wrapper --- tests/test_gc.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 829bb881d73..16700e7ca5d 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -2,6 +2,7 @@ use pyo3::class::PyGCProtocol; use pyo3::class::PyTraverseError; use pyo3::class::PyVisit; use pyo3::prelude::*; +use pyo3::type_object::PyTypeObject; use pyo3::{py_run, AsPyPointer, PyCell, PyTryInto}; use std::cell::RefCell; use std::sync::atomic::{AtomicBool, Ordering}; @@ -245,3 +246,63 @@ fn inheritance_with_new_methods_with_drop() { assert!(drop_called1.load(Ordering::Relaxed)); assert!(drop_called2.load(Ordering::Relaxed)); } + +#[pyclass(gc)] +struct TraversableClass { + traversed: AtomicBool, +} + +impl TraversableClass { + fn new() -> Self { + Self { + traversed: AtomicBool::new(false), + } + } +} + +#[pyproto] +impl PyGCProtocol for TraversableClass { + fn __clear__(&mut self) {} + fn __traverse__(&self, _visit: PyVisit) -> Result<(), PyTraverseError> { + self.traversed.store(true, Ordering::Relaxed); + Ok(()) + } +} + +#[test] +fn gc_during_borrow() { + let gil = Python::acquire_gil(); + let py = gil.python(); + + unsafe { + // declare a dummy visitor function + extern "C" fn novisit( + object: *mut pyo3::ffi::PyObject, + arg: *mut core::ffi::c_void, + ) -> std::os::raw::c_int { + 0 + } + + // get the traverse function + let ty = TraversableClass::type_object().as_ref(py).as_type_ptr(); + let traverse = (*ty).tp_traverse.unwrap(); + + // create an object and check that traversing it works normally + // when it's not borrowed + let cell = PyCell::new(py, TraversableClass::new()).unwrap(); + let obj = cell.to_object(py); + assert!(!cell.borrow().traversed.load(Ordering::Relaxed)); + traverse(obj.as_ptr(), novisit, std::ptr::null_mut()); + assert!(cell.borrow().traversed.load(Ordering::Relaxed)); + + // create an object and check that it is not traversed if the GC + // is invoked while it is already borrowed mutably + let cell2 = PyCell::new(py, TraversableClass::new()).unwrap(); + let obj2 = cell2.to_object(py); + let guard = cell2.borrow_mut(); + assert!(!guard.traversed.load(Ordering::Relaxed)); + traverse(obj2.as_ptr(), novisit, std::ptr::null_mut()); + assert!(!guard.traversed.load(Ordering::Relaxed)); + drop(guard); + } +} From 1e8e6fd8279e1e5824ed02ef59f90540350b304d Mon Sep 17 00:00:00 2001 From: Martin Larralde Date: Thu, 9 Apr 2020 02:06:16 +0200 Subject: [PATCH 4/4] Fix clippy warning about unused arguments in `tests/test_gc.rs` --- tests/test_gc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 16700e7ca5d..88e440fdf02 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -277,8 +277,8 @@ fn gc_during_borrow() { unsafe { // declare a dummy visitor function extern "C" fn novisit( - object: *mut pyo3::ffi::PyObject, - arg: *mut core::ffi::c_void, + _object: *mut pyo3::ffi::PyObject, + _arg: *mut core::ffi::c_void, ) -> std::os::raw::c_int { 0 }