classification
Title: Direct calls to PyObject_Del/PyObject_DEL are broken for --with-pydebug
Type: crash Stage: patch review
Components: Interpreter Core, Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, Rhamphoryncus, effbot, georg.brandl, haypo, jcea, ncoghlan, pitrou, timehorse
Priority: normal Keywords: patch

Created on 2008-07-06 14:20 by haypo, last changed 2010-07-28 20:59 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
pyexpat_py_decref.patch haypo, 2010-01-14 18:46
celementtree_py_decref.patch haypo, 2010-01-14 22:24
Messages (31)
msg69331 - (view) Author: STINNER Victor (haypo) * (Python committer) 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) * (Python committer) Date: 2008-07-06 17:03
This seems to be new in trunk; 2.5.2 raises a TypeError.
msg69345 - (view) Author: STINNER Victor (haypo) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) 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 (haypo) * (Python committer) 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) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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) * (Python committer) Date: 2010-01-14 17:38
The sre patch has been committed, thank you!
msg97780 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-07-18 21:29
I believe the two attached patches still need to be checked.
msg111759 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-07-28 01:32
I will open new issues for the two remaining patches.
msg111847 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-07-28 20:59
> I will open new issues for the two remaining patches.

Done: #9402 for pyexpat and #9403 for cElementTree.
History
Date User Action Args
2010-07-28 20:59:21hayposetstatus: open -> closed
resolution: fixed
messages: + msg111847
2010-07-28 01:32:24hayposetmessages: + msg111759
2010-07-18 21:29:35ncoghlansetmessages: + msg110688
2010-07-18 19:35:52BreamoreBoysetnosy: + BreamoreBoy
messages: + msg110677
2010-04-22 10:35:37hayposetfiles: - _curses_panel_py_decref.patch
2010-04-22 10:35:31hayposetfiles: - thread_py_decref.patch
2010-03-21 13:14:00hayposetmessages: + msg101421
2010-03-04 22:02:07hayposetmessages: + msg100424
2010-03-04 16:49:46hayposetmessages: + msg100398
2010-03-03 21:58:46hayposetmessages: + msg100352
2010-03-03 00:51:56hayposetmessages: + msg100321
2010-01-14 23:20:09hayposetfiles: - pyobject_del.patch
2010-01-14 22:24:23hayposetfiles: + celementtree_py_decref.patch

messages: + msg97792
2010-01-14 18:46:05hayposetfiles: + pyexpat_py_decref.patch

messages: + msg97781
2010-01-14 17:57:52hayposetfiles: - _curses_panel.patch
2010-01-14 17:46:36pitrousetmessages: + msg97780
2010-01-14 17:38:22pitrousetfiles: - sre_py_decref-2.patch
2010-01-14 17:38:12pitrousetmessages: + msg97779
2010-01-14 17:35:15hayposetmessages: + msg97778
2010-01-14 17:33:02hayposetfiles: + _curses_panel_py_decref.patch
2010-01-14 17:10:39hayposetfiles: - sre_py_decref.patch
2010-01-14 17:10:34hayposetfiles: - _sre-2.patch
2010-01-14 17:10:24hayposetfiles: + sre_py_decref-2.patch

messages: + msg97776
2010-01-14 17:02:26pitrousetmessages: + msg97774
2010-01-14 14:52:11pitrousetnosy: + pitrou
versions: + Python 3.2, - Python 3.0

stage: patch review
2010-01-13 21:58:23hayposetmessages: + msg97739
2010-01-13 21:57:30hayposetfiles: + thread_py_decref.patch
2010-01-13 21:57:14hayposetfiles: + sre_py_decref.patch
2009-01-02 00:08:58ncoghlansetmessages: + msg78743
2009-01-01 23:47:17ncoghlansetassignee: effbot ->
versions: + Python 2.6, Python 3.0, Python 3.1
messages: + msg78741
components: + Interpreter Core
title: invalid object destruction in re.finditer() -> Direct calls to PyObject_Del/PyObject_DEL are broken for --with-pydebug
2009-01-01 23:44:46ncoghlansetnosy: + ncoghlan
messages: + msg78740
2008-09-27 14:26:26timehorsesetversions: + Python 2.7, - Python 2.6
2008-09-26 22:38:36timehorsesetnosy: + timehorse
2008-09-17 10:17:59jceasetnosy: + jcea
2008-07-19 17:57:59effbotsetpriority: critical -> normal
messages: + msg70043
2008-07-19 17:39:16Rhamphoryncussetnosy: + Rhamphoryncus
2008-07-14 02:38:21hayposetmessages: + msg69639
2008-07-14 02:31:12hayposetfiles: + pyobject_del.patch
messages: + msg69638
2008-07-14 02:10:16hayposetfiles: + _curses_panel.patch
2008-07-14 02:08:44hayposetfiles: - re_finditer.patch
2008-07-14 02:08:33hayposetfiles: + _sre-2.patch
messages: + msg69637
2008-07-14 01:40:01hayposetmessages: + msg69636
2008-07-06 22:44:04hayposetmessages: + msg69357
2008-07-06 19:34:41georg.brandlsetmessages: + msg69351
2008-07-06 18:32:27effbotsetmessages: + msg69349
2008-07-06 18:05:44hayposetmessages: + msg69345
2008-07-06 17:03:48georg.brandlsetpriority: critical
2008-07-06 17:03:43georg.brandlsetassignee: effbot
nosy: + effbot
2008-07-06 17:03:38georg.brandlsetnosy: + georg.brandl
messages: + msg69341
2008-07-06 14:20:43haypocreate