Repository navigation
Try to print repr() when an C-level assert fails (in the garbage collector, beyond?) #53509
Description
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 2018-10-26.17:13:51.209>
created_at = <Date 2010-07-14.23:50:49.130>
labels = ['interpreter-core', 'type-feature']
title = 'Try to print repr() when an C-level assert fails (in the garbage collector, beyond?)'
updated_at = <Date 2018-11-22.16:40:57.760>
user = 'https://github.com/davidmalcolm'bugs.python.org fields:
activity = <Date 2018-11-22.16:40:57.760>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-10-26.17:13:51.209>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2010-07-14.23:50:49.130>
creator = 'dmalcolm'
dependencies = []
files = ['18007', '19777', '20343', '20344', '30532']
hgrepos = []
issue_num = 9263
keywords = ['patch']
message_count = 20.0
messages = ['110341', '122170', '125959', '125960', '131016', '131036', '190929', '328323', '328327', '328452', '328507', '328560', '328566', '328568', '328569', '328575', '328576', '330266', '330267', '330270']
nosy_count = 7.0
nosy_names = ['loewis', 'ncoghlan', 'pitrou', 'vstinner', 'dmalcolm', 'serhiy.storchaka', 'bkabrda']
pr_nums = ['10061', '10062', '10108', '10109', '10110', '10111', '10112', '10661', '10662', '10663']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue9263'
versions = ['Python 3.3']
Activity
davidmalcolm commentedon Jul 14, 2010
Modules/gcmodule.c contains various assertions which can fail due to reference counting errors elsewhere in either python, or an extension module. These can be difficult to track down.
In the hope of maximizing the information from crash reports, the attached patch (against py3k) introduces a new assertion macro to Objects/object.h and Objects.c, which provides a richer debug message. In particular, it identifies which object has the issue, and can more clearly spell out the problem.
The patch replaces all uses of assert() in Modules/gcmodule.c for which a specific object has an issue (e.g. bogus reference count).
The implementation may play somewhat fast-and-loose with rules about object invariants: you might have an only partially valid object, but the process is about to abort, so it seems acceptable to try to glean extra information on stderr. (This may turn an abort into a segfault, of course)
Caveats:
Thoughts?
davidmalcolm commentedon Nov 22, 2010
Attaching a simplified version of the patch; I got rid of the callbacks.
Still doesn't have test cases.
I suspect that the use of __STRING and __PRETTY_FUNCTION__ may be compatibility issues. I believe that __FILE__ and __LINE__ and standard C though.
davidmalcolm commentedon Jan 10, 2011
Attaching updated version of the patch.
I've added a selftest which (in a sacrificial subprocess) abuses ctypes to break an ob_refcnt, and then triggers a garbage collection.
I also changed the printing to stderr to directly use fprintf and fflush to ensure that data leaves the process before abort kills it (not sure if this is a cross-platform or unicode no-no, though).
davidmalcolm commentedon Jan 10, 2011
As above, but I added an extra call to fflush in case the call to _PyObject_Dump leads to a segfault.
pitrou commentedon Mar 15, 2011
How about turning these asserts into Py_FatalError()s and then enabling Victor's faulthandler extension?
ncoghlan commentedon Mar 15, 2011
I'd suggest calling Py_FatalError rather than calling abort() directly in _PyObject_AssertFailed, but otherwise this looks like a nice improvement over standard C asserts for state invariants that may be broken by buggy C extensions.
For the tests, take a look at test.script_helper - it provides some convenience wrappers for spawning subprocesses for tests that would cause problems if run in the current process.
bkabrda commentedon Jun 10, 2013
I'm currently patching Python 3.3.2 with this, so I thought it might be nice to attach an up-to-date patch. The only notable difference is that I added self.preclean() at the beginning of test_refcount_errors - without it, running test suite produced a huge number of these lines:
Exception AttributeError: "'GCCallbackTests' object has no attribute 'visit'" in <bound method GCCallbackTests.cb1 of <main.GCCallbackTests testMethod=test_refcount_errors>> ignored
vstinner commentedon Oct 23, 2018
New changeset 82af0b6 by Victor Stinner in branch 'master':
bpo-9263: _PyObject_Dump() detects freed memory (GH-10061)
82af0b6
vstinner commentedon Oct 23, 2018
00170-gc-assertions.patch is used in the Fedora package of Python.
I converted the patch to a pull request: PR 10062.
My PR only uses the new assertion macro once. I plan to write more changes to use the new assertion macros in more places, once the first PR is merged.
vstinner commentedon Oct 25, 2018
New changeset 626bff8 by Victor Stinner in branch 'master':
bpo-9263: Dump Python object on GC assertion failure (GH-10062)
626bff8
vstinner commentedon Oct 26, 2018
New changeset 3ec9af7 by Victor Stinner in branch 'master':
bpo-9263: _Py_NegativeRefcount() use _PyObject_AssertFailed() (GH-10109)
3ec9af7
vstinner commentedon Oct 26, 2018
New changeset 2470204 by Victor Stinner in branch 'master':
bpo-9263: Use _PyObject_ASSERT() in object.c (GH-10110)
2470204
vstinner commentedon Oct 26, 2018
New changeset a4b2bc7 by Victor Stinner in branch 'master':
bpo-9263: Use _PyObject_ASSERT() in gcmodule.c (GH-10112)
a4b2bc7
vstinner commentedon Oct 26, 2018
New changeset 0862505 by Victor Stinner in branch 'master':
bpo-9263: Use _PyObject_ASSERT() in typeobject.c (GH-10111)
0862505
vstinner commentedon Oct 26, 2018
New changeset 50fe3f8 by Victor Stinner in branch 'master':
bpo-9263: _PyXXX_CheckConsistency() use _PyObject_ASSERT() (GH-10108)
50fe3f8
vstinner commentedon Oct 26, 2018
I pushed the change and even more, so I consider that the issue can now be closed... 8 years later! Thank you very much Dave Malcolm for this nice idea, and for its implementation. Thanks Bohuslav "Slavek" Kabrda for the rebase in 2013, and thanks to my colleagues who rebased the patch frequently since 2013 in the Fedora package!
Maybe some people (like me?) want to use _PyObject_ASSERT() in more places, but I consider that we don't need to leave this issue open just for that.
I took the 00170-gc-assertions.patch rebased on Python 3.7.1 by my colleagues for the Fedora package, and I rebased it on master. I modified more functions in object.c and typeobject.c to use _PyObject_ASSERT(). I tried to not replace all assert(), but only when it's revelant.
I added code to detect if the object memory has been freed to avoid derefering 0xdbdbdbdbdbdbdbdb pointers which is very likely to cause a segmantation fault. It should reduce the risk of crash when dumping the faulty object.
Dave Malcolm:
I used Py_STRINGIFY() and __func__ in the final patch. __func__ is part of the C99 standard which is now required since Python 3.6: see PEP-7.
Dave Malcolm:
I reworked the unit test to not use ctypes, but write the crashing code in C instead.
Antoine Pitrou:
Done.
davidmalcolm commentedon Oct 26, 2018
Thanks!
vstinner commentedon Nov 22, 2018
New changeset 2cf5d32 by Victor Stinner in branch 'master':
bpo-9263: Fix _PyObject_Dump() for freed object (bpo-10661)
2cf5d32
vstinner commentedon Nov 22, 2018
New changeset 95036ea by Victor Stinner in branch '3.7':
[3.7] bpo-9263: _PyObject_Dump() detects freed memory (GH-10061) (GH-10662)
95036ea
vstinner commentedon Nov 22, 2018
New changeset c9b3fc6 by Victor Stinner in branch '3.6':
bpo-9263: _PyObject_Dump() detects freed memory (GH-10061) (GH-10662) (GH-10663)
c9b3fc6
pythongh-53509: Move _PyObject_ASSERT() to the internal C API
pythongh-53509: Move _PyObject_ASSERT() to the internal C API
pythongh-53509: Move _PyObject_ASSERT() to the internal C API