Repository navigation
Descriptor resolution should own arguments passed to descriptors #90719
Description
| BPO | 46561 |
|---|---|
| Nosy | @rhettinger, @markshannon, @tekknolagi, @sweeneyde |
| PRs |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2022-01-28.07:06:14.782>
labels = ['expert-C-API', '3.8', '3.9', '3.10', '3.11']
title = 'Descriptor resolution should own arguments passed to descriptors'
updated_at = <Date 2022-02-01.15:21:47.460>
user = 'https://github.com/tekknolagi'bugs.python.org fields:
activity = <Date 2022-02-01.15:21:47.460>
actor = 'rhettinger'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['C API']
creation = <Date 2022-01-28.07:06:14.782>
creator = 'tekknolagi'
dependencies = []
files = []
hgrepos = []
issue_num = 46561
keywords = ['patch']
message_count = 7.0
messages = ['411978', '411979', '412035', '412040', '412049', '412250', '412277']
nosy_count = 4.0
nosy_names = ['rhettinger', 'Mark.Shannon', 'tekknolagi', 'Dennis Sweeney']
pr_nums = ['30979']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue46561'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']
Activity
tekknolagi commentedon Jan 28, 2022
Currently the descriptor (self) argument to __get__ is passed borrowed, since _PyType_LookupId returns a borrowed reference (see _PyObject_LookupSpecial and lookup_maybe_method in Objects/typeobject.c). This should instead own the reference.
sweeneyde commentedon Jan 28, 2022
Why? Callee-borrowing-from-caller is the established norm across the C API. You mention use-after-free, but can you elaborate on how that can happen in practice?
https://docs.python.org/3/extending/extending.html?highlight=borrowed#ownership-rules says:
"""When you pass an object reference into another function, in general, the function borrows the reference from you — if it needs to store it, it will use Py_INCREF() to become an independent owner."""
tekknolagi commentedon Jan 28, 2022
Hi Dennis,
Sorry, let me be more clear. CPython in general ensures that objects passed in as arguments to a function will live for the duration of the function call if they are otherwise untouched. As it is now, this invariant is not maintained when calling the __get__ descriptor. Right now, it is not only borrowed by the callee but also not owned by the caller (!).
Max
tekknolagi commentedon Jan 28, 2022
Ah, and another piece of the puzzle: this can happen in runtimes like Cinder that provide their own native code entrypoints to functions like a __get__.
sweeneyde commentedon Jan 29, 2022
Hi Max,
My apologies -- my first message was probably too dismissive.
Is there any way to make the existing code crash with pure Python, and can we then add such a test case? If not with pure Python, then perhaps with some minimal reproducible example using the C API?
Also, does the change have any effect on microbenchmarks involving the modified functions?
markshannon commentedon Feb 1, 2022
# This needs to be C code for this to fail, I'm writing it in Python for clarity and brevity
# This can be Python
C.d
rhettinger commentedon Feb 1, 2022
If this can only be triggered from C code, it less of a concern.
The PR increases cost on a critical path, so we ought to be wary of applying it unless a known problem is being solved.
Note, this code path has survived two decades of deployment. Also, the PR doesn't have a test to demonstrate that it fixes anything. Those facts give some evidence that the PR has every user paying to solve a problem that almost no one actually has.
rhettinger commentedon May 5, 2022
Marking this as closed. If another core dev wants to champion it, feel free to reopen.
serhiy-storchaka commentedon Jun 4, 2026
This was fixed in #118362.