Merge pull request #893 from davidhewitt/safe_acquire_gil

kngwyu · web-flow · commit 8e8472143867 · 2020-05-09T19:03:15.000+09:00
Close soundness hole with acquire_gil
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   - When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.)
 - `FromPyObject` for `Py<T>` now works for a wider range of `T`, in particular for `T: PyClass`. [#880](https://github.com/PyO3/pyo3/pull/880)
 - Some functions such as `PyList::get_item` which return borrowed objects at the C ffi layer now return owned objects at the PyO3 layer, for safety reasons. [#890](https://github.com/PyO3/pyo3/pull/890)
+- The `GILGuard` returned from `Python::acquire_gil` will now only assume responsiblity for freeing owned references on drop if no other `GILPool` or `GILGuard` exists. This ensures that multiple calls to the safe api `Python::acquire_gil` cannot lead to dangling references. [#893](https://github.com/PyO3/pyo3/pull/893)
 - The trait `ObjectProtocol` has been removed, and all the methods from the trait have been moved to `PyAny`. [#911](https://github.com/PyO3/pyo3/pull/911)
   - The exception to this is `ObjectProtocol::None`, which has simply been removed. Use `Python::None` instead.
 
diff --git a/guide/src/advanced.md b/guide/src/advanced.md
@@ -6,6 +6,16 @@ PyO3 exposes much of Python's C API through the `ffi` module.
 
 The C API is naturally unsafe and requires you to manage reference counts, errors and specific invariants yourself. Please refer to the [C API Reference Manual](https://docs.python.org/3/c-api/) and [The Rustonomicon](https://doc.rust-lang.org/nightly/nomicon/ffi.html) before using any function from that API.
 
+## Memory Management
+
+PyO3's "owned references" (`&PyAny` etc.) make PyO3 more ergonomic to use by ensuring that their lifetime can never be longer than the duration the Python GIL is held. This means that most of PyO3's API can assume the GIL is held. (If PyO3 could not assume this, every PyO3 API would need to take a `Python` GIL token to prove that the GIL is held.)
+
+The caveat to these "owned references" is that Rust references do not normally convey ownership (they are always `Copy`, and cannot implement `Drop`). Whenever a PyO3 API returns an owned reference, PyO3 stores it internally, so that PyO3 can decrease the reference count just before PyO3 releases the GIL.
+
+For most use cases this behaviour is invisible. Occasionally, however, users may need to clear memory usage sooner than PyO3 usually does. PyO3 exposes this functionality with the  the `GILPool` struct. When a `GILPool` is dropped, ***all*** owned references created after the `GILPool` was created will be cleared.
+
+The unsafe function `Python::new_pool` allows you to create a new `GILPool`. When doing this, you must be very careful to ensure that once the `GILPool` is dropped you do not retain access any owned references created after the `GILPool` was created.
+
 ## Testing
 
 Currently, [#341](https://github.com/PyO3/pyo3/issues/341) causes `cargo test` to fail with weird linking errors when the `extension-module` feature is activated. For now you can work around this by making the `extension-module` feature optional and running the tests with `cargo test --no-default-features`:
diff --git a/src/gil.rs b/src/gil.rs
@@ -12,8 +12,8 @@ static START: sync::Once = sync::Once::new();
 thread_local! {
     /// This is a internal counter in pyo3 monitoring whether this thread has the GIL.
     ///
-    /// It will be incremented whenever a GIL-holding RAII struct is created, and decremented
-    /// whenever they are dropped.
+    /// It will be incremented whenever a GILPool is created, and decremented whenever they are
+    /// dropped.
     ///
     /// As a result, if this thread has the GIL, GIL_COUNT is greater than zero.
     ///
@@ -115,22 +115,35 @@ pub fn prepare_freethreaded_python() {
 #[must_use]
 pub struct GILGuard {
     gstate: ffi::PyGILState_STATE,
-    pool: ManuallyDrop<GILPool>,
+    pool: ManuallyDrop<Option<GILPool>>,
 }
 
 impl GILGuard {
-    /// Acquires the global interpreter lock, which allows access to the Python runtime.
+    /// Acquires the global interpreter lock, which allows access to the Python runtime. This is
+    /// safe to call multiple times without causing a deadlock.
     ///
     /// If the Python runtime is not already initialized, this function will initialize it.
     /// See [prepare_freethreaded_python()](fn.prepare_freethreaded_python.html) for details.
+    ///
+    /// If PyO3 does not yet have a `GILPool` for tracking owned PyObject references, then this
+    /// new `GILGuard` will also contain a `GILPool`.
     pub fn acquire() -> GILGuard {
         prepare_freethreaded_python();
 
         unsafe {
             let gstate = ffi::PyGILState_Ensure(); // acquire GIL
+
+            // If there's already a GILPool, we should not create another or this could lead to
+            // incorrect dangling references in safe code (see #864).
+            let pool = if !gil_is_acquired() {
+                Some(GILPool::new())
+            } else {
+                None
+            };
+
             GILGuard {
                 gstate,
-                pool: ManuallyDrop::new(GILPool::new()),
+                pool: ManuallyDrop::new(pool),
             }
         }
     }
@@ -208,7 +221,7 @@ unsafe impl Sync for ReleasePool {}
 
 static POOL: ReleasePool = ReleasePool::new();
 
-#[doc(hidden)]
+/// A RAII pool which PyO3 uses to store owned Python references.
 pub struct GILPool {
     owned_objects_start: usize,
     owned_anys_start: usize,
@@ -217,8 +230,13 @@ pub struct GILPool {
 }
 
 impl GILPool {
+    /// Create a new `GILPool`. This function should only ever be called with the GIL.
+    ///
+    /// It is recommended not to use this API directly, but instead to use `Python::new_pool`, as
+    /// that guarantees the GIL is held.
+    ///
     /// # Safety
-    /// This function requires that GIL is already acquired.
+    /// As well as requiring the GIL, see the notes on `Python::new_pool`.
     #[inline]
     pub unsafe fn new() -> GILPool {
         increment_gil_count();
@@ -230,8 +248,10 @@ impl GILPool {
             no_send: Unsendable::default(),
         }
     }
-    pub unsafe fn python(&self) -> Python {
-        Python::assume_gil_acquired()
+
+    /// Get the Python token associated with this `GILPool`.
+    pub fn python(&self) -> Python {
+        unsafe { Python::assume_gil_acquired() }
     }
 }
 
@@ -325,13 +345,13 @@ mod test {
     use crate::{ffi, gil, AsPyPointer, IntoPyPointer, PyObject, Python, ToPyObject};
     use std::ptr::NonNull;
 
-    fn get_object() -> PyObject {
-        // Convenience function for getting a single unique object
-        let gil = Python::acquire_gil();
-        let py = gil.python();
+    fn get_object(py: Python) -> PyObject {
+        // Convenience function for getting a single unique object, using `new_pool` so as to leave
+        // the original pool state unchanged.
+        let pool = unsafe { py.new_pool() };
+        let py = pool.python();
 
         let obj = py.eval("object()", None, None).unwrap();
-
         obj.to_object(py)
     }
 
@@ -343,21 +363,21 @@ mod test {
     fn test_owned() {
         let gil = Python::acquire_gil();
         let py = gil.python();
-        let obj = get_object();
+        let obj = get_object(py);
         let obj_ptr = obj.as_ptr();
         // Ensure that obj does not get freed
         let _ref = obj.clone_ref(py);
 
         unsafe {
             {
-                let gil = Python::acquire_gil();
-                gil::register_owned(gil.python(), NonNull::new_unchecked(obj.into_ptr()));
+                let pool = py.new_pool();
+                gil::register_owned(pool.python(), NonNull::new_unchecked(obj.into_ptr()));
 
-                assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
                 assert_eq!(owned_object_count(), 1);
+                assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
             }
             {
-                let _gil = Python::acquire_gil();
+                let _pool = py.new_pool();
                 assert_eq!(owned_object_count(), 0);
                 assert_eq!(ffi::Py_REFCNT(obj_ptr), 1);
             }
@@ -368,23 +388,23 @@ mod test {
     fn test_owned_nested() {
         let gil = Python::acquire_gil();
         let py = gil.python();
-        let obj = get_object();
+        let obj = get_object(py);
         // Ensure that obj does not get freed
         let _ref = obj.clone_ref(py);
         let obj_ptr = obj.as_ptr();
 
         unsafe {
             {
-                let _pool = GILPool::new();
+                let _pool = py.new_pool();
                 assert_eq!(owned_object_count(), 0);
 
                 gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
 
                 assert_eq!(owned_object_count(), 1);
                 assert_eq!(ffi::Py_REFCNT(obj_ptr), 2);
                 {
-                    let _pool = GILPool::new();
-                    let obj = get_object();
+                    let _pool = py.new_pool();
+                    let obj = get_object(py);
                     gil::register_owned(py, NonNull::new_unchecked(obj.into_ptr()));
                     assert_eq!(owned_object_count(), 2);
                 }
@@ -401,7 +421,7 @@ mod test {
     fn test_pyobject_drop_with_gil_decreases_refcnt() {
         let gil = Python::acquire_gil();
         let py = gil.python();
-        let obj = get_object();
+        let obj = get_object(py);
         // Ensure that obj does not get freed
         let _ref = obj.clone_ref(py);
         let obj_ptr = obj.as_ptr();
@@ -422,7 +442,7 @@ mod test {
     fn test_pyobject_drop_without_gil_doesnt_decrease_refcnt() {
         let gil = Python::acquire_gil();
         let py = gil.python();
-        let obj = get_object();
+        let obj = get_object(py);
         // Ensure that obj does not get freed
         let _ref = obj.clone_ref(py);
         let obj_ptr = obj.as_ptr();
@@ -465,15 +485,16 @@ mod test {
         drop(pool);
         assert_eq!(get_gil_count(), 2);
 
+        // Creating a new GILGuard should not increment the gil count if a GILPool already exists
         let gil2 = Python::acquire_gil();
-        assert_eq!(get_gil_count(), 3);
-
-        drop(gil2);
         assert_eq!(get_gil_count(), 2);
 
         drop(pool2);
         assert_eq!(get_gil_count(), 1);
 
+        drop(gil2);
+        assert_eq!(get_gil_count(), 1);
+
         drop(gil);
         assert_eq!(get_gil_count(), 0);
     }
@@ -482,7 +503,7 @@ mod test {
     fn test_allow_threads() {
         let gil = Python::acquire_gil();
         let py = gil.python();
-        let object = get_object();
+        let object = get_object(py);
 
         py.allow_threads(move || {
             // Should be no pointers to drop
@@ -499,12 +520,27 @@ mod test {
             // (Acquiring the GIL should have cleared the pool).
             assert!(unsafe { (*POOL.pointers_to_drop.get()).is_empty() });
 
-            let object = get_object();
+            let object = get_object(gil.python());
             drop(object);
             drop(gil);
 
             // Previous drop should have decreased count immediately instead of put in pool
             assert!(unsafe { (*POOL.pointers_to_drop.get()).is_empty() });
         })
     }
+
+    #[test]
+    fn dropping_gil_does_not_invalidate_references() {
+        // Acquiring GIL for the second time should be safe - see #864
+        let gil = Python::acquire_gil();
+        let py = gil.python();
+        let obj;
+
+        let gil2 = Python::acquire_gil();
+        obj = py.eval("object()", None, None).unwrap();
+        drop(gil2);
+
+        // After gil2 drops, obj should still have a reference count of one
+        assert_eq!(unsafe { ffi::Py_REFCNT(obj.as_ptr()) }, 1);
+    }
 }
diff --git a/src/python.rs b/src/python.rs
@@ -3,7 +3,7 @@
 // based on Daniel Grunwald's https://github.com/dgrunwald/rust-cpython
 
 use crate::err::{PyDowncastError, PyErr, PyResult};
-use crate::gil::{self, GILGuard};
+use crate::gil::{self, GILGuard, GILPool};
 use crate::type_object::{PyTypeInfo, PyTypeObject};
 use crate::types::{PyAny, PyDict, PyModule, PyType};
 use crate::{
@@ -292,6 +292,62 @@ impl<'p> Python<'p> {
     pub fn NotImplemented(self) -> PyObject {
         unsafe { PyObject::from_borrowed_ptr(self, ffi::Py_NotImplemented()) }
     }
+
+    /// Create a new pool for managing PyO3's owned references.
+    ///
+    /// When this `GILPool` is dropped, all PyO3 owned references created after this `GILPool` will
+    /// all have their Python reference counts decremented, potentially allowing Python to drop
+    /// the corresponding Python objects.
+    ///
+    /// Typical usage of PyO3 will not need this API, as `Python::acquire_gil` automatically
+    /// creates a `GILPool` where appropriate.
+    ///
+    /// Advanced uses of PyO3 which perform long-running tasks which never free the GIL may need
+    /// to use this API to clear memory, as PyO3 usually does not clear memory until the GIL is
+    /// released.
+    ///
+    /// # Example
+    /// ```rust
+    /// # use pyo3::prelude::*;
+    /// let gil = Python::acquire_gil();
+    /// let py = gil.python();
+    ///
+    /// // Some long-running process like a webserver, which never releases the GIL.
+    /// loop {
+    ///     // Create a new pool, so that PyO3 can clear memory at the end of the loop.
+    ///     let pool = unsafe { py.new_pool() };
+    ///
+    ///     // It is recommended to *always* immediately set py to the pool's Python, to help
+    ///     // avoid creating references with invalid lifetimes.
+    ///     let py = unsafe { pool.python() };
+    ///
+    ///     // do stuff...
+    /// # break;  // Exit the loop so that doctest terminates!
+    /// }
+    /// ```
+    ///
+    /// # Safety
+    /// Extreme care must be taken when using this API, as misuse can lead to accessing invalid
+    /// memory. In addition, the caller is responsible for guaranteeing that the GIL remains held
+    /// for the entire lifetime of the returned `GILPool`.
+    ///
+    /// Two best practices are required when using this API:
+    /// - From the moment `new_pool()` is called, only the `Python` token from the returned
+    ///   `GILPool` (accessible using `.python()`) should be used in PyO3 APIs. All other older
+    ///   `Python` tokens with longer lifetimes are unsafe to use until the `GILPool` is dropped,
+    ///   because they can be used to create PyO3 owned references which have lifetimes which
+    ///   outlive the `GILPool`.
+    /// - Similarly, methods on existing owned references will implicitly refer back to the
+    ///   `Python` token which that reference was originally created with. If the returned values
+    ///   from these methods are owned references they will inherit the same lifetime. As a result,
+    ///   Rust's lifetime rules may allow them to outlive the `GILPool`, even though this is not
+    ///   safe for reasons discussed above. Care must be taken to never access these return values
+    ///   after the `GILPool` is dropped, unless they are converted to `Py<T>` *before* the pool
+    ///   is dropped.
+    #[inline]
+    pub unsafe fn new_pool(self) -> GILPool {
+        GILPool::new()
+    }
 }
 
 impl<'p> Python<'p> {
