From 6710dcd1225ab09952d0ddfbd4156227f0cebd2a Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 1 Aug 2024 10:31:45 -0600 Subject: [PATCH] Update dict.get_item binding to use PyDict_GetItemRef (#4355) * Update dict.get_item binding to use PyDict_GetItemRef Refs #4265 * test: add test for dict.get_item error path * test: add test for dict.get_item error path * test: add test for dict.get_item error path * fix: fix logic error in dict.get_item bindings * update: apply david's review suggestions for dict.get_item bindings * update: create ffi::compat to store compatibility shims * update: move PyDict_GetItemRef bindings to spot in order from dictobject.h * build: fix build warning with --no-default-features * doc: expand release note fragments * fix: fix clippy warnings * respond to review comments * Apply suggestion from @mejrs * refactor so cfg is applied to functions * properly set cfgs * fix clippy lints * Apply @davidhewitt's suggestion * deal with upstream deprecation of new_bound --- newsfragments/4355.added.md | 10 ++++++++ newsfragments/4355.fixed.md | 2 ++ pyo3-ffi/src/compat.rs | 44 +++++++++++++++++++++++++++++++++++ pyo3-ffi/src/dictobject.rs | 6 +++++ pyo3-ffi/src/lib.rs | 3 +++ src/types/dict.rs | 46 +++++++++++++++++++++++++++++++++---- 6 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 newsfragments/4355.added.md create mode 100644 newsfragments/4355.fixed.md create mode 100644 pyo3-ffi/src/compat.rs diff --git a/newsfragments/4355.added.md b/newsfragments/4355.added.md new file mode 100644 index 00000000000..1410c0720bf --- /dev/null +++ b/newsfragments/4355.added.md @@ -0,0 +1,10 @@ +* Added an `ffi::compat` namespace to store compatibility shims for C API + functions added in recent versions of Python. + +* Added bindings for `PyDict_GetItemRef` on Python 3.13 and newer. Also added + `ffi::compat::PyDict_GetItemRef` which re-exports the FFI binding on Python + 3.13 or newer and defines a compatibility version on older versions of + Python. This function is inherently safer to use than `PyDict_GetItem` and has + an API that is easier to use than `PyDict_GetItemWithError`. It returns a + strong reference to value, as opposed to the two older functions which return + a possibly unsafe borrowed reference. diff --git a/newsfragments/4355.fixed.md b/newsfragments/4355.fixed.md new file mode 100644 index 00000000000..9a141bc6b96 --- /dev/null +++ b/newsfragments/4355.fixed.md @@ -0,0 +1,2 @@ +Avoid creating temporary borrowed reference in dict.get_item bindings. Borrowed +references like this are unsafe in the free-threading build. diff --git a/pyo3-ffi/src/compat.rs b/pyo3-ffi/src/compat.rs new file mode 100644 index 00000000000..ef9a3fbe1c9 --- /dev/null +++ b/pyo3-ffi/src/compat.rs @@ -0,0 +1,44 @@ +//! C API Compatibility Shims +//! +//! Some CPython C API functions added in recent versions of Python are +//! inherently safer to use than older C API constructs. This module +//! exposes functions available on all Python versions that wrap the +//! old C API on old Python versions and wrap the function directly +//! on newer Python versions. + +// Unless otherwise noted, the compatibility shims are adapted from +// the pythoncapi-compat project: https://github.com/python/pythoncapi-compat + +#[cfg(not(Py_3_13))] +use crate::object::PyObject; +#[cfg(not(Py_3_13))] +use std::os::raw::c_int; + +#[cfg_attr(docsrs, doc(cfg(all)))] +#[cfg(Py_3_13)] +pub use crate::dictobject::PyDict_GetItemRef; + +#[cfg_attr(docsrs, doc(cfg(all)))] +#[cfg(not(Py_3_13))] +pub unsafe fn PyDict_GetItemRef( + dp: *mut PyObject, + key: *mut PyObject, + result: *mut *mut PyObject, +) -> c_int { + { + use crate::dictobject::PyDict_GetItemWithError; + use crate::object::_Py_NewRef; + use crate::pyerrors::PyErr_Occurred; + + let item: *mut PyObject = PyDict_GetItemWithError(dp, key); + if !item.is_null() { + *result = _Py_NewRef(item); + return 1; // found + } + *result = std::ptr::null_mut(); + if PyErr_Occurred().is_null() { + return 0; // not found + } + -1 + } +} diff --git a/pyo3-ffi/src/dictobject.rs b/pyo3-ffi/src/dictobject.rs index 99fc56b246b..4d8315d441e 100644 --- a/pyo3-ffi/src/dictobject.rs +++ b/pyo3-ffi/src/dictobject.rs @@ -66,6 +66,12 @@ extern "C" { ) -> c_int; #[cfg_attr(PyPy, link_name = "PyPyDict_DelItemString")] pub fn PyDict_DelItemString(dp: *mut PyObject, key: *const c_char) -> c_int; + #[cfg(Py_3_13)] + pub fn PyDict_GetItemRef( + dp: *mut PyObject, + key: *mut PyObject, + result: *mut *mut PyObject, + ) -> c_int; // skipped 3.10 / ex-non-limited PyObject_GenericGetDict } diff --git a/pyo3-ffi/src/lib.rs b/pyo3-ffi/src/lib.rs index ff4d03d3a44..55c7f31404f 100644 --- a/pyo3-ffi/src/lib.rs +++ b/pyo3-ffi/src/lib.rs @@ -1,3 +1,4 @@ +#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] //! Raw FFI declarations for Python's C API. //! //! PyO3 can be used to write native Python modules or run Python code and modules from Rust. @@ -290,6 +291,8 @@ pub const fn _cstr_from_utf8_with_nul_checked(s: &str) -> &CStr { use std::ffi::CStr; +pub mod compat; + pub use self::abstract_::*; pub use self::bltinmodule::*; pub use self::boolobject::*; diff --git a/src/types/dict.rs b/src/types/dict.rs index 50d00477c2e..154fd97c241 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -436,13 +436,13 @@ impl<'py> PyDictMethods<'py> for Bound<'py, PyDict> { key: Bound<'_, PyAny>, ) -> PyResult>> { let py = dict.py(); + let mut result: *mut ffi::PyObject = std::ptr::null_mut(); match unsafe { - ffi::PyDict_GetItemWithError(dict.as_ptr(), key.as_ptr()) - .assume_borrowed_or_opt(py) - .map(Borrowed::to_owned) + ffi::compat::PyDict_GetItemRef(dict.as_ptr(), key.as_ptr(), &mut result) } { - some @ Some(_) => Ok(some), - None => PyErr::take(py).map(Err).transpose(), + std::os::raw::c_int::MIN..=-1 => Err(PyErr::fetch(py)), + 0 => Ok(None), + 1..=std::os::raw::c_int::MAX => Ok(Some(unsafe { result.assume_owned(py) })), } } @@ -957,6 +957,42 @@ mod tests { }); } + #[cfg(feature = "macros")] + #[test] + fn test_get_item_error_path() { + use crate::exceptions::PyTypeError; + + #[crate::pyclass(crate = "crate")] + struct HashErrors; + + #[crate::pymethods(crate = "crate")] + impl HashErrors { + #[new] + fn new() -> Self { + HashErrors {} + } + + fn __hash__(&self) -> PyResult { + Err(PyTypeError::new_err("Error from __hash__")) + } + } + + Python::with_gil(|py| { + let class = py.get_type_bound::(); + let instance = class.call0().unwrap(); + let d = PyDict::new_bound(py); + match d.get_item(instance) { + Ok(_) => { + panic!("this get_item call should always error") + } + Err(err) => { + assert!(err.is_instance_of::(py)); + assert_eq!(err.value_bound(py).to_string(), "Error from __hash__") + } + } + }) + } + #[test] #[allow(deprecated)] #[cfg(all(not(any(PyPy, GraalPy)), feature = "gil-refs"))]