Decref filepath only if the filename is not needed anymore #114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oschwald
requested changes
Jun 28, 2023
| @@ -111,6 +110,8 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { | |||
| return -1; | |||
| } | |||
|
|
|||
| Py_XDECREF(filepath); | |||
There was a problem hiding this comment.
Thanks! This makes sense, but I think we would leak above when there is an error opening the file. I think we would want a similar Py_XDECREF(filepath); before the return there.
There was a problem hiding this comment.
All right. Thanks for the superquick review. Should be fixed by the second decref now.
|
I released 2.4.0 with this change. |
|
Great! Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Filename is created by:
which means that filename contains only a pointer to the string in filepath and if the filepath is destroyed before we check the status, the filename cannot be used in the error message.
I have no idea why this happens to us only on i686 architecture with Python 3.12 but this change fixes the problem. Without it, the tests
test_nondatabasefail with non-matching regex for the expected error message: