Skip to content

Commit 68eabae

Browse files
committed
gh-161062: Fix crash on concurrent first read of Element.text or tail
On the free-threaded build, join the tree builder's pending text chunks in the element's critical section so that two readers cannot both join and release the same list.
1 parent e5f1477 commit 68eabae

3 files changed

Lines changed: 90 additions & 27 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import unittest
2+
3+
from test.support import import_helper, threading_helper
4+
from test.support.threading_helper import run_concurrently
5+
6+
# The C accelerator keeps text delivered by expat in several chunks as a
7+
# list and joins it on first access.
8+
ET = import_helper.import_fresh_module('xml.etree.ElementTree',
9+
fresh=['_elementtree'])
10+
11+
NTHREADS = 8
12+
NITEMS = 500
13+
14+
# Each item text arrives as three chunks ('name ', '&', ' N') and each
15+
# tail as two ('\n', ' ').
16+
DOC = ("<root>\n" +
17+
"".join(f" <item>name &amp; {i}</item>\n" for i in range(NITEMS)) +
18+
" </root>\n")
19+
20+
21+
@threading_helper.requires_working_threading()
22+
@unittest.skipIf(ET is None, 'requires _elementtree')
23+
class TestElementConcurrentReads(unittest.TestCase):
24+
def check(self, worker, nrounds=20):
25+
for _ in range(nrounds):
26+
root = ET.fromstring(DOC)
27+
run_concurrently(worker_func=worker, nthreads=NTHREADS,
28+
args=(root,))
29+
30+
def test_concurrent_text_tail(self):
31+
def worker(root):
32+
for i, item in enumerate(root):
33+
self.assertEqual(item.text, f"name & {i}")
34+
self.assertEqual(item.tail, "\n ")
35+
36+
self.check(worker)
37+
38+
def test_concurrent_tostring(self):
39+
expected = ET.tostring(ET.fromstring(DOC))
40+
41+
def worker(root):
42+
self.assertEqual(ET.tostring(root), expected)
43+
44+
self.check(worker)
45+
46+
47+
if __name__ == "__main__":
48+
unittest.main()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix a crash in the :term:`free-threaded build` when multiple threads
2+
concurrently read the :attr:`~xml.etree.ElementTree.Element.text` or
3+
:attr:`~xml.etree.ElementTree.Element.tail` attribute of a freshly parsed
4+
:class:`xml.etree.ElementTree.Element` for the first time.

‎Modules/_elementtree.c‎

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -571,45 +571,56 @@ element_get_attrib(ElementObject* self)
571571
}
572572

573573
LOCAL(PyObject*)
574-
element_get_text(ElementObject* self)
574+
element_get_joined(ElementObject* self, PyObject **slot)
575575
{
576-
/* return new reference to text attribute */
576+
/* return new reference to the text or tail attribute stored in *slot,
577+
joining a list of chunks left there by the tree builder on first
578+
access. */
577579

578-
PyObject *res = self->text;
580+
PyObject *res;
579581

580-
if (JOIN_GET(res)) {
581-
res = JOIN_OBJ(res);
582-
if (PyList_CheckExact(res)) {
583-
PyObject *tmp = list_join(res);
584-
if (!tmp)
585-
return NULL;
586-
self->text = tmp;
587-
Py_SETREF(res, tmp);
582+
Py_BEGIN_CRITICAL_SECTION(self);
583+
res = *slot;
584+
if (JOIN_GET(res) && PyList_CheckExact(JOIN_OBJ(res))) {
585+
/* The critical section may be suspended while PyUnicode_Join()
586+
locks the list, letting another thread join and release it
587+
first: hold a reference and re-check *slot before storing. */
588+
PyObject *list = Py_NewRef(JOIN_OBJ(res));
589+
PyObject *joined = list_join(list);
590+
if (joined == NULL) {
591+
res = NULL;
592+
}
593+
else if (*slot == res) {
594+
_set_joined_ptr(slot, joined);
595+
res = joined;
588596
}
597+
else {
598+
Py_DECREF(joined);
599+
res = JOIN_OBJ(*slot);
600+
}
601+
Py_DECREF(list);
602+
}
603+
else {
604+
res = JOIN_OBJ(res);
589605
}
606+
Py_XINCREF(res);
607+
Py_END_CRITICAL_SECTION();
590608

591-
return Py_NewRef(res);
609+
return res;
610+
}
611+
612+
LOCAL(PyObject*)
613+
element_get_text(ElementObject* self)
614+
{
615+
/* return new reference to text attribute */
616+
return element_get_joined(self, &self->text);
592617
}
593618

594619
LOCAL(PyObject*)
595620
element_get_tail(ElementObject* self)
596621
{
597622
/* return new reference to tail attribute */
598-
599-
PyObject *res = self->tail;
600-
601-
if (JOIN_GET(res)) {
602-
res = JOIN_OBJ(res);
603-
if (PyList_CheckExact(res)) {
604-
PyObject *tmp = list_join(res);
605-
if (!tmp)
606-
return NULL;
607-
self->tail = tmp;
608-
Py_SETREF(res, tmp);
609-
}
610-
}
611-
612-
return Py_NewRef(res);
623+
return element_get_joined(self, &self->tail);
613624
}
614625

615626
/*[clinic input]

0 commit comments

Comments
 (0)