◐ Shell
reader mode source ↗
Skip to content

bpo-34080: Fix memory leak on parsing error#8242

Closed
vstinner wants to merge 2 commits into
python:masterfrom
vstinner:err_clear
Closed

bpo-34080: Fix memory leak on parsing error#8242
vstinner wants to merge 2 commits into
python:masterfrom
vstinner:err_clear

Conversation

@vstinner

@vstinner vstinner commented Jul 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member
  • bpo-34080, bpo-34084: err_free() now always releases 'text' memory
    allocated by PyObject_Malloc() to fix a memory leak on parsing
    error. Previously, err->text was not released (by err_input()) on
    E_ERROR error.
  • Remove also "with Barry as BDFL, use '<>' instead of '!='" static
    string error message (previously set to err_ret->text) because in
    practice, err_ret->text is always set later. Add also an assertion
    to make sure that err_ret->text is only set once.
  • Replace old PyObject_MALLOC() and PyObject_FREE() aliases with
    PyObject_Malloc() and PyObject_Free() function calls.

Co-Authored-By: Serhiy Storchaka storchaka@gmail.com
Co-Authored-By: Xiang Zhang angwerzx@126.com

https://bugs.python.org/issue34080

@vstinner vstinner mentioned this pull request Jul 11, 2018
Merged
@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

"./python -m test -j0 -R 3:3 test_compile test_grammar" pass with this change.

@serhiy-storchaka

serhiy-storchaka commented Jul 11, 2018

Copy link
Copy Markdown
Member

I have wrote the same fix, but not published it because failed with writing a test for bpo-34084. Could you please write a test?

Shouldn't you remove PyObject_FREE from err_input?

@serhiy-storchaka

serhiy-storchaka commented Jul 11, 2018

Copy link
Copy Markdown
Member

The commit message is misleading. The leak was caused by the bug in err_input().

@vstinner @serhiy-storchaka @zhangyangyu
bpo-34080: Fix memory leak on parsing error …
6224531 
* bpo-34080, bpo-34084: err_free() now always releases 'text' memory
  allocated by PyObject_Malloc() to fix a memory leak on parsing
  error.  Previously, err->text was not released (by err_input()) on
  E_ERROR error.
* Remove also "with Barry as BDFL, use '<>' instead of '!='" static
  string error message (previously set to err_ret->text) because in
  practice, err_ret->text is always set later. Add also an assertion
  to make sure that err_ret->text is only set once.
* Replace old PyObject_MALLOC() and PyObject_FREE() aliases with
  PyObject_Malloc() and PyObject_Free() function calls.

Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
Co-Authored-By: Xiang Zhang <angwerzx@126.com>
@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

I have wrote the same fix, but not published it because failed with writing a test for bpo-34084. Could you please write a test?

In fact, bpo-34084 is not a bug, just dead code: I changed my PR to remove dead code, but I also added an assertion to make sure that the 'text' field is only set once.

Shouldn't you remove PyObject_FREE from err_input?

It's harmless. I prefer to release memory as soon as possible.

The commit message is misleading. The leak was caused by the bug in err_input().

I used "git push --force" to rewrite the commit message.

Oops, I also fix my commit message: err_clear() => err_free().

@vstinner vstinner changed the title bpo-34080: Fix memory leak in err_clear() Jul 11, 2018
@zhangyangyu

zhangyangyu commented Jul 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Ahh, I don't read the entire code path carefully. It's just dead code. But I don't quite understand why put the cleaning logic two places. I know it's harmless, but it looks to me only the err_input part works and the err_free part is totally useless when using Serhiy's way.

@serhiy-storchaka

serhiy-storchaka commented Jul 11, 2018

Copy link
Copy Markdown
Member

I prefer to release memory as soon as possible.

Then it should be in err_input for E_ERROR too, isn't?

@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

Ok ok, I removed the code clearing the 'text' field from err_input() and marked its argument as constant to make it more explicit that err_input() intent is only to raise an exception, whereas err_free() is used to free memory.

@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

zhangyangyu approved these changes an hour ago

Thanks. But I would like to see @serhiy-storchaka opinion on the latest version of my PR :-)

serhiy-storchaka
serhiy-storchaka reviewed Jul 11, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I'm not convinced there is a need in such complication. Note that this PR changes public (although undocumented) C API: PyParser_SetError(). And it would be less easier to backport it to 2.7 than #8222.

@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

I'm not convinced there is a need in such complication.

Sorry, I fail to see how my change makes the code more complicated. Would you mind to elaborate?

Note that this PR changes public (although undocumented) C API: PyParser_SetError().

Sorry, what do you mean by "changing" the function? My PR doesn't change the behaviour of the function, it still raises a Python exception from a 'perrdetail' structure. The only changes is that with my change, it no longer releases the memory. Does it really matter when the memory is released?

There is PyParser_ClearError() which free the memory. Hum, sadly PyParser_ClearError is not documented neither. It seems like only high-level functions are exposed (documented), so this PR should not be noticed by anyone.

And it would be less easier to backport it to 2.7 than #8222.

This PR also fixes https://bugs.python.org/issue34084

Honestly, I don't understand why you are working so hard against this change: it's short, straighforward, and it fixes a real bug. Moreover, I already fixed multiple issues that you reported on previous versions of my change...

@serhiy-storchaka

serhiy-storchaka commented Jul 11, 2018

Copy link
Copy Markdown
Member

Sorry, I fail to see how my change makes the code more complicated. Would you mind to elaborate?

In comparison with a one-line change in #8222 which also fixes a real bug, it is much more complex.

The only changes is that with my change, it no longer releases the memory. Does it really matter when the memory is released?

This change is not necessary for fixing a real bug. As such, it should be discussed separately and made only in master.

This PR also fixes https://bugs.python.org/issue34084

Is there a reproducer for this bug? It looks to me that it can't be reproduced due to other bug. I think it would be better to fix the broken easter egg than remove the part of the broken code. There is no haste here, in contrary to fixing the memory leak.

@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

I merged PR #8222.

@vstinner vstinner closed this Jul 11, 2018
@vstinner vstinner deleted the err_clear branch July 11, 2018 21:18
@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

I wrote a simpler PR to fix https://bugs.python.org/issue34084: PR #8259.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@zhangyangyu zhangyangyu zhangyangyu approved these changes

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @serhiy-storchaka @zhangyangyu @the-knights-who-say-ni @bedevere-bot