Skip to content

gh-118605: Fix reference leak in FrameLocalsProxy #118607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 5, 2024

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented May 5, 2024

There are a couple of reference leaks in the implementation of PEP 667. Fix those and hopefully that's all.

@@ -306,7 +308,10 @@ framelocalsproxy_visit(PyObject *self, visitproc visit, void *arg)
static PyObject *
framelocalsproxy_iter(PyObject *self)
{
return PyObject_GetIter(framelocalsproxy_keys(self, NULL));
PyObject* keys = framelocalsproxy_keys(self, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should error-check this too, and the PyList_Append in that function could fail so should clean up and return NULL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there is a lack of error checking in some new "frameobject.c" functions. I will create a separate issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the framelocalsproxy_keys related checks. We can address the others in a seperate PR?

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we can merge this now and for the rest the separate PR.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, there's another error check on line 243 (PyList_New(0)). Sorry for the premature approval.

@bedevere-app
Copy link

bedevere-app bot commented May 5, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@gaogaotiantian
Copy link
Member Author

Right, I actually caught that while I was working on the PR to add proper checks, but I'm happy to make it in this PR.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have put that immediately after the PyObject *names = ... line, but this is fine. Good luck with the rest!

@gvanrossum gvanrossum enabled auto-merge (squash) May 5, 2024 21:12
@gaogaotiantian
Copy link
Member Author

I would have put that immediately after the PyObject *names = ... line, but this is fine. Good luck with the rest!

I put it after to make the declarations stick together because the other two declarations is quick and without any memory allocation.

I also understand why people would put the check immediately after the declaration, but in this specific case I'm leaning slightly more towards the current way.

@gvanrossum
Copy link
Member

Doing it immediately has some advantages for people used to reading through this code base. But since I already set up auto-commit, let's keep it as is.

@gaogaotiantian
Copy link
Member Author

I can polish it up in the next PR that does a bunch of fixes everywhere.

@gvanrossum gvanrossum merged commit b4f8eb0 into python:main May 5, 2024
34 of 35 checks passed
@gaogaotiantian gaogaotiantian deleted the fix-framelocals-ref-leak branch May 5, 2024 22:03
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants