Repository navigation
PyEval_GetLocals() leaks locals #118934
Description
Activity
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 af_localsdict in the frame to host the dictionary, but we do not anymore. Eachframe.f_localscall generates a new proxy andlocals()(orPyEval_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?
- Change
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?
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.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'sf_localsattribute or when callingPyFrame_GetLocals(f)on the result ofPyEval_GetFrame().That interoperability issue is avoided if
PyEval_GetLocals()caches and returns the result of callingPyFrame_GetLocals(f)on the result ofPyEval_GetFrame()(i.e. keeping its equivalence to Python-levelf_localsaccess) rather than having it start making independent snapshots the wayPyEval_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.I think it's better to keep the behavior of
PyEval_GetLocals()(return a dict, not a proxy).PyEval_GetLocals()should be equivalent tolocals()except for the borrowed reference.PyEval_GetFrameLocals()is strictly equivalent tolocals().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 dictChanging the semantics of
PyEval_GetLocals()is equivalent to changing the semantics oflocals()which would cause many backwards compatibility issues. It's our intention to keeplocals()as it is instead of returning a proxy likef_locals, and we should do the same withPyEval_GetLocals().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 emulatinglocals()orframe.f_localsat the Python level, since they both returned references to the same shared cache of the local variable bindings. (In gh-119893 I amended thePyEval_GetLocals()deprecation notice to point to bothPyEval_GetFrameLocalsand callingPyFrame_GetLocals()on the result ofPyEval_GetFrame()depending on which behaviour the caller actually wants)As written, aside from the reference to
PyEval_GetFrameLocalsin the sample code, PEP 667 bringsPyEval_GetLocals()down on theframe.f_localsside of things, withPyEval_GetFrameLocalsbeing introduced if you actually want alocals()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 ofPyEval_GetLocals()will be able to see any changes. Users ofPyFrame_GetLocals()andframe.f_localsaren't accessing the internal cache anymore, so they won't see any edits. By contrast, ifPyEval_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 usingPyFrame_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.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 thePyEval_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_GetLocalson the passed in frame reference, they're not going to be callingPyEval_GetLocals.I'll post new PRs bringing the docs and PEP into line with
PyEval_GetLocals()continuing to return a cached snapshot dict.- added 4 commits that reference this issue
on Jun 2, 2024 4 remaining items
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)
- added a commit that references this issue
on Jul 18, 2024 PyEval_GetLocalshas 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)Reacted by Petr Viktorin- moved this from Todo to Done in Release and Deferred blockers 🚫
on Jul 18, 2024 I think something went wrong with the behavior revert since the linked commit is now causing assertion errors in
gpgmetest 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.
Bug report
PyEval_GetLocals()is documented as returning a borrowed reference. It now returns a new reference, which causes callers to leak the local variables:cpython/Python/ceval.c
Lines 2478 to 2479 in 35c4361
cc @gaogaotiantian @markshannon
Linked PRs