Merge pull request #1563 from davidhewitt/safer-datetime-ffi

ffi: prevent segfault with datetime bindings
PyO3 · Apr 18, 2021 · 6bfca27 · 6bfca27
2 parents 82113d7 + eaf7502
commit 6bfca27
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 34 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fix use of Python argument for #[pymethods] inside macro expansions. [#1505](https://github.com/PyO3/pyo3/pull/1505)
 - Always use cross-compiling configuration if any of the environment variables are set. [#1514](https://github.com/PyO3/pyo3/pull/1514)
 - Support `EnvironmentError`, `IOError`, and `WindowsError` on PyPy. [#1533](https://github.com/PyO3/pyo3/pull/1533)
+- Segfault when dereferencing `ffi::PyDateTimeAPI` without the GIL. [#1563](https://github.com/PyO3/pyo3/pull/1563)
 
 ## [0.13.2] - 2021-02-12
 ### Packaging

diff --git a/src/ffi/datetime.rs b/src/ffi/datetime.rs
@@ -435,26 +435,18 @@ pub struct PyDateTime_CAPI {
 
 // Python already shares this object between threads, so it's no more evil for us to do it too!
 unsafe impl Sync for PyDateTime_CAPI {}
-static PY_DATETIME_API: GILOnceCell<&'static PyDateTime_CAPI> = GILOnceCell::new();
 
-#[derive(Debug)]
-pub struct PyDateTimeAPI {
-    __private_field: (),
-}
-
-pub static PyDateTimeAPI: PyDateTimeAPI = PyDateTimeAPI {
-    __private_field: (),
+/// Safe wrapper around the Python datetime C-API global. Note that this object differs slightly
+/// from the equivalent C object: in C, this is implemented as a `static PyDateTime_CAPI *`. Here
+/// this is implemented as a wrapper which implements [`Deref`] to access a reference to a
+/// [`PyDateTime_CAPI`] object.
+///
+/// In the [`Deref`] implementation, if the underlying object has not yet been initialized, the GIL
+/// will automatically be acquired and [`PyDateTime_IMPORT()`] called.
+pub static PyDateTimeAPI: _PyDateTimeAPI_impl = _PyDateTimeAPI_impl {
+    inner: GILOnceCell::new(),
 };
 
-impl Deref for PyDateTimeAPI {
-    type Target = PyDateTime_CAPI;
-
-    fn deref(&self) -> &'static PyDateTime_CAPI {
-        unsafe { PyDateTime_IMPORT() }
-    }
-}
-
-#[inline]
 /// Populates the `PyDateTimeAPI` object
 ///
 /// Unlike in C, this does *not* need to be actively invoked in Rust, which
@@ -466,23 +458,29 @@ impl Deref for PyDateTimeAPI {
 /// # Safety
 /// The Python GIL must be held.
 pub unsafe fn PyDateTime_IMPORT() -> &'static PyDateTime_CAPI {
-    let py = Python::assume_gil_acquired();
-    PY_DATETIME_API.get_or_init(py, || {
-        // PyPy expects the C-API to be initialized via PyDateTime_Import, so trying to use
-        // `PyCapsule_Import` will behave unexpectedly in pypy.
-        #[cfg(PyPy)]
-        let py_datetime_c_api = PyDateTime_Import();
-
-        #[cfg(not(PyPy))]
-        let py_datetime_c_api = {
-            // PyDateTime_CAPSULE_NAME is a macro in C
-            let PyDateTime_CAPSULE_NAME = CString::new("datetime.datetime_CAPI").unwrap();
-
-            &*(PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1) as *const PyDateTime_CAPI)
-        };
-
-        py_datetime_c_api
-    })
+    PyDateTimeAPI
+        .inner
+        .get_or_init(Python::assume_gil_acquired(), || {
+            // Because `get_or_init` is called with `assume_gil_acquired()`, it's necessary to acquire
+            // the GIL here to prevent segfault in usage of the safe `PyDateTimeAPI::deref` impl.
+            Python::with_gil(|_py| {
+                // PyPy expects the C-API to be initialized via PyDateTime_Import, so trying to use
+                // `PyCapsule_Import` will behave unexpectedly in pypy.
+                #[cfg(PyPy)]
+                let py_datetime_c_api = PyDateTime_Import();
+
+                #[cfg(not(PyPy))]
+                let py_datetime_c_api = {
+                    // PyDateTime_CAPSULE_NAME is a macro in C
+                    let PyDateTime_CAPSULE_NAME = CString::new("datetime.datetime_CAPI").unwrap();
+
+                    &*(PyCapsule_Import(PyDateTime_CAPSULE_NAME.as_ptr(), 1)
+                        as *const PyDateTime_CAPI)
+                };
+
+                py_datetime_c_api
+            })
+        })
 }
 
 // skipped non-limited PyDateTime_TimeZone_UTC
@@ -573,3 +571,19 @@ extern "C" {
     #[link_name = "_PyPyDateTime_Import"]
     pub fn PyDateTime_Import() -> &'static PyDateTime_CAPI;
 }
+
+// -- implementation details which are specific to Rust. --
+
+#[doc(hidden)]
+pub struct _PyDateTimeAPI_impl {
+    inner: GILOnceCell<&'static PyDateTime_CAPI>,
+}
+
+impl Deref for _PyDateTimeAPI_impl {
+    type Target = PyDateTime_CAPI;
+
+    #[inline]
+    fn deref(&self) -> &'static PyDateTime_CAPI {
+        unsafe { PyDateTime_IMPORT() }
+    }
+}