Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ffi: prevent segfault with datetime bindings #1563

Merged
merged 1 commit into from
Apr 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
82 changes: 48 additions & 34 deletions src/ffi/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +441 to +445
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these correctly linked?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I checked in the docs locally🚀

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
Expand All @@ -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
Expand Down Expand Up @@ -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() }
}
}