From 79412f2357c9978530948889b7118eeb1912863b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 20 Sep 2025 21:33:21 +0100 Subject: [PATCH 1/3] stop leaking `PyMethodDef` inside macro-generated code --- pyo3-macros-backend/src/pyfunction.rs | 7 +- src/impl_/pyclass.rs | 4 +- src/impl_/pyfunction.rs | 100 ++++++++++++++++++++++---- src/impl_/pymethods.rs | 41 ++++------- src/impl_/pymodule.rs | 9 +-- src/pyclass/create_type_object.rs | 2 +- src/sealed.rs | 2 + src/types/function.rs | 52 ++++---------- 8 files changed, 131 insertions(+), 86 deletions(-) diff --git a/pyo3-macros-backend/src/pyfunction.rs b/pyo3-macros-backend/src/pyfunction.rs index 303af155acf..a91f064a161 100644 --- a/pyo3-macros-backend/src/pyfunction.rs +++ b/pyo3-macros-backend/src/pyfunction.rs @@ -433,17 +433,18 @@ pub fn impl_wrap_pyfunction( #[doc(hidden)] #vis mod #name { pub(crate) struct MakeDef; - pub const _PYO3_DEF: #pyo3_path::impl_::pymethods::PyMethodDef = MakeDef::_PYO3_DEF; + pub static _PYO3_DEF: #pyo3_path::impl_::pyfunction::PyFunctionDef = MakeDef::_PYO3_DEF; #introspection_id } - // Generate the definition inside an anonymous function in the same scope as the original function - + // Generate the definition in the same scope as the original function - // this avoids complications around the fact that the generated module has a different scope // (and `super` doesn't always refer to the outer scope, e.g. if the `#[pyfunction] is // inside a function body) #[allow(unknown_lints, non_local_definitions)] impl #name::MakeDef { - const _PYO3_DEF: #pyo3_path::impl_::pymethods::PyMethodDef = #methoddef; + const _PYO3_DEF: #pyo3_path::impl_::pyfunction::PyFunctionDef = + #pyo3_path::impl_::pyfunction::PyFunctionDef::from_method_def(#methoddef); } #[allow(non_snake_case)] diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 8950b2ffc95..3fda206eb5b 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -1444,7 +1444,7 @@ mod tests { for items in FrozenClass::items_iter() { methods.extend(items.methods.iter().map(|m| match m { - MaybeRuntimePyMethodDef::Static(m) => m.clone(), + MaybeRuntimePyMethodDef::Static(m) => *m, MaybeRuntimePyMethodDef::Runtime(r) => r(), })); slots.extend_from_slice(items.slots); @@ -1482,7 +1482,7 @@ mod tests { for items in FrozenClass::items_iter() { methods.extend(items.methods.iter().map(|m| match m { - MaybeRuntimePyMethodDef::Static(m) => m.clone(), + MaybeRuntimePyMethodDef::Static(m) => *m, MaybeRuntimePyMethodDef::Runtime(r) => r(), })); slots.extend_from_slice(items.slots); diff --git a/src/impl_/pyfunction.rs b/src/impl_/pyfunction.rs index c389165488c..36de0473ae0 100644 --- a/src/impl_/pyfunction.rs +++ b/src/impl_/pyfunction.rs @@ -1,42 +1,118 @@ +use std::cell::UnsafeCell; + use crate::{ - types::{PyCFunction, PyModule}, + ffi, + ffi_ptr_ext::FfiPtrExt, + py_result_ext::PyResultExt, + types::{PyCFunction, PyModule, PyModuleMethods}, Borrowed, Bound, PyResult, Python, }; pub use crate::impl_::pymethods::PyMethodDef; +/// Wrapper around `ffi::PyMethodDef` suitable to use as a static variable for `#[pyfunction]` values. +/// +/// The `UnsafeCell` is used because the Python interpreter consumes these as `*mut ffi::PyMethodDef`. +pub struct PyFunctionDef(UnsafeCell); + +// Safety: contents are only ever used by the Python interpreter, which uses global statics in this way. +unsafe impl Sync for PyFunctionDef {} + +impl PyFunctionDef { + pub const fn new(def: ffi::PyMethodDef) -> Self { + Self(UnsafeCell::new(def)) + } + + pub const fn from_method_def(def: PyMethodDef) -> Self { + Self::new(def.into_raw()) + } + + pub(crate) fn create_py_c_function<'py>( + &'static self, + py: Python<'py>, + module: Option<&Bound<'py, PyModule>>, + ) -> PyResult> { + // Safety: self is static + unsafe { create_py_c_function(py, self.0.get(), module) } + } +} + /// Trait to enable the use of `wrap_pyfunction` with both `Python` and `PyModule`, /// and also to infer the return type of either `&'py PyCFunction` or `Bound<'py, PyCFunction>`. pub trait WrapPyFunctionArg<'py, T> { - fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult; + fn wrap_pyfunction(self, function_def: &'static PyFunctionDef) -> PyResult; } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Bound<'py, PyModule> { - fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new(self.py(), method_def, Some(&self)) + fn wrap_pyfunction( + self, + function_def: &'static PyFunctionDef, + ) -> PyResult> { + function_def.create_py_c_function(self.py(), Some(&self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Bound<'py, PyModule> { - fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new(self.py(), method_def, Some(self)) + fn wrap_pyfunction( + self, + function_def: &'static PyFunctionDef, + ) -> PyResult> { + function_def.create_py_c_function(self.py(), Some(self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Borrowed<'_, 'py, PyModule> { - fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new(self.py(), method_def, Some(&self)) + fn wrap_pyfunction( + self, + function_def: &'static PyFunctionDef, + ) -> PyResult> { + function_def.create_py_c_function(self.py(), Some(&self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for &'_ Borrowed<'_, 'py, PyModule> { - fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new(self.py(), method_def, Some(self)) + fn wrap_pyfunction( + self, + function_def: &'static PyFunctionDef, + ) -> PyResult> { + function_def.create_py_c_function(self.py(), Some(self)) } } impl<'py> WrapPyFunctionArg<'py, Bound<'py, PyCFunction>> for Python<'py> { - fn wrap_pyfunction(self, method_def: &PyMethodDef) -> PyResult> { - PyCFunction::internal_new(self, method_def, None) + fn wrap_pyfunction( + self, + function_def: &'static PyFunctionDef, + ) -> PyResult> { + function_def.create_py_c_function(self, None) + } +} + +/// Creates a `PyCFunction` object from a `PyMethodDef`. +/// +/// # Safety +/// +/// The `method_def` pointer must be valid for the lifetime of the returned `PyCFunction` +/// (effectively, it must be a static variable). +pub unsafe fn create_py_c_function<'py>( + py: Python<'py>, + method_def: *mut ffi::PyMethodDef, + module: Option<&Bound<'py, PyModule>>, +) -> PyResult> { + let (mod_ptr, module_name) = if let Some(m) = module { + let mod_ptr = m.as_ptr(); + (mod_ptr, Some(m.name()?)) + } else { + (std::ptr::null_mut(), None) + }; + + let module_name_ptr = module_name + .as_ref() + .map_or(std::ptr::null_mut(), Bound::as_ptr); + + unsafe { + ffi::PyCFunction_NewEx(method_def, mod_ptr, module_name_ptr) + .assume_owned_or_err(py) + .cast_into_unchecked() } } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index cbc7b626d03..fadfdb5ce7b 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -58,7 +58,7 @@ impl IPowModulo { /// `PyMethodDefType` represents different types of Python callable objects. /// It is used by the `#[pymethods]` attribute. -#[cfg_attr(test, derive(Clone))] +#[derive(Copy, Clone)] pub enum PyMethodDefType { /// Represents class method Class(PyMethodDef), @@ -86,10 +86,7 @@ pub enum PyMethodType { pub type PyClassAttributeFactory = for<'p> fn(Python<'p>) -> PyResult>; -// TODO: it would be nice to use CStr in these types, but then the constructors can't be const fn -// until `CStr::from_bytes_with_nul_unchecked` is const fn. - -#[derive(Clone, Debug)] +#[derive(Copy, Clone, Debug)] pub struct PyMethodDef { pub(crate) ml_name: &'static CStr, pub(crate) ml_meth: PyMethodType, @@ -103,26 +100,20 @@ pub struct PyClassAttributeDef { pub(crate) meth: PyClassAttributeFactory, } -#[derive(Clone)] +#[derive(Copy, Clone)] pub struct PyGetterDef { pub(crate) name: &'static CStr, pub(crate) meth: Getter, pub(crate) doc: &'static CStr, } -#[derive(Clone)] +#[derive(Copy, Clone)] pub struct PySetterDef { pub(crate) name: &'static CStr, pub(crate) meth: Setter, pub(crate) doc: &'static CStr, } -unsafe impl Sync for PyMethodDef {} - -unsafe impl Sync for PyGetterDef {} - -unsafe impl Sync for PySetterDef {} - impl PyMethodDef { /// Define a function with no `*args` and `**kwargs`. pub const fn noargs( @@ -172,8 +163,7 @@ impl PyMethodDef { self } - /// Convert `PyMethodDef` to Python method definition struct `ffi::PyMethodDef` - pub(crate) fn as_method_def(&self) -> ffi::PyMethodDef { + pub const fn into_raw(self) -> ffi::PyMethodDef { let meth = match self.ml_meth { PyMethodType::PyCFunction(meth) => ffi::PyMethodDefPointer { PyCFunction: meth }, PyMethodType::PyCFunctionWithKeywords(meth) => ffi::PyMethodDefPointer { @@ -737,10 +727,18 @@ mod tests { #[cfg(any(Py_3_10, not(Py_LIMITED_API)))] fn test_fastcall_function_with_keywords() { use super::PyMethodDef; - use crate::types::{PyAnyMethods, PyCFunction}; + use crate::impl_::pyfunction::PyFunctionDef; + use crate::types::PyAnyMethods; use crate::{ffi, Python}; Python::attach(|py| { + static DEF: PyFunctionDef = + PyFunctionDef::from_method_def(PyMethodDef::fastcall_cfunction_with_keywords( + ffi::c_str!("test"), + accepts_no_arguments, + ffi::c_str!("doc"), + )); + unsafe extern "C" fn accepts_no_arguments( _slf: *mut ffi::PyObject, _args: *const *mut ffi::PyObject, @@ -752,16 +750,7 @@ mod tests { unsafe { Python::assume_attached().None().into_ptr() } } - let f = PyCFunction::internal_new( - py, - &PyMethodDef::fastcall_cfunction_with_keywords( - ffi::c_str!("test"), - accepts_no_arguments, - ffi::c_str!("doc"), - ), - None, - ) - .unwrap(); + let f = DEF.create_py_c_function(py, None).unwrap(); f.call0().unwrap(); }); diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 75a0598e145..c743a5354ff 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -25,9 +25,9 @@ use crate::prelude::PyTypeMethods; use crate::PyErr; use crate::{ ffi, - impl_::pymethods::PyMethodDef, + impl_::pyfunction::PyFunctionDef, sync::PyOnceLock, - types::{PyCFunction, PyModule, PyModuleMethods}, + types::{PyModule, PyModuleMethods}, Bound, Py, PyClass, PyResult, PyTypeInfo, Python, }; @@ -203,9 +203,10 @@ impl PyAddToModule for AddClassToModule { } /// For adding a function to a module. -impl PyAddToModule for PyMethodDef { +impl PyAddToModule for PyFunctionDef { fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { - module.add_function(PyCFunction::internal_new(module.py(), self, Some(module))?) + // safety: self is static + module.add_function(self.create_py_c_function(module.py(), Some(module))?) } } diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs index 599acd5c898..beb4f67b78c 100644 --- a/src/pyclass/create_type_object.rs +++ b/src/pyclass/create_type_object.rs @@ -195,7 +195,7 @@ impl PyTypeBuilder { .add_setter(setter), PyMethodDefType::Method(def) | PyMethodDefType::Class(def) - | PyMethodDefType::Static(def) => self.method_defs.push(def.as_method_def()), + | PyMethodDefType::Static(def) => self.method_defs.push(def.into_raw()), // These class attributes are added after the type gets created by LazyStaticType PyMethodDefType::ClassAttribute(_) => {} PyMethodDefType::StructMember(def) => self.member_defs.push(*def), diff --git a/src/sealed.rs b/src/sealed.rs index 66b80840097..ce66511c15a 100644 --- a/src/sealed.rs +++ b/src/sealed.rs @@ -1,3 +1,4 @@ +use crate::impl_::pyfunction::PyFunctionDef; use crate::types::{ PyBool, PyByteArray, PyBytes, PyCapsule, PyComplex, PyDict, PyFloat, PyFrozenSet, PyList, PyMapping, PyMappingProxy, PyModule, PyRange, PySequence, PySet, PySlice, PyString, @@ -50,6 +51,7 @@ impl Sealed for Bound<'_, PyWeakrefReference> {} impl Sealed for AddTypeToModule {} impl Sealed for AddClassToModule {} impl Sealed for PyMethodDef {} +impl Sealed for PyFunctionDef {} impl Sealed for ModuleDef {} impl Sealed for PyNativeTypeInitializer {} diff --git a/src/types/function.rs b/src/types/function.rs index fa822b5faa0..d393528a897 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -1,13 +1,13 @@ use crate::ffi_ptr_ext::FfiPtrExt; +use crate::impl_::pyfunction::create_py_c_function; use crate::py_result_ext::PyResultExt; use crate::types::capsule::PyCapsuleMethods; -use crate::types::module::PyModuleMethods; use crate::{ ffi, impl_::pymethods::{self, PyMethodDef}, - types::{PyCapsule, PyDict, PyModule, PyString, PyTuple}, + types::{PyCapsule, PyDict, PyModule, PyTuple}, }; -use crate::{Bound, Py, PyAny, PyResult, Python}; +use crate::{Bound, PyAny, PyResult, Python}; use std::cell::UnsafeCell; use std::ffi::CStr; @@ -32,11 +32,11 @@ impl PyCFunction { doc: &'static CStr, module: Option<&Bound<'py, PyModule>>, ) -> PyResult> { - Self::internal_new( - py, - &PyMethodDef::cfunction_with_keywords(name, fun, doc), - module, - ) + let def = PyMethodDef::cfunction_with_keywords(name, fun, doc).into_raw(); + // FIXME: stop leaking the def + let def = Box::leak(Box::new(def)); + // Safety: def is static + unsafe { create_py_c_function(py, def, module) } } /// Create a new built-in function which takes no arguments. @@ -50,7 +50,11 @@ impl PyCFunction { doc: &'static CStr, module: Option<&Bound<'py, PyModule>>, ) -> PyResult> { - Self::internal_new(py, &PyMethodDef::noargs(name, fun, doc), module) + let def = PyMethodDef::noargs(name, fun, doc).into_raw(); + // FIXME: stop leaking the def + let def = Box::leak(Box::new(def)); + // Safety: def is static + unsafe { create_py_c_function(py, def, module) } } /// Create a new function from a closure. @@ -84,7 +88,7 @@ impl PyCFunction { let doc = doc.unwrap_or(ffi::c_str!("")); let method_def = pymethods::PyMethodDef::cfunction_with_keywords(name, run_closure::, doc); - let def = method_def.as_method_def(); + let def = method_def.into_raw(); let capsule = PyCapsule::new( py, @@ -104,34 +108,6 @@ impl PyCFunction { .cast_into_unchecked() } } - - #[doc(hidden)] - pub fn internal_new<'py>( - py: Python<'py>, - method_def: &PyMethodDef, - module: Option<&Bound<'py, PyModule>>, - ) -> PyResult> { - let (mod_ptr, module_name): (_, Option>) = if let Some(m) = module { - let mod_ptr = m.as_ptr(); - (mod_ptr, Some(m.name()?.unbind())) - } else { - (std::ptr::null_mut(), None) - }; - let def = method_def.as_method_def(); - - // FIXME: stop leaking the def - let def = Box::into_raw(Box::new(def)); - - let module_name_ptr = module_name - .as_ref() - .map_or(std::ptr::null_mut(), Py::as_ptr); - - unsafe { - ffi::PyCFunction_NewEx(def, mod_ptr, module_name_ptr) - .assume_owned_or_err(py) - .cast_into_unchecked() - } - } } static CLOSURE_CAPSULE_NAME: &CStr = ffi::c_str!("pyo3-closure"); From 8019927210e6f2d6843179ba6f5c0234d7baf59e Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 21 Sep 2025 19:17:57 +0100 Subject: [PATCH 2/3] newsfragment --- newsfragments/5459.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/5459.fixed.md diff --git a/newsfragments/5459.fixed.md b/newsfragments/5459.fixed.md new file mode 100644 index 00000000000..5061eeff98d --- /dev/null +++ b/newsfragments/5459.fixed.md @@ -0,0 +1 @@ +Stop leaking `PyMethodDef` instances inside `#[pyfunction]` macro generated code. From a34f47652b3657382d12213567bacefcdfc6c94f Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sun, 21 Sep 2025 19:28:07 +0100 Subject: [PATCH 3/3] coverage --- src/impl_/pyfunction.rs | 34 ++++++++++++++++++++++++++++++++++ src/impl_/pymethods.rs | 7 +++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/impl_/pyfunction.rs b/src/impl_/pyfunction.rs index 36de0473ae0..6bf7d4ad099 100644 --- a/src/impl_/pyfunction.rs +++ b/src/impl_/pyfunction.rs @@ -116,3 +116,37 @@ pub unsafe fn create_py_c_function<'py>( .cast_into_unchecked() } } + +#[cfg(test)] +#[cfg(feature = "macros")] +mod tests { + #[test] + fn test_wrap_pyfunction_forms() { + use crate::types::{PyAnyMethods, PyModule}; + use crate::{wrap_pyfunction, Python}; + + #[crate::pyfunction(crate = "crate")] + fn f() {} + + Python::attach(|py| { + let module = PyModule::new(py, "test_wrap_pyfunction_forms").unwrap(); + + let func = wrap_pyfunction!(f, module.clone()).unwrap(); + func.call0().unwrap(); + + let func = wrap_pyfunction!(f, &module).unwrap(); + func.call0().unwrap(); + + let module_borrowed = module.as_borrowed(); + + let func = wrap_pyfunction!(f, module_borrowed).unwrap(); + func.call0().unwrap(); + + let func = wrap_pyfunction!(f, &module_borrowed).unwrap(); + func.call0().unwrap(); + + let func = wrap_pyfunction!(f, py).unwrap(); + func.call0().unwrap(); + }); + } +} diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index fadfdb5ce7b..32abadcf8c4 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -732,12 +732,15 @@ mod tests { use crate::{ffi, Python}; Python::attach(|py| { - static DEF: PyFunctionDef = + let def = PyFunctionDef::from_method_def(PyMethodDef::fastcall_cfunction_with_keywords( ffi::c_str!("test"), accepts_no_arguments, ffi::c_str!("doc"), )); + // leak to make it 'static + // deliberately done at runtime to have coverage of `PyFunctionDef::from_method_def` + let def = Box::leak(Box::new(def)); unsafe extern "C" fn accepts_no_arguments( _slf: *mut ffi::PyObject, @@ -750,7 +753,7 @@ mod tests { unsafe { Python::assume_attached().None().into_ptr() } } - let f = DEF.create_py_c_function(py, None).unwrap(); + let f = def.create_py_c_function(py, None).unwrap(); f.call0().unwrap(); });