| msg69331 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2008-07-06 14:20 |
| "import re: re.finditer("a", {})" crash on Python 2.x (tested with svn trunk) because of an invalid use of PyObject_NEW/PyObject_DEL. The error is in pattern_scanner(), in block "if (!string) { ... }". - scanner_dealloc() calls Py_DECREF(self->pattern); whereas pattern attribute is not initialized - scanner_dealloc() calls PyObject_DEL(self); whereas scanner_dealloc() is already the destructor of self object! I wrote a patch to fix these errors. Please review my patch, I don't know C API very well. I also patched _compile(): replace PyObject_DEL() by Py_DECREF(). Is it really needed? |
|
|
| msg69341 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2008-07-06 17:03 |
| This seems to be new in trunk; 2.5.2 raises a TypeError. |
|
|
| msg69345 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2008-07-06 18:05 |
| @georg.brandl: The error is an Heisenbug. Yes, sometimes it doesn't crash, but recheck in Valgrind ;-) |
|
|
| msg69349 - (view) |
Author: Fredrik Lundh (effbot) * |
Date: 2008-07-06 18:32 |
| This report makes no sense to me; at least in Python 2.X, PyObject_Del removes a chunk of memory from the object heap. It's designed to be used from dealloc implementations, to release the actual memory (either directly, or as the default implementation for the tp_free slot). It can also be used in constructors, to destroy an object that was just created if something goes wrong. If you change PyObject_Del to Py_DECREF nillywilly, things will indeed crash. (with the original 2.5 code, I cannot see how a non-string argument to finditer() can result in a call to scanner_dealloc(); the argument will be rejected by getstring(), which causes state_init() to return, which causes pattern_scanner() to free the object it just created, and return.) |
|
|
| msg69351 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2008-07-06 19:34 |
| > It can also be used in constructors, to destroy an object that was just > created if something goes wrong. It appears that this is not true in debug builds: PyObject_NEW adds the object to the global linked list of all objects, which PyObject_DEL obviously doesn't undo. Therefore, when the object created immediately before is deallocated (in this case the argument tuple to finditer()), a fatal error will be caused. |
|
|
| msg69357 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2008-07-06 22:44 |
| Hum, the bug is maybe specific to Python build with --with-pydebug. I don't know exactly the behaviour changes of the PYDEBUG option. @effbot: in my patch, I replaced PyObject_DEL (PyObject_FREE) and not PyObject_Del (PyMem_Free). |
|
|
| msg69636 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2008-07-14 01:39 |
| Valgrind output for Python trunk compiled with pydebug option: ==29848== Invalid read of size 4 ==29848== at 0x809AF61: _Py_ForgetReference (object.c:2044) ==29848== by 0x809AFCF: _Py_Dealloc (object.c:2065) ==29848== by 0x80FE635: call_function (ceval.c:3653) ==29848== by 0x80F9C83: PyEval_EvalFrameEx (ceval.c:2350) ==29848== by 0x80FC2D1: PyEval_EvalCodeEx (ceval.c:2914) ==29848== by 0x80FEAFE: fast_function (ceval.c:3747) ==29848== by 0x80FE75F: call_function (ceval.c:3672) ==29848== by 0x80F9C83: PyEval_EvalFrameEx (ceval.c:2350) ==29848== by 0x80FC2D1: PyEval_EvalCodeEx (ceval.c:2914) ==29848== by 0x80F1219: PyEval_EvalCode (ceval.c:495) ==29848== by 0x812838E: run_mod (pythonrun.c:1330) ==29848== by 0x8128324: PyRun_FileExFlags (pythonrun.c:1316) ==29848== Address 0x4475680 is 8 bytes inside a block of size 896 free'd ==29848== at 0x402237F: free (vg_replace_malloc.c:233) ==29848== by 0x809C51D: PyObject_Free (obmalloc.c:1114) ==29848== by 0x809C86E: _PyObject_DebugFree (obmalloc.c:1361) ==29848== by 0x814ECBD: pattern_scanner (_sre.c:3371) ==29848== by 0x814C245: pattern_finditer (_sre.c:2148) ==29848== by 0x81708D6: PyCFunction_Call (methodobject.c:81) ==29848== by 0x80FE5C9: call_function (ceval.c:3651) ==29848== by 0x80F9C83: PyEval_EvalFrameEx (ceval.c:2350) ==29848== by 0x80FC2D1: PyEval_EvalCodeEx (ceval.c:2914) ==29848== by 0x80FEAFE: fast_function (ceval.c:3747) ==29848== by 0x80FE75F: call_function (ceval.c:3672) ==29848== by 0x80F9C83: PyEval_EvalFrameEx (ceval.c:2350) Fatal Python error: UNREF invalid object The error comes from invalid use of PyObject_DEL(): as said by georg.brandl, "PyObject_NEW adds the object to the global linked list of all objects, which PyObject_DEL obviously doesn't undo". An invalid object will stay in the object list until it is used and then Python crash. |
|
|
| msg69637 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2008-07-14 02:08 |
| F*ck, Firefox just crashed! I have to rewrite my long comment... First, to explain why the problem only occurs in pydebug mode: a PyObject has only _ob_next and _ob_prev attributes if Py_TRACE_REFS is set (eg. by pydebug). PyObject_NEW() and PyObject_NEW_VAR() call PyObject_Init() which register the new object to the object list, whereas PyObject_DEL() only free the memory. PyObject_DEL() doesn't the dealloc() method nor removing the object from the object list. So Py_DECREF() should be used instead: it calls the dealloc method and remove the object from the object list. PyObject_DEL() is misused in _sre and in curses modules. See attached patches. |
|
|
| msg69638 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2008-07-14 02:31 |
| Other examples of invalid use of PyObject_Del(). Don't apply the patch, i didn't check it. Eg. element_dealloc() should crash if self->tag is NULL... and at line 331, self->tag is NULL whereas I called element_dealloc() using Py_DECREF()! |
|
|
| msg69639 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2008-07-14 02:38 |
| About _curses_panel.patch: same as pyobject_del.patch, i didn't tested the code, and is looks like PyCursesPanel_Dealloc() expects that the object is properly initialized. It looks like this bug (invalid use of PyObject_Del/PyObject_DEL) is not an easy job and may changes many lines of code to fix it. Instead of replacing PyObjectDel/PyObjectDEL by Py_DECREF, another solution would be to patch PyObjectDel/PyObjectDEL for the case when Py_TRACE_REFS is defined (and then call _Py_ForgetReference()). |
|
|
| msg70043 - (view) |
Author: Fredrik Lundh (effbot) * |
Date: 2008-07-19 17:57 |
| Reducing priority to normal; this bug has been around since Python 2.2, and only affects code that doesn't work anyway when running on debug builds. |
|
|
| msg78740 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2009-01-01 23:44 |
| It certainly looks like all direct calls to PyObject_DEL/PyObject_Del from outside tp_dealloc implementations are going to be broken in pydebug builds. Replacing those calls with either Py_DECREF operations (as Victor's patch does) or direct calls to _Py_Dealloc should get them to do the right thing. I'm a little wary of adding calls to _Py_ForgetReference into PyObject_Del though, since most of the time the reference will already have been forgotten by the time that PyObject_Del is called. My suggestion would be: 1. Update the docs on PyObject_Del to specifically warn against using it for error cleanup after a successful PyObject_New invocation, instead pointing users of the C API to Py_DECREF. Basically, code that calls PyObject_Del outside a tp_dealloc method is probably doing something wrong. Note also that tp_dealloc implementations in such cases will need to cope with instances that are only partially initialised (e.g. by using Py_XDECREF instead of Py_DECREF). 2. Fix the instances in the standard library where _Py_Dealloc may be bypassed by direct calls to PyObject_Del. The alternative would be to change the pydebug version of PyObject_Del to check if the reference count on the object is 1 rather than 0. That should be a pretty good indication that we're dealing with a case of cleaning up after a failure to fully initialise an object, and we can call _Py_ForgetReference directly from PyObject_Del, retroactively making all those direct invocations of PyObject_Del "correct". Hmm, now that I write that idea down, it doesn't sound nearly as scary as I thought it would... |
|
|
| msg78741 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2009-01-01 23:47 |
| (changing title and unassigning from Fredrik since this isn't an RE specific problem, flagged as also affecting interpreter core, flagged as affecting all currently maintained versions) |
|
|
| msg78743 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2009-01-02 00:08 |
| Ah, now that I go to implement it, I remember why I didn't like the idea of doing anything directly in PyObject_Del. While the Python memory allocator is primarily designed for allocation of Python objects, it isn't actually *limited* to that. So there is no guarantee that the void pointer passed in to PyObject_Del can be safely cast to a PyObject pointer. The idea could still be implemented by scanning the list of active objects to see if the passed in pointer refers to one of them, but that would make object deallocation in pydebug builds extraordinarily slow. |
|
|
| msg97739 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-01-13 21:58 |
| This issue is still open, it's still possible to crash Python in debug mode. I updated patches for the _sre and thread modules. |
|
|
| msg97774 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-01-14 17:02 |
| Victor, your overflow test in the sre patch tests for TypeError, but OverflowError is actually raised: ====================================================================== ERROR: test_dealloc (test.test_re.ReTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/antoine/cpython/debug/Lib/test/test_re.py", line 711, in test_dealloc self.assertRaises(TypeError, _sre.compile, "abc", 0, [long_overflow]) File "/home/antoine/cpython/debug/Lib/unittest/case.py", line 394, in assertRaises callableObj(*args, **kwargs) OverflowError: regular expression code size limit exceeded ---------------------------------------------------------------------- |
|
|
| msg97776 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-01-14 17:10 |
| > Victor, your overflow test in the sre patch tests for TypeError, > but OverflowError is actually raised: (...) Oops, fixed. |
|
|
| msg97778 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-01-14 17:35 |
| Update _curses_panel patch. The crash occurs if malloc() fail in insert_lop(). I don't know how to write reliable test for this case. Maybe using http://www.nongnu.org/failmalloc/ library (a little bit overkill, isn't it?). |
|
|
| msg97779 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-01-14 17:38 |
| The sre patch has been committed, thank you! |
|
|
| msg97780 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-01-14 17:46 |
| > I don't know how to write reliable test for this case. Maybe using > http://www.nongnu.org/failmalloc/ library (a little bit overkill, isn't > it?). Yes, it would IMO be overkill. |
|
|
| msg97781 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-01-14 18:46 |
| Update pyexpat patch. As _curses_panel, the bug is raised on malloc() failure. The patch adds also a dummy test on ExternalEntityParserCreate(). |
|
|
| msg97792 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-01-14 22:24 |
| Patch for cElementTree: * Replace PyObject_Del() by Py_DECREF() * Catch element_new_extra() errors * parser dealloc: replace Py_DECREF() by Py_XDECREF() because the pointer may be NULL (error in the constructor) * set all parser attributes to NULL at the beginning of the constructor to be able to call safetly the destructor * element_new(): define tag, text, tail attributes before calling element_new_extra() to be able to call the destructor * raise a MemoryError on element_new_extra() failure. element_new() didn't raise any error on element_new_extra() failure. Other functions just forget to catch element_new_extra() error. |
|
|
| msg100321 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-03-03 00:51 |
| thread fix commited: r78610 (trunk), r78611 (py3k), r78612 (3.1). Delay the backport to 2.6 after the 2.6.5 release. |
|
|
| msg100352 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-03-03 21:58 |
| curses panel fix commited: r78635 (trunk), r78636 (py3k), r78637 (3.1). I will backport the fix to Python 2.6 after the 2.6.5 release. |
|
|
| msg100398 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-03-04 16:49 |
| pitou> The sre patch has been committed, thank you! Commit numbers: r77499 (trunk), r77500 (2.6), r77501 (py3k), r77502 (3.1). |
|
|
| msg100424 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-03-04 22:02 |
| Another bug, specific to Python3, was missed in the _sre module: fixed by r78664 (py3k) and r78665 (3.1). |
|
|
| msg101421 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-03-21 13:14 |
| > thread fix commited: r78610 (trunk) > curses panel fix commited: r78635 (trunk) Backport done in r79198 (2.6). |
|
|
| msg110677 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2010-07-18 19:35 |
| Loads of comments about backports, can this be closed or are there still any outstanding issues? |
|
|
| msg110688 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2010-07-18 21:29 |
| I believe the two attached patches still need to be checked. |
|
|
| msg111759 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-07-28 01:32 |
| I will open new issues for the two remaining patches. |
|
|
| msg111847 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-07-28 20:59 |
| > I will open new issues for the two remaining patches. Done: #9402 for pyexpat and #9403 for cElementTree. |
|
|