Repository navigation
Direct calls to PyObject_Del/PyObject_DEL are broken for --with-pydebug #47549
Description
| BPO | 3299 |
|---|---|
| Nosy | @birkenfeld, @jcea, @ncoghlan, @pitrou, @vstinner |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2010-07-28.20:59:21.114>
created_at = <Date 2008-07-06.14:20:43.049>
labels = ['interpreter-core', 'library', 'type-crash']
title = 'Direct calls to PyObject_Del/PyObject_DEL are broken for --with-pydebug'
updated_at = <Date 2010-07-28.20:59:21.113>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2010-07-28.20:59:21.113>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2010-07-28.20:59:21.114>
closer = 'vstinner'
components = ['Interpreter Core', 'Library (Lib)']
creation = <Date 2008-07-06.14:20:43.049>
creator = 'vstinner'
dependencies = []
files = ['15881', '15887']
hgrepos = []
issue_num = 3299
keywords = ['patch']
message_count = 31.0
messages = ['69331', '69341', '69345', '69349', '69351', '69357', '69636', '69637', '69638', '69639', '70043', '78740', '78741', '78743', '97739', '97774', '97776', '97778', '97779', '97780', '97781', '97792', '100321', '100352', '100398', '100424', '101421', '110677', '110688', '111759', '111847']
nosy_count = 9.0
nosy_names = ['effbot', 'georg.brandl', 'jcea', 'ncoghlan', 'Rhamphoryncus', 'pitrou', 'vstinner', 'timehorse', 'BreamoreBoy']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue3299'
versions = ['Python 2.6', 'Python 3.1', 'Python 2.7', 'Python 3.2']
Activity
vstinner commentedon Jul 6, 2008
"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) { ... }".
attribute is not initialized
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?
birkenfeld commentedon Jul 6, 2008
This seems to be new in trunk; 2.5.2 raises a TypeError.
vstinner commentedon Jul 6, 2008
@georg.brandl: The error is an Heisenbug. Yes, sometimes it doesn't
crash, but recheck in Valgrind ;-)
effbot commentedon Jul 6, 2008
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.)
birkenfeld commentedon Jul 6, 2008
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.
vstinner commentedon Jul 6, 2008
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).
vstinner commentedon Jul 14, 2008
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.
vstinner commentedon Jul 14, 2008
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.
vstinner commentedon Jul 14, 2008
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()!
vstinner commentedon Jul 14, 2008
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()).
effbot commentedon Jul 19, 2008
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.
ncoghlan commentedon Jan 1, 2009
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:
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).
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...
9 remaining items
pitrou commentedon Jan 14, 2010
The sre patch has been committed, thank you!
pitrou commentedon Jan 14, 2010
Yes, it would IMO be overkill.
vstinner commentedon Jan 14, 2010
Update pyexpat patch. As _curses_panel, the bug is raised on malloc() failure. The patch adds also a dummy test on ExternalEntityParserCreate().
vstinner commentedon Jan 14, 2010
Patch for cElementTree:
vstinner commentedon Mar 3, 2010
thread fix commited: r78610 (trunk), r78611 (py3k), r78612 (3.1).
Delay the backport to 2.6 after the 2.6.5 release.
vstinner commentedon Mar 3, 2010
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.
vstinner commentedon Mar 4, 2010
pitou> The sre patch has been committed, thank you!
Commit numbers: r77499 (trunk), r77500 (2.6), r77501 (py3k), r77502 (3.1).
vstinner commentedon Mar 4, 2010
Another bug, specific to Python3, was missed in the _sre module: fixed by r78664 (py3k) and r78665 (3.1).
vstinner commentedon Mar 21, 2010
Backport done in r79198 (2.6).
BreamoreBoy commentedon Jul 18, 2010
Loads of comments about backports, can this be closed or are there still any outstanding issues?
ncoghlan commentedon Jul 18, 2010
I believe the two attached patches still need to be checked.
vstinner commentedon Jul 28, 2010
I will open new issues for the two remaining patches.
vstinner commentedon Jul 28, 2010
Done: bpo-9402 for pyexpat and bpo-9403 for cElementTree.