GeezerVison is showing github.com — `PyEval_GetLocals()` leaks locals · Issue #118934 · python/cpython
PyEval_GetLocals() leaks locals #118934
DescriptionBug report
PyEval_GetLocals() is documented as returning a borrowed reference. It now returns a new reference, which causes callers to leak the local variables:
|
PyObject *locals = _PyEval_GetFrameLocals(); |
|
return locals; |
cc @gaogaotiantian @markshannon
Linked PRs
Activitygaogaotiantian commented on May 11, 2024 This is tricky, because after PEP 669, the reference PyEval_GetLocals() gets, is the only reference, for the function-level cases. We used to have a f_locals dict in the frame to host the dictionary, but we do not anymore. Each frame.f_locals call generates a new proxy and locals() (or PyEval_GetLocals()) turns that to a dict. That's the only reference, we can't return a borrowed reference.
So we either
- Change
PyEval_GetLocals() to return a new reference, or
- Have a list in the frame object to host all the results from
PyEval_GetLocals() call (because each call could result in a different dict, it's a snapshot), and release those when the frame is released.
Which is the lesser of two evils? encukou commented on May 13, 2024 Per PEP 667:
PyEval_GetLocals() will be implemented roughly as follows:
PyObject *PyEval_GetLocals(void) {
PyFrameObject * = ...; // Get the current frame.
if (frame->_locals_cache == NULL) {
frame->_locals_cache = PyEval_GetFrameLocals();
}
return frame->_locals_cache;
}
As with all functions that return a borrowed reference, care must be taken to ensure that the reference is not used beyond the lifetime of the object.
i.e. a proxy should be created on first acccess, and then returned.
Is that not an option any more? gaogaotiantian commented on May 30, 2024 I had a fix in #119769. It's not the prettiest solution, but it should work. We don't have a plan to deprecate this API yet, but it should be discouraged to use PyEval_GetLocals() because it returns borrowed reference, so I guess we don't need the best structure for it. ncoghlan commented on Jun 1, 2024 It turns out there's an internal inconsistency in PEP 667 here. When writing the docs updates in #119893 I was going off https://peps.python.org/pep-0667/#pyeval-getlocals stating that the "proxy mapping" would be cached on the frame, and https://peps.python.org/pep-0667/#changes-to-existing-apis saying "The semantics of PyEval_GetLocals() is changed as it now returns a view of the frame locals, not a dictionary." (as accepted in https://github.com/python/peps/blob/134897bc1f610a1ca9e24dd8d7ab3053fa3520aa/peps/pep-0667.rst?plain=1#L172 )
I missed the subsequent definition in terms of PyEval_GetFrameLocals() in https://peps.python.org/pep-0667/#id3 or I would have postponed those docs changes until the intention was clarified.
Caching a snapshot (as suggested in the sample code) rather than a proxy instance (as suggested in the prose) would mean that updates made via the return value of PyEval_GetLocals() wouldn't be visible through the frame's f_locals attribute or when calling PyFrame_GetLocals(f) on the result of PyEval_GetFrame().
That interoperability issue is avoided if PyEval_GetLocals() caches and returns the result of calling PyFrame_GetLocals(f) on the result of PyEval_GetFrame() (i.e. keeping its equivalence to Python-level f_locals access) rather than having it start making independent snapshots the way PyEval_GetFrameLocals() does or keeping the old "make and update a shared snapshot" behaviour. We do get the reference cycle problem mentioned in the PEP (and the updated docs), but that seems better than having the old and new APIs not playing nice with each other. gaogaotiantian commented on Jun 1, 2024 I think it's better to keep the behavior of PyEval_GetLocals() (return a dict, not a proxy). PyEval_GetLocals() should be equivalent to locals() except for the borrowed reference. PyEval_GetFrameLocals() is strictly equivalent to locals().
This is consistent because in that way:
PyEval_GetLocals()
PyEval_GetGlobals()
PyEval_GetBuiltins()
are borrowed version of
PyEval_GetFrameLocals()
PyEval_GetFrameGlobals()
PyEval_GetFrameBuiltins()
to their python equivalent
locals()
globals()
# a way to get builtins dict
Changing the semantics of PyEval_GetLocals() is equivalent to changing the semantics of locals() which would cause many backwards compatibility issues. It's our intention to keep locals() as it is instead of returning a proxy like f_locals, and we should do the same with PyEval_GetLocals(). ncoghlan commented on Jun 1, 2024 We'll need to get confirmation from the SC then, since that's definitely not what the prose in the accepted PEP 667 says.
PyEval_GetLocals() never historically distinguished between whether it was emulating locals() or frame.f_locals at the Python level, since they both returned references to the same shared cache of the local variable bindings. (In gh-119893 I amended the PyEval_GetLocals() deprecation notice to point to both PyEval_GetFrameLocals and calling PyFrame_GetLocals() on the result of PyEval_GetFrame() depending on which behaviour the caller actually wants)
As written, aside from the reference to PyEval_GetFrameLocals in the sample code, PEP 667 brings PyEval_GetLocals() down on the frame.f_locals side of things, with PyEval_GetFrameLocals being introduced if you actually want a locals() style snapshot.
This approach makes sense, since using shared mutable storage is only beneficial if other consumers of that storage can see the changes that you make. If PyEval_GetLocals() keeps its Python 3.12 behaviour, then only other callers of PyEval_GetLocals() will be able to see any changes. Users of PyFrame_GetLocals() and frame.f_locals aren't accessing the internal cache anymore, so they won't see any edits. By contrast, if PyEval_GetLocals() returns a cached proxy instance, then changes will be visible to other consumers as expected (including the frame itself for callers that were also using PyFrame_LocalsToFast() in previous versions). (The original PEP 558 implementation did a bunch of tapdancing to let proxy instances still see values that only existed in the internal cache, and to replicate writes so they affected the cache as well, but actually doing that is sufficiently messy that I think tolerating the reference cycle is a better option)
Ensuring that PyEval_GetLocals() returns a dict isn't a significant concern, since that already isn't guaranteed in class scopes. ncoghlan commented on Jun 2, 2024 After further reflection, I've come around to the point of view that reverting PyEval_GetLocals() to its Python 3.12 behaviour isn't likely to be more disruptive than the alternative. That means as long as the SC are OK with it, the simplest resolution is to update the docs and the PEP text to align with the PyEval_GetFrameLocals() based implementation sketch rather than the other way around.
The cases that were concerning me were mainly tracing functions implemented in C, but those are going to be calling PyFrame_GetLocals on the passed in frame reference, they're not going to be calling PyEval_GetLocals.
I'll post new PRs bringing the docs and PEP into line with PyEval_GetLocals() continuing to return a cached snapshot dict. markshannon commented on Jun 5, 2024 I really think we should implement PyEval_GetLocals as described in the PEP.
https://peps.python.org/pep-0667/#id3 seems quite clear and unambiguous.
It seems that all the complication stems from wanting to return a dict instead of a FrameLocalsProxy.
What is the advantage of returning a dict? There are semantic differences from 3.12 in both cases. ncoghlan commented on Jul 10, 2024 This proved complex enough that it became a question for the Steering Council in python/steering-council#245 (the PEP text was internally inconsistent, with different sections suggesting different behaviour for PyEval_GetLocals() in 3.13+)
@Yhg1s I added the deferred blocker label (if we don't get this resolved for the final beta, it should definitely be resolved before the first release candidate) ncoghlan commented on Jul 18, 2024 PyEval_GetLocals has been reverted back to its Python 3.12 behaviour for 3.13rc1 (this was supposed to be in the b4 release, but it had only been merged to main when the release was cut)
mgorny commented on Aug 6, 2024 I think something went wrong with the behavior revert since the linked commit is now causing assertion errors in gpgme test suite, and I don't think upstream has done any changes to support 3.13 betas, so I doubt they've actually caused the regression. I've filed #122728.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|