Repository navigation
_elementtree.TreeBuilder broken with a non-C-deriving element_factory #60293
Description
| BPO | 16089 |
|---|---|
| Nosy | @birkenfeld, @pitrou, @larryhastings, @tiran, @ezio-melotti, @skrah, @florentx, @mitya57 |
| 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 2012-10-04.17:59:30.043>
created_at = <Date 2012-09-30.02:07:42.686>
labels = ['extension-modules', 'expert-XML', 'type-crash', 'release-blocker']
title = '_elementtree.TreeBuilder broken with a non-C-deriving element_factory'
updated_at = <Date 2012-11-09.13:51:34.072>
user = 'https://github.com/tiran'bugs.python.org fields:
activity = <Date 2012-11-09.13:51:34.072>
actor = 'mitya57'
assignee = 'none'
closed = True
closed_date = <Date 2012-10-04.17:59:30.043>
closer = 'pitrou'
components = ['Extension Modules', 'XML']
creation = <Date 2012-09-30.02:07:42.686>
creator = 'christian.heimes'
dependencies = []
files = ['27355', '27377', '27378', '27392', '27393']
hgrepos = []
issue_num = 16089
keywords = ['patch', '3.3regression']
message_count = 25.0
messages = ['171603', '171605', '171619', '171625', '171629', '171630', '171642', '171644', '171738', '171739', '171742', '171746', '171748', '171749', '171751', '171752', '171753', '171778', '171829', '171831', '171981', '172009', '172014', '172019', '175235']
nosy_count = 11.0
nosy_names = ['georg.brandl', 'pitrou', 'larry', 'christian.heimes', 'ezio.melotti', 'Arfrever', 'eli.bendersky', 'skrah', 'flox', 'python-dev', 'mitya57']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue16089'
versions = ['Python 3.3', 'Python 3.4']
Activity
tiran commentedon Sep 30, 2012
The issue was reported by Arfrever on #python-dev. The test suite of simpletal [1] was segfaulting with Python 3.3. It doesn't segfault without the _elementtree C extension.
I'm able to reproduce the issue. It may take a couple of runs with a debug build of Python but eventually Python segfaults. So far segfaults occur inside a GC collection run. Benjamin suspects the issue is related to the new GC code in _elementtree.
[1] http://www.owlfish.com/software/simpleTAL/downloads/SimpleTAL-5.1.tar.gz
tiran commentedon Sep 30, 2012
The issue could be related to bpo-14007.
skrah commentedon Sep 30, 2012
I can't reproduce this, so just a wild guess: Does the segfault still
happen if you replace Py_XDECREF() with Py_CLEAR() in xmlparser_gc_clear()?
tiran commentedon Sep 30, 2012
No, it doesn't make a difference when I replace Py_XDECREF() with Py_CLEAR(). I've also replaced Py_(X)DECREF() in the other *_gc_clear() methods without success.
skrah commentedon Sep 30, 2012
I'm now able to reproduce the issue with a non-debug build. As
Christian says, using Py_CLEAR() does not help (though I wonder
if it shouldn't be used anyway).
Reverting part of 20b8f0ee3d64 "fixes" the segfault, see the
attached diff. I don't know the code well enough to say if this
is a valid possibility.
tiran commentedon Sep 30, 2012
I can confirm that Stefan's fix works. I ran the SimpleTAL test suite about hundred times in a loop without a segfault. But I don't understand why the change fixes the issue. Could the alteration just lead to another code path so the erroneous code isn't triggered?
+1 for Py_CLEAR()
elibendersky commentedon Sep 30, 2012
I'm currently somewhat "offline" for a while (cross-continental move), but I'll do my best to try to recreate my setup to test this problem and the proposed solution.
skrah commentedon Sep 30, 2012
Do note that the patch is somewhat cargo-cult. I don't understand yet why
it works; it may very well just silence the real problem.
birkenfeld commentedon Oct 1, 2012
Let's make sure this gets into 3.3.1.
pitrou commentedon Oct 1, 2012
Here is a collection of assorted small fixes for celementtree. There are probably other problems lurking (the coding style there is quite old).
I cannot say anything about the crasher until there's a simple reproducer :)
pitrou commentedon Oct 1, 2012
More assorted celementtree cleanups.
2 remaining items
pitrou commentedon Oct 1, 2012
Example of this is the following code in treebuilder_handle_start:
(note the overly optimistic cast)
but this is really a pervasive problem, since in many places TreeBuilder is hard-wired to use a Element instance and nothing else (despite the element_factory).
Note that simpleTAL cannot use the _elementtree.Element class, since their subclass also inherits from an exception subclass, and the object layouts conflict with each other (yeah, crappy design).
pitrou commentedon Oct 1, 2012
I'll still commit my cleanup patch in the meantime :-)
python-dev commentedon Oct 1, 2012
New changeset f9224f23f473 by Antoine Pitrou in branch '3.3':
Sanitize and modernize some of the _elementtree code (see issue bpo-16089).
http://hg.python.org/cpython/rev/f9224f23f473
New changeset 9fb0a8fc5d79 by Antoine Pitrou in branch 'default':
Sanitize and modernize some of the _elementtree code (see issue bpo-16089).
http://hg.python.org/cpython/rev/9fb0a8fc5d79
elibendersky commentedon Oct 1, 2012
Thank you, Antoine, for looking into this. I wish I could participate in a meaningful way, but alas it will be days or weeks before I can recreate a suitable setup to get back hacking on Python.
skrah commentedon Oct 2, 2012
Nice find. -- The Python version does this:
So (naively) I would think the same should be done for the C version
after importing Element.
But then one runs into the object layouts conflict that you mentioned.
On the other hand, in the original documentation direct use of
_ElementInterface was discouraged:
http://effbot.org/zone/pythondoc-elementtree-ElementTree.htm#elementtree.ElementTree.\_ElementInterface-class
pitrou commentedon Oct 2, 2012
Here is a patch. It still needs some tests.
[-]_elementtree causes segfault in GC[/-][+]_elementtree.TreeBuilder broken with a non-C-deriving element_factory[/+]pitrou commentedon Oct 2, 2012
Updated patch with tests.
python-dev commentedon Oct 4, 2012
New changeset 7bd9626d8b4f by Antoine Pitrou in branch '3.3':
Issue bpo-16089: Allow ElementTree.TreeBuilder to work again with a non-Element element_factory (fixes a regression in SimpleTAL).
http://hg.python.org/cpython/rev/7bd9626d8b4f
New changeset a8c044188731 by Antoine Pitrou in branch 'default':
Issue bpo-16089: Allow ElementTree.TreeBuilder to work again with a non-Element element_factory (fixes a regression in SimpleTAL).
http://hg.python.org/cpython/rev/a8c044188731
elibendersky commentedon Oct 4, 2012
Antoine, Thanks!
You said :
It would be great if you could elaborate, however briefly, if there's anything else besides your fixes that is old and should be modernized. I will admit to writing some of that code very recently, but that's mostly because I was trying to follow the existing coding style of the module, which was written a long time ago.
pitrou commentedon Oct 4, 2012
I don't think that's you. I was talking about some of the cruft I
removed (such as list_join() which was going through complicated hoops
instead of calling PyUnicode_Join(), or manual filling of tuples instead
of calling PyTuple_Pack()).
There are still issues with internal functions stealing references and
such.
elibendersky commentedon Oct 4, 2012
Ah, OK. I was actually putting off refactorings in that code to after refactoring the test suite. The latter is in progress, it was paused by my vacation but I do plan to resume it eventually. Once the test suite is sufficiently humane (esp. moves off doctest to enable normal testing of multiple modules with the same set of tests) the refactoring should become easier.
mitya57 commentedon Nov 9, 2012
There are still some false-positive warnings caused by C code that affect docutils (see http://sourceforge.net/tracker/?func=detail&atid=422030&aid=3555164&group_id=38414).
Look at this traceback for exmaple:
http://paste.ubuntu.com/1345164/
There, _ElementInterfaceWrapper is a subclass of etree._ElementInterface (http://repo.or.cz/w/docutils.git/blob/HEAD:/docutils/docutils/writers/odf_odt/__init__.py#l91).