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

vstinner · 2018-07-11T09:24:28Z

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 · 2018-07-11T09:37:59Z

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

serhiy-storchaka · 2018-07-11T09:43:08Z

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 · 2018-07-11T09:43:42Z

Parser/parsetok.c

@@ -322,7 +327,7 @@ parsetok(struct tok_state *tok, grammar *g, int start, perrdetail *err_ret,
            assert(tok->cur - tok->buf < INT_MAX);
            err_ret->offset = (int)(tok->cur - tok->buf);
            len = tok->inp - tok->buf;
-            err_ret->text = (char *) PyObject_MALLOC(len + 1);
+            err_ret->text = (char *) PyObject_Malloc(len + 1);


PyObject_FREE is used in err_input.

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

serhiy-storchaka · 2018-07-11T10:03:45Z

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

* 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 · 2018-07-11T10:30:05Z

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().

zhangyangyu · 2018-07-11T11:47:18Z

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 · 2018-07-11T11:50:15Z

I prefer to release memory as soon as possible.

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

vstinner · 2018-07-11T12:20:31Z

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 · 2018-07-11T14:00:50Z

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

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.

serhiy-storchaka · 2018-07-11T14:17:38Z

Python/pythonrun.c

@@ -1445,10 +1447,6 @@ err_input(perrdetail *err)
    Py_XDECREF(w);
 cleanup:


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

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.

vstinner · 2018-07-11T15:21:29Z

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 · 2018-07-11T15:43:54Z

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 · 2018-07-11T21:18:08Z

I merged PR #8222.

vstinner · 2018-07-11T21:23:24Z

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

vstinner added needs backport to 3.6 labels Jul 11, 2018

vstinner requested review from serhiy-storchaka and zhangyangyu July 11, 2018 09:24

the-knights-who-say-ni added the CLA signed label Jul 11, 2018

bedevere-bot added the awaiting merge label Jul 11, 2018

vstinner mentioned this pull request Jul 11, 2018

bpo-34080: Fix a memory leak in the compiler. #8222

Merged

serhiy-storchaka reviewed Jul 11, 2018

View reviewed changes

vstinner changed the title ~~bpo-34080: Fix memory leak in err_clear()~~ bpo-34080: Fix memory leak on parsing error Jul 11, 2018

Only clear err->text in err_free()

fdf73c9

zhangyangyu approved these changes Jul 11, 2018

View reviewed changes

serhiy-storchaka reviewed Jul 11, 2018

View reviewed changes

vstinner closed this Jul 11, 2018

vstinner deleted the err_clear branch July 11, 2018 21:18

		@@ -1445,10 +1447,6 @@ err_input(perrdetail *err)
		Py_XDECREF(w);
		cleanup:

Uh oh!

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

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

Uh oh!

Conversation

vstinner commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

serhiy-storchaka commented Jul 11, 2018

Uh oh!

serhiy-storchaka Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jul 11, 2018

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

zhangyangyu commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Jul 11, 2018

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

serhiy-storchaka commented Jul 11, 2018

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

vstinner commented Jul 11, 2018

Uh oh!

Uh oh!

vstinner commented Jul 11, 2018 •

edited

Loading

zhangyangyu commented Jul 11, 2018 •

edited

Loading