GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-34080: Fix memory leak on parsing error by vstinner · Pull Request #8242 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Jul 11, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 [email protected]
Co-Authored-By: Xiang Zhang [email protected]

https://bugs.python.org/issue34080

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner vstinner added needs backport to 3.6 labels Jul 11, 2018
@vstinner
vstinner requested review from serhiy-storchaka and zhangyangyu July 11, 2018 09:24
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 11, 2018
@bedevere-bot bedevere-bot added the awaiting merge label Jul 11, 2018
@vstinner vstinner mentioned this pull request Jul 11, 2018
bpo-34080: Fix a memory leak in the compiler. #8222
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Jul 11, 2018
View reviewed changes
Comment thread Parser/parsetok.c Outdated

@serhiy-storchaka serhiy-storchaka Jul 11, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

PyObject_FREE is used in err_input.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jul 11, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I guess that you are asking me to replace PyObject_FREE with PyObject_Free(): done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Co-Authored-By: Xiang Zhang <[email protected]>
@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().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner changed the title bpo-34080: Fix memory leak in err_clear() bpo-34080: Fix memory leak on parsing error Jul 11, 2018
@zhangyangyu

zhangyangyu commented Jul 11, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
Only clear err->text in err_free()
fdf73c9
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zhangyangyu
zhangyangyu approved these changes Jul 11, 2018
View reviewed changes
@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 :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Jul 11, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pythonrun.c
@@ -1445,10 +1447,6 @@ err_input(perrdetail *err)
Py_XDECREF(w);
cleanup:

@serhiy-storchaka serhiy-storchaka Jul 11, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

goto cleanup is used even in branches when msg_obj is always NULL. Either replace them with return, or merge #8222.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jul 11, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't see anything wrong with calling Py_XDECREF(msg_obj) when msg_obj is NULL. It's a common try/finally-like pattern applied to the C language. Moreover, I wrote this PR to fix a real memory leak, not to cleanup the code. Don't hesitate to propose a new PR once the memory leak is fixed, if you want to clean up the code further. I prefer to write the shortest PR to be able to backport it to all branches.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Jul 11, 2018

Copy link
Copy Markdown
Member Author

I merged PR #8222.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@vstinner @serhiy-storchaka @zhangyangyu @the-knights-who-say-ni @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.