leak references for safety in PyWeakRefMethods::upgrade_borrowed#4590
Merged
davidhewitt merged 2 commits intoPyO3:release-0.22.4from Oct 4, 2024
Merged
leak references for safety in PyWeakRefMethods::upgrade_borrowed#4590davidhewitt merged 2 commits intoPyO3:release-0.22.4from
PyWeakRefMethods::upgrade_borrowed#4590davidhewitt merged 2 commits intoPyO3:release-0.22.4from
Conversation
Merged
* 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
6ea1e1d to
675e505
Compare
2a89e17 to
f31271f
Compare
Member
Author
|
To get on with shipping 0.24 I will merge this now; I can't see a way we can do anything reasonable other than leak. |
maartenflippo
added a commit
to ConSol-Lab/Pumpkin
that referenced
this pull request
Oct 15, 2024
= ID: RUSTSEC-2024-0378 = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0378 = The family of functions to read "borrowed" values from Python weak references were fundamentally unsound, because the weak reference does itself not have ownership of the value. At any point the last strong reference could be cleared and the borrowed value would become dangling. In PyO3 0.22.4 these functions have all been deprecated and patched to leak a strong reference as a mitigation. PyO3 0.23 will remove these functions entirely. = Announcement: PyO3/pyo3#4590 = Solution: Upgrade to >=0.22.4 (try `cargo update -p pyo3`) = pyo3 v0.22.2 └── pumpkin-py v0.1.0
ImkoMarijnissen
pushed a commit
to ConSol-Lab/Pumpkin
that referenced
this pull request
Oct 16, 2024
= ID: RUSTSEC-2024-0378 = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0378 = The family of functions to read "borrowed" values from Python weak references were fundamentally unsound, because the weak reference does itself not have ownership of the value. At any point the last strong reference could be cleared and the borrowed value would become dangling. In PyO3 0.22.4 these functions have all been deprecated and patched to leak a strong reference as a mitigation. PyO3 0.23 will remove these functions entirely. = Announcement: PyO3/pyo3#4590 = Solution: Upgrade to >=0.22.4 (try `cargo update -p pyo3`) = pyo3 v0.22.2 └── pumpkin-py v0.1.0
|
It may be the lesser evil in this case, but I have found out the hard way that python lacks overflow checking on it's reference counts. So a reference count leak, repeated a sufficiently large number of times on the same object, can lead to a use after free. "sufficiently large number of times" is unlikely to happen on 64-bit platforms but is far more plausible on 32-bit platforms. |
Member
Author
|
Hmm, that's a good point. I mean this also just straight-up ensures a memory leak before the overflow too, so it's definitely unpleasant for users and they should move to the owned methods which don't have any of this mess. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: this is a patch fix for 0.22.4, the PR is targeting that branch, not
main.As per discussion in #4528, borrowing from a weakref is definitely unsafe and high risk for use-after-free; any arbitrary Python code could in theory remove the last strong reference.
In 0.23 we will straight-up remove these unsound methods, but in 0.22.4 we can't really do that so the mitigation proposed here is to switch to leak the upgraded reference when "borrowing" from the weakref. This avoids use-after-free in favour of the more gentle memory leak. The methods are also all deprecated as a nudge for users to shift away from them ASAP.