Add PyWeakref_GetRef and use it in weakref wrappers. #4528
Conversation
ngoldbaum
force-pushed
the
pyweakref_getref
branch
from
b80e257 to
d7e4c95
Compare
ngoldbaum
force-pushed
the
pyweakref_getref
branch
from
0d1f651 to
dd8236d
Compare
|
@ngoldbaum, Cool that you looked at it. (Sorry for the slow response on my side, though) Everything looks good, except I'm wondering if it would be better to drop |
src/types/weakref/anyref.rs
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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
- remove this API immediately in 0.23
- 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.
There was a problem hiding this comment.
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(&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() | ||
| } |
There was a problem hiding this comment.
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.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
|
When the merge fails like that, how do you see what happened? I don't see anything on the merge queue page. |
|
I think it just timed out, CI was a bit saturated last night. |
* add FFI bindings and a compat definition for PyWeakref_GetRef * implement get_object method of weakref wrappers using PyWeakref_GetRef * add changelog * rename _ref argument to reference * mark PyWeakref_GetObject as deprecated * mark test as using deprecated API item * update docs to reference PyWeakref_GetRef semantics * remove weakref methods that return borrowed references * fix lints about unused imports
* add FFI bindings and a compat definition for PyWeakref_GetRef * implement get_object method of weakref wrappers using PyWeakref_GetRef * add changelog * rename _ref argument to reference * mark PyWeakref_GetObject as deprecated * mark test as using deprecated API item * update docs to reference PyWeakref_GetRef semantics * remove weakref methods that return borrowed references * fix lints about unused imports
…4590) * Add PyWeakref_GetRef and use it in weakref wrappers. (#4528) * deprecate weakref methods that return borrowed references --------- Co-authored-by: Nathan Goldbaum <[email protected]>
…4590) * Add PyWeakref_GetRef and use it in weakref wrappers. (#4528) * deprecate weakref methods that return borrowed references --------- Co-authored-by: Nathan Goldbaum <[email protected]>
Refs #4265. Also ping @SuperJappie08.
The compat definition is adapted from the C implementation in pythoncapi-compat.
See also my comment in the weakref issue about whether we should add a new API based on
PyWeakref_GetRefthat doesn't panic or return borrowed references.