diff --git a/CHANGELOG.md b/CHANGELOG.md index de82f92b..a60e7249 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- Fix deadlock in `Python::acquire_gil()` after dropping a `PyObject` or `Py`. [#924](https://github.com/PyO3/pyo3/pull/924) -## [0.10.0] +## [0.10.0] - 2020-05-13 ### Added - Add FFI definition `_PyDict_NewPresized`. [#849](https://github.com/PyO3/pyo3/pull/849) - Implement `IntoPy` for `HashSet` and `BTreeSet`. [#864](https://github.com/PyO3/pyo3/pull/864) diff --git a/src/gil.rs b/src/gil.rs index b3a9c078..351a88e6 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -214,28 +214,39 @@ impl ReferencePool { } fn update_counts(&self, _py: Python) { - let _lock = Lock::new(&self.locked); + macro_rules! swap_vec_with_lock { + // Get vec from one of ReferencePool's UnsafeCell fields by locking, swapping the + // vec for a new one if needed, and unlocking. + ($cell:expr) => {{ + let _lock = Lock::new(&self.locked); + let v = $cell.get(); + let mut out = None; + unsafe { + if !(*v).is_empty() { + std::mem::swap(out.get_or_insert_with(Vec::new), &mut *v); + } + } + drop(_lock); + out + }}; + }; // Always increase reference counts first - as otherwise objects which have a // nonzero total reference count might be incorrectly dropped by Python during // this update. { - let v = self.pointers_to_incref.get(); - unsafe { - for ptr in &(*v) { - ffi::Py_INCREF(ptr.as_ptr()); + if let Some(v) = swap_vec_with_lock!(self.pointers_to_incref) { + for ptr in v { + unsafe { ffi::Py_INCREF(ptr.as_ptr()) }; } - (*v).clear(); } } { - let v = self.pointers_to_decref.get(); - unsafe { - for ptr in &(*v) { - ffi::Py_DECREF(ptr.as_ptr()); + if let Some(v) = swap_vec_with_lock!(self.pointers_to_decref) { + for ptr in v { + unsafe { ffi::Py_DECREF(ptr.as_ptr()) }; } - (*v).clear(); } } } @@ -650,4 +661,35 @@ mod test { // Overall count is still unchanged assert_eq!(count, obj.get_refcnt()); } + + #[test] + fn test_update_counts_does_not_deadlock() { + // update_counts can run arbitrary Python code during Py_DECREF. + // if the locking is implemented incorrectly, it will deadlock. + + let gil = Python::acquire_gil(); + let obj = get_object(gil.python()); + + unsafe { + unsafe extern "C" fn capsule_drop(capsule: *mut ffi::PyObject) { + // This line will implicitly call update_counts + // -> and so cause deadlock if update_counts is not handling recursion correctly. + let pool = GILPool::new(); + + // Rebuild obj so that it can be dropped + PyObject::from_owned_ptr( + pool.python(), + ffi::PyCapsule_GetPointer(capsule, std::ptr::null()) as _, + ); + } + + let ptr = obj.into_ptr(); + let capsule = ffi::PyCapsule_New(ptr as _, std::ptr::null(), Some(capsule_drop)); + + (*POOL.pointers_to_decref.get()).push(NonNull::new(capsule).unwrap()); + + // Updating the counts will call decref on the capsule, which calls capsule_drop + POOL.update_counts(gil.python()) + } + } }