Skip to content
New issue

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

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

Already on GitHub? # to your account

Add PyWeakref_GetRef and use it in weakref wrappers. #4528

Merged
merged 9 commits into from
Oct 2, 2024
2 changes: 2 additions & 0 deletions newsfragments/4528.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
* Added bindings for `pyo3_ffi::PyWeakref_GetRef` on Python 3.13 and newer and
`py03_ffi::compat::PyWeakref_GetRef` for older Python versions.
33 changes: 33 additions & 0 deletions pyo3-ffi/src/compat/py_3_13.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,36 @@ compat_function!(
Py_XNewRef(PyImport_AddModule(name))
}
);

compat_function!(
originally_defined_for(Py_3_13);

#[inline]
pub unsafe fn PyWeakref_GetRef(
reference: *mut crate::PyObject,
pobj: *mut *mut crate::PyObject,
) -> std::os::raw::c_int {
use crate::{
compat::Py_NewRef, PyErr_SetString, PyExc_TypeError, PyWeakref_Check,
PyWeakref_GetObject, Py_None,
};

if !reference.is_null() && PyWeakref_Check(reference) == 0 {
*pobj = std::ptr::null_mut();
PyErr_SetString(PyExc_TypeError, c_str!("expected a weakref").as_ptr());
return -1;
}
let obj = PyWeakref_GetObject(reference);
if obj.is_null() {
// SystemError if reference is NULL
*pobj = std::ptr::null_mut();
return -1;
}
if obj == Py_None() {
*pobj = std::ptr::null_mut();
return 0;
}
*pobj = Py_NewRef(obj);
1
}
);
9 changes: 8 additions & 1 deletion pyo3-ffi/src/weakrefobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,12 @@ extern "C" {
#[cfg_attr(PyPy, link_name = "PyPyWeakref_NewProxy")]
pub fn PyWeakref_NewProxy(ob: *mut PyObject, callback: *mut PyObject) -> *mut PyObject;
#[cfg_attr(PyPy, link_name = "PyPyWeakref_GetObject")]
pub fn PyWeakref_GetObject(_ref: *mut PyObject) -> *mut PyObject;
#[cfg_attr(
Py_3_13,
deprecated(note = "deprecated since Python 3.13. Use `PyWeakref_GetRef` instead.")
)]
pub fn PyWeakref_GetObject(reference: *mut PyObject) -> *mut PyObject;
#[cfg(Py_3_13)]
#[cfg_attr(PyPy, link_name = "PyPyWeakref_GetRef")]
pub fn PyWeakref_GetRef(reference: *mut PyObject, pobj: *mut *mut PyObject) -> c_int;
}
45 changes: 28 additions & 17 deletions src/types/weakref/anyref.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use crate::err::{DowncastError, PyResult};
use crate::ffi_ptr_ext::FfiPtrExt;
use crate::type_object::{PyTypeCheck, PyTypeInfo};
use crate::types::any::{PyAny, PyAnyMethods};
use crate::types::{
any::{PyAny, PyAnyMethods},
PyNone,
};
use crate::{ffi, Borrowed, Bound};

/// Represents any Python `weakref` reference.
Expand Down Expand Up @@ -34,7 +37,7 @@ pub trait PyWeakrefMethods<'py> {
/// Upgrade the weakref to a direct Bound object reference.
///
/// It is named `upgrade` to be inline with [rust's `Weak::upgrade`](std::rc::Weak::upgrade).
/// In Python it would be equivalent to [`PyWeakref_GetObject`].
/// In Python it would be equivalent to [`PyWeakref_GetRef`].
///
/// # Example
#[cfg_attr(
Expand Down Expand Up @@ -94,7 +97,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function panics is the current object is invalid.
/// If used propperly this is never the case. (NonNull and actually a weakref type)
///
/// [`PyWeakref_GetObject`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetObject
/// [`PyWeakref_GetRef`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetRef
/// [`weakref.ReferenceType`]: https://docs.python.org/3/library/weakref.html#weakref.ReferenceType
/// [`weakref.ref`]: https://docs.python.org/3/library/weakref.html#weakref.ref
fn upgrade_as<T>(&self) -> PyResult<Option<Bound<'py, T>>>
Expand Down Expand Up @@ -191,7 +194,7 @@ pub trait PyWeakrefMethods<'py> {
/// Upgrade the weakref to a direct Bound object reference unchecked. The type of the recovered object is not checked before downcasting, this could lead to unexpected behavior. Use only when absolutely certain the type can be guaranteed. The `weakref` may still return `None`.
///
/// It is named `upgrade` to be inline with [rust's `Weak::upgrade`](std::rc::Weak::upgrade).
/// In Python it would be equivalent to [`PyWeakref_GetObject`].
/// In Python it would be equivalent to [`PyWeakref_GetRef`].
///
/// # Safety
/// Callers must ensure that the type is valid or risk type confusion.
Expand Down Expand Up @@ -255,7 +258,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function panics is the current object is invalid.
/// If used propperly this is never the case. (NonNull and actually a weakref type)
///
/// [`PyWeakref_GetObject`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetObject
/// [`PyWeakref_GetRef`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetRef
/// [`weakref.ReferenceType`]: https://docs.python.org/3/library/weakref.html#weakref.ReferenceType
/// [`weakref.ref`]: https://docs.python.org/3/library/weakref.html#weakref.ref
unsafe fn upgrade_as_unchecked<T>(&self) -> Option<Bound<'py, T>> {
Expand Down Expand Up @@ -342,7 +345,7 @@ pub trait PyWeakrefMethods<'py> {
/// Upgrade the weakref to a exact direct Bound object reference.
///
/// It is named `upgrade` to be inline with [rust's `Weak::upgrade`](std::rc::Weak::upgrade).
/// In Python it would be equivalent to [`PyWeakref_GetObject`].
/// In Python it would be equivalent to [`PyWeakref_GetRef`].
///
/// # Example
#[cfg_attr(
Expand Down Expand Up @@ -402,7 +405,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function panics is the current object is invalid.
/// If used propperly this is never the case. (NonNull and actually a weakref type)
///
/// [`PyWeakref_GetObject`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetObject
/// [`PyWeakref_GetRef`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetRef
/// [`weakref.ReferenceType`]: https://docs.python.org/3/library/weakref.html#weakref.ReferenceType
/// [`weakref.ref`]: https://docs.python.org/3/library/weakref.html#weakref.ref
fn upgrade_as_exact<T>(&self) -> PyResult<Option<Bound<'py, T>>>
Expand Down Expand Up @@ -502,7 +505,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function returns `Some(Bound<'py, PyAny>)` if the reference still exists, otherwise `None` will be returned.
///
/// This function gets the optional target of this [`weakref.ReferenceType`] (result of calling [`weakref.ref`]).
/// It produces similair results to using [`PyWeakref_GetObject`] in the C api.
/// It produces similar results to using [`PyWeakref_GetRef`] in the C api.
///
/// # Example
#[cfg_attr(
Expand Down Expand Up @@ -553,7 +556,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function panics is the current object is invalid.
/// If used propperly this is never the case. (NonNull and actually a weakref type)
///
/// [`PyWeakref_GetObject`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetObject
/// [`PyWeakref_GetRef`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetRef
/// [`weakref.ReferenceType`]: https://docs.python.org/3/library/weakref.html#weakref.ReferenceType
/// [`weakref.ref`]: https://docs.python.org/3/library/weakref.html#weakref.ref
fn upgrade(&self) -> Option<Bound<'py, PyAny>> {
Expand All @@ -572,7 +575,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function returns `Some(Borrowed<'_, 'py, PyAny>)` if the reference still exists, otherwise `None` will be returned.
///
/// This function gets the optional target of this [`weakref.ReferenceType`] (result of calling [`weakref.ref`]).
/// It produces similair results to using [`PyWeakref_GetObject`] in the C api.
/// It produces similar results to using [`PyWeakref_GetObject`] in the C api.
///
/// # Example
#[cfg_attr(
Expand Down Expand Up @@ -644,7 +647,7 @@ pub trait PyWeakrefMethods<'py> {
/// This function returns `Bound<'py, PyAny>`, which is either the object if it still exists, otherwise it will refer to [`PyNone`](crate::types::PyNone).
///
/// This function gets the optional target of this [`weakref.ReferenceType`] (result of calling [`weakref.ref`]).
/// It produces similair results to using [`PyWeakref_GetObject`] in the C api.
/// It produces similar results to using [`PyWeakref_GetRef`] in the C api.
///
/// # Example
#[cfg_attr(
Expand Down Expand Up @@ -692,20 +695,17 @@ pub trait PyWeakrefMethods<'py> {
/// This function panics is the current object is invalid.
/// If used propperly this is never the case. (NonNull and actually a weakref type)
///
/// [`PyWeakref_GetObject`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetObject
/// [`PyWeakref_GetRef`]: https://docs.python.org/3/c-api/weakref.html#c.PyWeakref_GetRef
/// [`weakref.ReferenceType`]: https://docs.python.org/3/library/weakref.html#weakref.ReferenceType
/// [`weakref.ref`]: https://docs.python.org/3/library/weakref.html#weakref.ref
fn get_object(&self) -> Bound<'py, PyAny> {
// PyWeakref_GetObject does some error checking, however we ensure the passed object is Non-Null and a Weakref type.
self.get_object_borrowed().to_owned()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you'll need to keep this and tell clippy to ignore the warning on 0.22.4? Or I guess just backport the compat function too.

fn get_object(&self) -> Bound<'py, PyAny>;

/// Retrieve to a Borrowed object pointed to by the weakref.
///
/// This function returns `Borrowed<'py, PyAny>`, which is either the object if it still exists, otherwise it will refer to [`PyNone`](crate::types::PyNone).
///
/// This function gets the optional target of this [`weakref.ReferenceType`] (result of calling [`weakref.ref`]).
/// It produces similair results to using [`PyWeakref_GetObject`] in the C api.
/// It produces similar results to using [`PyWeakref_GetObject`] in the C api.
///
/// # Example
#[cfg_attr(
Expand Down Expand Up @@ -758,15 +758,26 @@ pub trait PyWeakrefMethods<'py> {
/// [`weakref.ref`]: https://docs.python.org/3/library/weakref.html#weakref.ref
#[track_caller]
// TODO: This function is the reason every function tracks caller, however it only panics when the weakref object is not actually a weakreference type. So is it this neccessary?
// TODO: we should deprecate this because it relies on the semantics of a deprecated C API item
Copy link
Member

Choose a reason for hiding this comment

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

Actually I think this is fundamentally unsafe even not on the freethreaded build because a GIL thread switch could theoretically remove the last strong reference, and using this borrow further would then be a use-after-free? So I think we have to

  1. remove this API immediately in 0.23
  2. backport a fix to 0.22.4 which adds an immediate deprecation to this API and leaks the object in question here. Pretty bad, but I think better than possible UAF. Most users should be using the higher-level APIs, I hope, so shouldn't be too breaking and is definitely necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a number of other functions that rely on the semantics of get_object_borrowed, so I deleted those as well for the same soundness reason.

fn get_object_borrowed(&self) -> Borrowed<'_, 'py, PyAny>;
}

impl<'py> PyWeakrefMethods<'py> for Bound<'py, PyWeakref> {
fn get_object_borrowed(&self) -> Borrowed<'_, 'py, PyAny> {
// PyWeakref_GetObject does some error checking, however we ensure the passed object is Non-Null and a Weakref type.
#![allow(deprecated)]
unsafe { ffi::PyWeakref_GetObject(self.as_ptr()).assume_borrowed_or_err(self.py()) }
.expect("The 'weakref' weak reference instance should be valid (non-null and actually a weakref reference)")
}

fn get_object(&self) -> Bound<'py, PyAny> {
let mut obj: *mut ffi::PyObject = std::ptr::null_mut();
match unsafe { ffi::compat::PyWeakref_GetRef(self.as_ptr(), &mut obj) } {
std::os::raw::c_int::MIN..=-1 => panic!("The 'weakref' weak reference instance should be valid (non-null and actually a weakref reference)"),
0 => PyNone::get(self.py()).to_owned().into_any(),
1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned(self.py()) },
}
}
}

#[cfg(test)]
Expand Down
12 changes: 11 additions & 1 deletion src/types/weakref/proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::err::PyResult;
use crate::ffi_ptr_ext::FfiPtrExt;
use crate::py_result_ext::PyResultExt;
use crate::type_object::PyTypeCheck;
use crate::types::any::PyAny;
use crate::types::{any::PyAny, PyNone};
use crate::{ffi, Borrowed, Bound, ToPyObject};

use super::PyWeakrefMethods;
Expand Down Expand Up @@ -184,9 +184,19 @@ impl PyWeakrefProxy {
impl<'py> PyWeakrefMethods<'py> for Bound<'py, PyWeakrefProxy> {
fn get_object_borrowed(&self) -> Borrowed<'_, 'py, PyAny> {
// PyWeakref_GetObject does some error checking, however we ensure the passed object is Non-Null and a Weakref type.
#![allow(deprecated)]
unsafe { ffi::PyWeakref_GetObject(self.as_ptr()).assume_borrowed_or_err(self.py()) }
.expect("The 'weakref.ProxyType' (or `weakref.CallableProxyType`) instance should be valid (non-null and actually a weakref reference)")
}

fn get_object(&self) -> Bound<'py, PyAny> {
let mut obj: *mut ffi::PyObject = std::ptr::null_mut();
match unsafe { ffi::compat::PyWeakref_GetRef(self.as_ptr(), &mut obj) } {
std::os::raw::c_int::MIN..=-1 => panic!("The 'weakref.ProxyType' (or `weakref.CallableProxyType`) instance should be valid (non-null and actually a weakref reference)"),
0 => PyNone::get(self.py()).to_owned().into_any(),
1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned(self.py()) },
}
}
}

#[cfg(test)]
Expand Down
12 changes: 11 additions & 1 deletion src/types/weakref/reference.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::err::PyResult;
use crate::ffi_ptr_ext::FfiPtrExt;
use crate::py_result_ext::PyResultExt;
use crate::types::any::PyAny;
use crate::types::{any::PyAny, PyNone};
use crate::{ffi, Borrowed, Bound, ToPyObject};

#[cfg(any(PyPy, GraalPy, Py_LIMITED_API))]
Expand Down Expand Up @@ -193,9 +193,19 @@ impl PyWeakrefReference {
impl<'py> PyWeakrefMethods<'py> for Bound<'py, PyWeakrefReference> {
fn get_object_borrowed(&self) -> Borrowed<'_, 'py, PyAny> {
// PyWeakref_GetObject does some error checking, however we ensure the passed object is Non-Null and a Weakref type.
#![allow(deprecated)]
unsafe { ffi::PyWeakref_GetObject(self.as_ptr()).assume_borrowed_or_err(self.py()) }
.expect("The 'weakref.ReferenceType' instance should be valid (non-null and actually a weakref reference)")
}

fn get_object(&self) -> Bound<'py, PyAny> {
let mut obj: *mut ffi::PyObject = std::ptr::null_mut();
match unsafe { ffi::compat::PyWeakref_GetRef(self.as_ptr(), &mut obj) } {
std::os::raw::c_int::MIN..=-1 => panic!("The 'weakref.ReferenceType' instance should be valid (non-null and actually a weakref reference)"),
0 => PyNone::get(self.py()).to_owned().into_any(),
1..=std::os::raw::c_int::MAX => unsafe { obj.assume_owned(self.py()) },
}
}
}

#[cfg(test)]
Expand Down
Loading