Skip to content
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

Fix memory leak during argument parsing. #3592

Closed
wants to merge 1 commit into from

Conversation

jamadden
Copy link
Contributor

@jamadden jamadden commented May 8, 2020

The bug was on Python 2, or PyPy < 7.2

Fixes #3578

The bug was on Python 2, or PyPy < 7.2

Fixes cython#3578
Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

any tests need to be updated?

@jamadden
Copy link
Contributor Author

jamadden commented May 8, 2020

Yeah, ideally it could use tests; it didn't seem to have any before, though, and I'm not sure how to go about adding any.

Possibly even more ideally this would be integrated, somehow, with the refnanny system, which would probably just make tests happen. But that's beyond my familiarity with this code base. So this is just the minimal fix for now.

@da-woods
Copy link
Contributor

da-woods commented May 8, 2020

refnanny is mainly for the Cython generated code. It assumes that the utility code is right. So it wouldn't be the right thing to use here. I think it's probably OK for this to be test-less.

@da-woods
Copy link
Contributor

da-woods commented May 8, 2020

From the test failures I think PyObject_GetItem was right because it'll raise exceptions (and the PyDict version won't. I think the you probably want a decref to make the reference "borrowed" instead.

@scoder
Copy link
Contributor

scoder commented May 8, 2020

Oh well… #3091

@scoder
Copy link
Contributor

scoder commented May 8, 2020

I think it can be ok to add the refnanny also to some non-trivial utility functions, but this isn't a candidate.

@scoder
Copy link
Contributor

scoder commented May 8, 2020

I think the you probably want a decref to make the reference "borrowed" instead.

PyPy won't be happy about that. It's generally not happy about borrowed references, but this would be even worse.

I think the best fix would be to step away from borrowed references in the kwargs parsing code, but that's probably not for today any more.

@scoder
Copy link
Contributor

scoder commented May 8, 2020

I think the best fix would be to step away from borrowed references in the kwargs parsing code

Or, drop Py2 support. :)

@scoder
Copy link
Contributor

scoder commented May 9, 2020

Thanks for the analysis and the proposed fix. I found a more correct implementation in #3593.

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.

Possible memory leaks in Cython 3.0a1—a4
4 participants