From 3b1f720eb0f57a636456a584a68b9e4755d36e73 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 13 May 2020 18:36:40 +0100 Subject: [PATCH 1/4] Fix deadlock in update_counts --- CHANGELOG.md | 4 +++- src/gil.rs | 64 +++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 56 insertions(+), 12 deletions(-) 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()) + } + } } From 509db1177795ba97d25ec12ddfe9f21624a8f1ac Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 14 May 2020 09:06:56 +0100 Subject: [PATCH 2/4] Use parking_lot::Mutex instead of spinlock --- Cargo.toml | 1 + src/gil.rs | 65 +++++++++++++++--------------------------------------- 2 files changed, 19 insertions(+), 47 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e12e718e..ad000b4a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ appveyor = { repository = "fafhrd91/pyo3" } indoc = { version = "0.3.4", optional = true } inventory = { version = "0.1.4", optional = true } libc = "0.2.62" +parking_lot = "0.10.2" num-bigint = { version = "0.2", optional = true } num-complex = { version = "0.2", optional = true } paste = { version = "0.1.6", optional = true } diff --git a/src/gil.rs b/src/gil.rs index 351a88e6..c06ee906 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -3,8 +3,8 @@ //! Interaction with python's global interpreter lock use crate::{ffi, internal_tricks::Unsendable, Python}; -use std::cell::{Cell, RefCell, UnsafeCell}; -use std::sync::atomic::{spin_loop_hint, AtomicBool, Ordering}; +use parking_lot::{const_mutex, Mutex}; +use std::cell::{Cell, RefCell}; use std::{any, mem::ManuallyDrop, ptr::NonNull, sync}; static START: sync::Once = sync::Once::new(); @@ -168,65 +168,36 @@ impl Drop for GILGuard { /// Thread-safe storage for objects which were inc_ref / dec_ref while the GIL was not held. struct ReferencePool { - locked: AtomicBool, - pointers_to_incref: UnsafeCell>>, - pointers_to_decref: UnsafeCell>>, -} - -struct Lock<'a> { - lock: &'a AtomicBool, -} - -impl<'a> Lock<'a> { - fn new(lock: &'a AtomicBool) -> Self { - while lock.compare_and_swap(false, true, Ordering::Acquire) { - spin_loop_hint(); - } - Self { lock } - } -} - -impl<'a> Drop for Lock<'a> { - fn drop(&mut self) { - self.lock.store(false, Ordering::Release); - } + pointers_to_incref: Mutex>>, + pointers_to_decref: Mutex>>, } impl ReferencePool { const fn new() -> Self { Self { - locked: AtomicBool::new(false), - pointers_to_incref: UnsafeCell::new(Vec::new()), - pointers_to_decref: UnsafeCell::new(Vec::new()), + pointers_to_incref: const_mutex(Vec::new()), + pointers_to_decref: const_mutex(Vec::new()), } } fn register_incref(&self, obj: NonNull) { - let _lock = Lock::new(&self.locked); - let v = self.pointers_to_incref.get(); - unsafe { (*v).push(obj) }; + self.pointers_to_incref.lock().push(obj) } fn register_decref(&self, obj: NonNull) { - let _lock = Lock::new(&self.locked); - let v = self.pointers_to_decref.get(); - unsafe { (*v).push(obj) }; + self.pointers_to_decref.lock().push(obj) } fn update_counts(&self, _py: Python) { 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. + // Get vec from one of ReferencePool's mutexes via lock, swap vec if needed, unlock. ($cell:expr) => {{ - let _lock = Lock::new(&self.locked); - let v = $cell.get(); + let mut locked = $cell.lock(); let mut out = None; - unsafe { - if !(*v).is_empty() { - std::mem::swap(out.get_or_insert_with(Vec::new), &mut *v); - } + if !locked.is_empty() { + std::mem::swap(out.get_or_insert_with(Vec::new), &mut *locked); } - drop(_lock); + drop(locked); out }}; }; @@ -648,15 +619,15 @@ mod test { // The pointer should appear once in the incref pool, and once in the // decref pool (for the clone being created and also dropped) - assert_eq!(unsafe { &*POOL.pointers_to_incref.get() }, &vec![ptr]); - assert_eq!(unsafe { &*POOL.pointers_to_decref.get() }, &vec![ptr]); + assert_eq!(&*POOL.pointers_to_incref.lock(), &vec![ptr]); + assert_eq!(&*POOL.pointers_to_decref.lock(), &vec![ptr]); // Re-acquring GIL will clear these pending changes drop(gil); let _gil = Python::acquire_gil(); - assert!(unsafe { (*POOL.pointers_to_incref.get()).is_empty() }); - assert!(unsafe { (*POOL.pointers_to_decref.get()).is_empty() }); + assert!(POOL.pointers_to_incref.lock().is_empty()); + assert!(POOL.pointers_to_decref.lock().is_empty()); // Overall count is still unchanged assert_eq!(count, obj.get_refcnt()); @@ -686,7 +657,7 @@ mod test { 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()); + POOL.register_decref(NonNull::new(capsule).unwrap()); // Updating the counts will call decref on the capsule, which calls capsule_drop POOL.update_counts(gil.python()) From 2954342f6c986b1a5ba63e02a55fab2cccaaf9fa Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 14 May 2020 20:03:14 +0900 Subject: [PATCH 3/4] A tiny optimization in ReferencePool::update_counts --- src/gil.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/gil.rs b/src/gil.rs index c06ee906..7c333da2 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -193,9 +193,9 @@ impl ReferencePool { // Get vec from one of ReferencePool's mutexes via lock, swap vec if needed, unlock. ($cell:expr) => {{ let mut locked = $cell.lock(); - let mut out = None; + let mut out = Vec::new(); if !locked.is_empty() { - std::mem::swap(out.get_or_insert_with(Vec::new), &mut *locked); + std::mem::swap(&mut out, &mut *locked); } drop(locked); out @@ -205,20 +205,12 @@ impl ReferencePool { // Always increase reference counts first - as otherwise objects which have a // nonzero total reference count might be incorrectly dropped by Python during // this update. - { - if let Some(v) = swap_vec_with_lock!(self.pointers_to_incref) { - for ptr in v { - unsafe { ffi::Py_INCREF(ptr.as_ptr()) }; - } - } + for ptr in swap_vec_with_lock!(self.pointers_to_incref) { + unsafe { ffi::Py_INCREF(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()) }; - } - } + for ptr in swap_vec_with_lock!(self.pointers_to_decref) { + unsafe { ffi::Py_DECREF(ptr.as_ptr()) }; } } } From 465e83bb962363cac950a794f6afeb19d16fbfc6 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Thu, 14 May 2020 20:40:33 +0900 Subject: [PATCH 4/4] [travis] Do not install perftools since now grcov doesn't need it --- .travis.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 9bc12788..e47990bf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -34,11 +34,6 @@ env: - TRAVIS_RUST_VERSION=nightly - RUST_BACKTRACE=1 -addons: - apt: - packages: - - libgoogle-perftools-dev - before_install: - source ./ci/travis/setup.sh - curl -L https://github.com/mozilla/grcov/releases/latest/download/grcov-linux-x86_64.tar.bz2 | tar jxf -