homepage

"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?

This seems to be new in trunk; 2.5.2 raises a TypeError.

@georg.brandl: The error is an Heisenbug. Yes, sometimes it doesn't 
crash, but recheck in Valgrind ;-)

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

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

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

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.

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.

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

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

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.

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

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

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.

This issue is still open, it's still possible to crash Python in debug mode. I updated patches for the _sre and thread modules.

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
----------------------------------------------------------------------

> Victor, your overflow test in the sre patch tests for TypeError, 
> but OverflowError is actually raised: (...)
Oops, fixed.

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

The sre patch has been committed, thank you!

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

Update pyexpat patch. As _curses_panel, the bug is raised on malloc() failure. The patch adds also a dummy test on ExternalEntityParserCreate().

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.

thread fix commited: r78610 (trunk), r78611 (py3k), r78612 (3.1).
Delay the backport to 2.6 after the 2.6.5 release.

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.

pitou> The sre patch has been committed, thank you!
Commit numbers: r77499 (trunk), r77500 (2.6), r77501 (py3k), r77502 (3.1).

Another bug, specific to Python3, was missed in the _sre module: fixed by r78664 (py3k) and r78665 (3.1).

> thread fix commited: r78610 (trunk)
> curses panel fix commited: r78635 (trunk)
Backport done in r79198 (2.6).

Loads of comments about backports, can this be closed or are there still any outstanding issues?

I believe the two attached patches still need to be checked.

I will open new issues for the two remaining patches.

> I will open new issues for the two remaining patches.
Done: #9402 for pyexpat and #9403 for cElementTree.