Segfault during deallocation of `_elementtree.XMLParser`

[mgorny]

Crash report

What happened?

I've originally hit it while running slixmpp's test suite. I've been able to reduce it to the following program:

import asyncio
import xml.etree.ElementTree as ET
from typing import Optional


def set_see_other_host() -> Optional[ET.Element]:
    pass


class XMLStream:
    def disconnect(self):
        asyncio.ensure_future(self._end_stream_wait())

    async def _end_stream_wait(self):
        await asyncio.wait_for(asyncio.Future(), 1)

    def _remove_schedules(self):
        pass


parser = ET.XMLPullParser()

xmpp = XMLStream()
foo = xmpp._remove_schedules
xmpp.disconnect()
loop = asyncio.get_event_loop()
loop.run_until_complete(asyncio.Queue().join())

I've been able to reproduce this with 3.12.0, and the tips of 3.12 and main branches. I've been able to bisect it to the following commit:

commit 1b5a2b085c28d230c9ef9bd9b472afc85e087ced
Author:     Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
AuthorDate: 2023-05-17 01:35:07 +0200
Commit:     GitHub <noreply@github.com>
CommitDate: 2023-05-17 01:35:07 +0200

    GH-103092: isolate `_elementtree` (#104561)

Backtrace:

(gdb) bt
#0  0x00007f9da2d8611c in xmlparser_gc_clear (self=0x7f9da2617760) at ./Modules/_elementtree.c:3784
#1  0x00007f9da2d86857 in xmlparser_dealloc (self=0x7f9da2617760) at ./Modules/_elementtree.c:3809
#2  0x0000559a2edd2145 in _Py_Dealloc (op=0x7f9da2617760) at Objects/object.c:2858
#3  0x0000559a2edae16d in Py_DECREF (op=0x7f9da2617760) at ./Include/object.h:879
#4  Py_XDECREF (op=0x7f9da2617760) at ./Include/object.h:972
#5  0x0000559a2edbbadb in _PyObject_FreeInstanceAttributes (self=0x7f9da3146d20) at Objects/dictobject.c:5646
#6  0x0000559a2ee03a71 in subtype_dealloc (self=0x7f9da3146d20) at Objects/typeobject.c:2064
#7  0x0000559a2edd2145 in _Py_Dealloc (op=0x7f9da3146d20) at Objects/object.c:2858
#8  0x0000559a2edae16d in Py_DECREF (op=0x7f9da3146d20) at ./Include/object.h:879
#9  Py_XDECREF (op=0x7f9da3146d20) at ./Include/object.h:972
#10 0x0000559a2edafbef in free_keys_object (interp=0x559a2f2a73a8 <_PyRuntime+76872>, keys=0x7f9da26c9ac0) at Objects/dictobject.c:675
#11 0x0000559a2edaef84 in dictkeys_decref (interp=0x559a2f2a73a8 <_PyRuntime+76872>, dk=0x7f9da26c9ac0) at Objects/dictobject.c:335
#12 0x0000559a2edb31e2 in PyDict_Clear (op=0x7f9da322ba00) at Objects/dictobject.c:2120
#13 0x0000559a2edb7a1e in dict_tp_clear (op=0x7f9da322ba00) at Objects/dictobject.c:3588
#14 0x0000559a2efb69ec in delete_garbage (tstate=0x559a2f30ce28 <_PyRuntime+493256>, gcstate=0x559a2f2a7798 <_PyRuntime+77880>, 
    collectable=0x7ffffccdee70, old=0x559a2f2a77e0 <_PyRuntime+77952>) at Modules/gcmodule.c:1033
#15 0x0000559a2efb707c in gc_collect_main (tstate=0x559a2f30ce28 <_PyRuntime+493256>, generation=2, n_collected=0x0, 
    n_uncollectable=0x0, nofail=1) at Modules/gcmodule.c:1313
#16 0x0000559a2efb8a1e in _PyGC_CollectNoFail (tstate=0x559a2f30ce28 <_PyRuntime+493256>) at Modules/gcmodule.c:2154
#17 0x0000559a2ef74d80 in finalize_modules (tstate=0x559a2f30ce28 <_PyRuntime+493256>) at Python/pylifecycle.c:1677
#18 0x0000559a2ef751cb in Py_FinalizeEx () at Python/pylifecycle.c:1931
#19 0x0000559a2efb4ced in Py_RunMain () at Modules/main.c:709
#20 0x0000559a2efb4d9f in pymain_main (args=0x7ffffccdefd0) at Modules/main.c:737
#21 0x0000559a2efb4e5f in Py_BytesMain (argc=2, argv=0x7ffffccdf138) at Modules/main.c:761
#22 0x0000559a2eccc885 in main (argc=2, argv=0x7ffffccdf138) at ./Programs/python.c:15

CC @kumaraditya303

CPython versions tested on:

3.12, CPython main branch

Operating systems tested on:

Linux

Output from running 'python -VV' on the command line:

Python 3.13.0a1+ (heads/main:ba8aa1fd37, Nov 6 2023, 16:26:31) [GCC 13.2.1 20231014]

Linked PRs

• gh-111784: Add PyCapsule_ImportCapsule and fix segfault in _elementtree.c #112053
• GH-111784: Fix segfault by keeping pyexpat module alive for capsule lifetime #112104
• gh-111784: Fix two segfaults #113405
• [3.12] gh-111784: Fix two segfaults in the elementtree module (GH-113405) #113446

[mdboom]

Cc: @kumaraditya303

[chgnrdv]

Reduced example, just in case:

import _asyncio
import _elementtree

async def _end_stream_wait():
    await "foo"

parser = _elementtree.XMLParser()
_asyncio.Task(_end_stream_wait())

[Eclips4]

The problem is more complicated than it may seem at first glance.
First of all, problem is unrelated to isolation of _elementtree. In fact, here's the first bad commit - b652d40.

What's actually happening and why this code segfaults?

1. XMLParser is trying to deallocate himself:

cpython/Modules/_elementtree.c

Lines 3777 to 3785 in 931f443

	static int
	xmlparser_gc_clear(XMLParserObject *self)
	{
	elementtreestate *st = self->state;
	if (self->parser != NULL) {
	XML_Parser parser = self->parser;
	self->parser = NULL;
	EXPAT(st, ParserFree)(parser);
	}

Line 3784 (which actually means (st->expat_capi->ParserFree)(parser)) leads to segfault if the second point (which described below) already executed.
2. pyexpat_capsule trying to deallocate himself (st->expat_capi of _elementtree is pointing at this data!)

Where's the problem? Order of execution of two points described above is not strict. Second point can be executed before first ponit and vice-versa (which is okay for us). Situation when pyexpat_capsule deallocated before deallocation of XMLParser leads to segmentation fault.

cc @vstinner

[vstinner]

_elementtree extension should make sure that the pyexpat extension remains usable until it is done using it.

struct PyExpat_CAPI *expat_capi; of elementtreestate is just a pointer, it doesn't impact lifetime of the capsule memory or the pyexpat extension.

[Eclips4]

_elementtree extension should make sure that the pyexpat extension remains usable until it is done using it.

struct PyExpat_CAPI *expat_capi; of elementtreestate is just a pointer, it doesn't impact lifetime of the capsule memory or the pyexpat extension.

Yes, that is what we should do. But I don't understand how we can make sure that the pyexpat extension remains usable until _elementtree is done using it.

[vstinner]

Can we keep a strong reference to the pyexpat module? Is it enough?

[Eclips4]

Can we keep a strong reference to the pyexat module? Is it enough?

There's no such API, and yes, it will be enough.
Imported capsule doesn't store any information about module which is bounded to.

[Eclips4]

To be honest, I don't see any solution without changing some code of capsule's implementation..

[vstinner]

To be honest, I don't see any solution without changing some code of capsule's implementation..

That's acceptable and has been done recently: see commit 513c89d.