diff --git a/newsfragments/5941.changed.md b/newsfragments/5941.changed.md new file mode 100644 index 00000000000..fa8ac39fe2f --- /dev/null +++ b/newsfragments/5941.changed.md @@ -0,0 +1 @@ +Avoid temporary reference count cycles inside `PyAnyMethods::call`, `PyAnyMethods::call1` and `PyAnyMethods::call_method1` for arguments passed as Rust tuples containing borrowed references to Python objects. diff --git a/pyo3-benches/benches/bench_call.rs b/pyo3-benches/benches/bench_call.rs index 909e5761149..fa41bb12de5 100644 --- a/pyo3-benches/benches/bench_call.rs +++ b/pyo3-benches/benches/bench_call.rs @@ -33,14 +33,14 @@ fn bench_call_1(b: &mut Bencher<'_>) { let foo_module = &module.getattr("foo").unwrap(); let args = ( - 1.into_pyobject(py).unwrap(), + 1i32.into_pyobject(py).unwrap(), "s".into_pyobject(py).unwrap(), - 1.23.into_pyobject(py).unwrap(), + 1.23f64.into_pyobject(py).unwrap(), ); b.iter(|| { for _ in 0..1000 { - black_box(foo_module).call1(args.clone()).unwrap(); + black_box(foo_module).call1(&args).unwrap(); } }); }) @@ -52,17 +52,15 @@ fn bench_call(b: &mut Bencher<'_>) { let foo_module = &module.getattr("foo").unwrap(); let args = ( - 1.into_pyobject(py).unwrap(), + 1i32.into_pyobject(py).unwrap(), "s".into_pyobject(py).unwrap(), - 1.23.into_pyobject(py).unwrap(), + 1.23f64.into_pyobject(py).unwrap(), ); let kwargs = [("d", 1), ("e", 42)].into_py_dict(py).unwrap(); b.iter(|| { for _ in 0..1000 { - black_box(foo_module) - .call(args.clone(), Some(&kwargs)) - .unwrap(); + black_box(foo_module).call(&args, Some(&kwargs)).unwrap(); } }); }) @@ -77,7 +75,7 @@ fn bench_call_one_arg(b: &mut Bencher<'_>) { b.iter(|| { for _ in 0..1000 { - black_box(foo_module).call1((arg.clone(),)).unwrap(); + black_box(foo_module).call1((&arg,)).unwrap(); } }); }) @@ -116,17 +114,16 @@ class Foo: ); let foo_module = &module.getattr("Foo").unwrap().call0().unwrap(); + let meth = "foo".into_pyobject(py).unwrap(); let args = ( - 1.into_pyobject(py).unwrap(), + 1i32.into_pyobject(py).unwrap(), "s".into_pyobject(py).unwrap(), - 1.23.into_pyobject(py).unwrap(), + 1.23f64.into_pyobject(py).unwrap(), ); b.iter(|| { for _ in 0..1000 { - black_box(foo_module) - .call_method1("foo", args.clone()) - .unwrap(); + black_box(foo_module).call_method1(&meth, &args).unwrap(); } }); }) @@ -144,17 +141,19 @@ class Foo: ); let foo_module = &module.getattr("Foo").unwrap().call0().unwrap(); + let meth = "foo".into_pyobject(py).unwrap(); + let args = ( - 1.into_pyobject(py).unwrap(), + 1i32.into_pyobject(py).unwrap(), "s".into_pyobject(py).unwrap(), - 1.23.into_pyobject(py).unwrap(), + 1.23f64.into_pyobject(py).unwrap(), ); let kwargs = [("d", 1), ("e", 42)].into_py_dict(py).unwrap(); b.iter(|| { for _ in 0..1000 { black_box(foo_module) - .call_method("foo", args.clone(), Some(&kwargs)) + .call_method(&meth, &args, Some(&kwargs)) .unwrap(); } }); @@ -173,13 +172,12 @@ class Foo: ); let foo_module = &module.getattr("Foo").unwrap().call0().unwrap(); + let meth = "foo".into_pyobject(py).unwrap(); let arg = 1i32.into_pyobject(py).unwrap(); b.iter(|| { for _ in 0..1000 { - black_box(foo_module) - .call_method1("foo", (arg.clone(),)) - .unwrap(); + black_box(foo_module).call_method1(&meth, (&arg,)).unwrap(); } }); }) diff --git a/src/types/tuple.rs b/src/types/tuple.rs index acc8be104aa..29a35264021 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -7,6 +7,11 @@ use crate::internal_tricks::get_ssize_index; #[cfg(feature = "experimental-inspect")] use crate::type_object::PyTypeInfo; use crate::types::{sequence::PySequenceMethods, PyList, PySequence}; +#[cfg(all( + not(any(PyPy, GraalPy)), + any(all(Py_3_9, not(Py_LIMITED_API)), Py_3_12) +))] +use crate::BoundObject; use crate::{ exceptions, Bound, FromPyObject, IntoPyObject, IntoPyObjectExt, PyAny, PyErr, PyResult, Python, }; @@ -14,6 +19,12 @@ use std::iter::FusedIterator; #[cfg(feature = "nightly")] use std::num::NonZero; +#[cfg(all( + not(any(PyPy, GraalPy)), + any(all(Py_3_9, not(Py_LIMITED_API)), Py_3_12) +))] +use libc::size_t; + #[inline] #[track_caller] fn try_new_from_iter<'py>( @@ -657,14 +668,14 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ ) -> PyResult> { let py = function.py(); // We need this to drop the arguments correctly. - let args_bound = [$(self.$n.into_bound_py_any(py)?,)*]; + let args_objects = ($(self.$n.into_pyobject_or_pyerr(py)?),*,); // Prepend one null argument for `PY_VECTORCALL_ARGUMENTS_OFFSET`. - let mut args = [std::ptr::null_mut(), $(args_bound[$n].as_ptr()),*]; + let mut args = [std::ptr::null_mut(), $(args_objects.$n.as_ptr()),*]; unsafe { ffi::PyObject_VectorcallDict( function.as_ptr(), args.as_mut_ptr().add(1), - $length + ffi::PY_VECTORCALL_ARGUMENTS_OFFSET, + const { with_vectorcall_arguments_offset($length) }, kwargs.as_ptr(), ) .assume_owned_or_err(py) @@ -678,27 +689,26 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ _: crate::call::private::Token, ) -> PyResult> { let py = function.py(); - // We need this to drop the arguments correctly. - let args_bound = [$(self.$n.into_bound_py_any(py)?,)*]; + let args_objects = ($(self.$n.into_pyobject_or_pyerr(py)?),*,); #[cfg(not(Py_LIMITED_API))] if $length == 1 { return unsafe { ffi::PyObject_CallOneArg( function.as_ptr(), - args_bound[0].as_ptr() + args_objects.0.as_ptr() ) .assume_owned_or_err(py) }; } // Prepend one null argument for `PY_VECTORCALL_ARGUMENTS_OFFSET`. - let mut args = [std::ptr::null_mut(), $(args_bound[$n].as_ptr()),*]; + let mut args = [std::ptr::null_mut(), $(args_objects.$n.as_ptr()),*]; unsafe { ffi::PyObject_Vectorcall( function.as_ptr(), args.as_mut_ptr().add(1), - $length + ffi::PY_VECTORCALL_ARGUMENTS_OFFSET, + const { with_vectorcall_arguments_offset($length) }, std::ptr::null_mut(), ) .assume_owned_or_err(py) @@ -713,28 +723,27 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ _: crate::call::private::Token, ) -> PyResult> { let py = object.py(); - // We need this to drop the arguments correctly. - let args_bound = [$(self.$n.into_bound_py_any(py)?,)*]; + let args_objects = ($(self.$n.into_pyobject_or_pyerr(py)?),*,); #[cfg(not(Py_LIMITED_API))] if $length == 1 { return unsafe { ffi::PyObject_CallMethodOneArg( - object.as_ptr(), - method_name.as_ptr(), - args_bound[0].as_ptr(), + object.as_ptr(), + method_name.as_ptr(), + args_objects.0.as_ptr() ) .assume_owned_or_err(py) }; } - let mut args = [object.as_ptr(), $(args_bound[$n].as_ptr()),*]; + let mut args = [object.as_ptr(), $(args_objects.$n.as_ptr()),*]; unsafe { ffi::PyObject_VectorcallMethod( method_name.as_ptr(), args.as_mut_ptr(), // +1 for the receiver. - 1 + $length + ffi::PY_VECTORCALL_ARGUMENTS_OFFSET, + const { with_vectorcall_arguments_offset(1 + $length) }, std::ptr::null_mut(), ) .assume_owned_or_err(py) @@ -785,15 +794,14 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ _: crate::call::private::Token, ) -> PyResult> { let py = function.py(); - // We need this to drop the arguments correctly. - let args_bound = [$(self.$n.into_bound_py_any(py)?,)*]; + let args_objects = ($(self.$n.into_pyobject_or_pyerr(py)?),*,); // Prepend one null argument for `PY_VECTORCALL_ARGUMENTS_OFFSET`. - let mut args = [std::ptr::null_mut(), $(args_bound[$n].as_ptr()),*]; + let mut args = [std::ptr::null_mut(), $(args_objects.$n.as_ptr()),*]; unsafe { ffi::PyObject_VectorcallDict( function.as_ptr(), args.as_mut_ptr().add(1), - $length + ffi::PY_VECTORCALL_ARGUMENTS_OFFSET, + const { with_vectorcall_arguments_offset($length) }, kwargs.as_ptr(), ) .assume_owned_or_err(py) @@ -807,27 +815,26 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ _: crate::call::private::Token, ) -> PyResult> { let py = function.py(); - // We need this to drop the arguments correctly. - let args_bound = [$(self.$n.into_bound_py_any(py)?,)*]; + let args_objects = ($(self.$n.into_pyobject_or_pyerr(py)?),*,); #[cfg(not(Py_LIMITED_API))] if $length == 1 { return unsafe { ffi::PyObject_CallOneArg( function.as_ptr(), - args_bound[0].as_ptr() + args_objects.0.as_ptr() ) .assume_owned_or_err(py) }; } // Prepend one null argument for `PY_VECTORCALL_ARGUMENTS_OFFSET`. - let mut args = [std::ptr::null_mut(), $(args_bound[$n].as_ptr()),*]; + let mut args = [std::ptr::null_mut(), $(args_objects.$n.as_ptr()),*]; unsafe { ffi::PyObject_Vectorcall( function.as_ptr(), args.as_mut_ptr().add(1), - $length + ffi::PY_VECTORCALL_ARGUMENTS_OFFSET, + const { with_vectorcall_arguments_offset($length) }, std::ptr::null_mut(), ) .assume_owned_or_err(py) @@ -842,8 +849,7 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ _: crate::call::private::Token, ) -> PyResult> { let py = object.py(); - // We need this to drop the arguments correctly. - let args_bound = [$(self.$n.into_bound_py_any(py)?,)*]; + let args_objects = ($(self.$n.into_pyobject_or_pyerr(py)?),*,); #[cfg(not(Py_LIMITED_API))] if $length == 1 { @@ -851,19 +857,19 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ ffi::PyObject_CallMethodOneArg( object.as_ptr(), method_name.as_ptr(), - args_bound[0].as_ptr(), + args_objects.0.as_ptr(), ) .assume_owned_or_err(py) }; } - let mut args = [object.as_ptr(), $(args_bound[$n].as_ptr()),*]; + let mut args = [object.as_ptr(), $(args_objects.$n.as_ptr()),*]; unsafe { ffi::PyObject_VectorcallMethod( method_name.as_ptr(), args.as_mut_ptr(), // +1 for the receiver. - 1 + $length + ffi::PY_VECTORCALL_ARGUMENTS_OFFSET, + const { with_vectorcall_arguments_offset(1 + $length) }, std::ptr::null_mut(), ) .assume_owned_or_err(py) @@ -942,6 +948,18 @@ fn array_into_tuple<'py, const N: usize>( } } +/// Add `PY_VECTORCALL_ARGUMENTS_OFFSET` to the given number, checking for overflow at compile time. +/// +/// Guarantees that we don't accidentally overflow a `size_t` should this get changed in the future. +#[cfg(all( + not(any(PyPy, GraalPy)), + any(all(Py_3_9, not(Py_LIMITED_API)), Py_3_12) +))] +const fn with_vectorcall_arguments_offset(n: size_t) -> size_t { + n.checked_add(ffi::PY_VECTORCALL_ARGUMENTS_OFFSET) + .expect("overflow adding PY_VECTORCALL_ARGUMENTS_OFFSET") +} + tuple_conversion!(1, (ref0, 0, T0)); tuple_conversion!(2, (ref0, 0, T0), (ref1, 1, T1)); tuple_conversion!(3, (ref0, 0, T0), (ref1, 1, T1), (ref2, 2, T2));