From 1a0c9709ce87c4c594abbc9799db52a84c44b541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 27 Dec 2025 15:43:07 +0100 Subject: [PATCH 1/4] gh-143200: fix UAFs in `Element.__{set,get}item__` --- Lib/test/test_xml_etree.py | 72 ++++++++++++++----- ...-12-27-15-41-27.gh-issue-143200.2hEUAl.rst | 4 ++ Modules/_elementtree.c | 50 ++++++++++--- 3 files changed, 101 insertions(+), 25 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-12-27-15-41-27.gh-issue-143200.2hEUAl.rst diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index 0178ed02b35be1..7aa949b2819172 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -3010,32 +3010,72 @@ def __del__(self): elem = b.close() self.assertEqual(elem[0].tail, 'ABCDEFGHIJKL') - def test_subscr(self): - # Issue #27863 + def test_subscr_with_clear(self): + # See https://github.com/python/cpython/issues/143200. + self.do_test_subscr_with_mutating_slice(use_clear_method=True) + + def test_subscr_with_delete(self): + # See https://github.com/python/cpython/issues/72050. + self.do_test_subscr_with_mutating_slice(use_clear_method=False) + + def do_test_subscr_with_mutating_slice(self, *, use_clear_method): class X: + def __init__(self, i=0): + self.i = i def __index__(self): - del e[:] - return 1 + if use_clear_method: + e.clear() + else: + del e[:] + return self.i - e = ET.Element('elem') - e.append(ET.Element('child')) - e[:X()] # shouldn't crash + for s in self.get_mutating_slices(X, 10): + with self.subTest(s): + e = ET.Element('elem') + e.extend([ET.Element(f'c{i}') for i in range(10)]) + e[s] # shouldn't crash - e.append(ET.Element('child')) - e[0:10:X()] # shouldn't crash + def test_ass_subscr_with_mutating_slice(self): + # See https://github.com/python/cpython/issues/72050 + # and https://github.com/python/cpython/issues/143200. - def test_ass_subscr(self): - # Issue #27863 class X: + def __init__(self, i=0): + self.i = i def __index__(self): e[:] = [] - return 1 + return self.i + + for s in self.get_mutating_slices(X, 10): + with self.subTest(s): + e = ET.Element('elem') + e.extend([ET.Element(f'c{i}') for i in range(10)]) + e[s] = [] # shouldn't crash + + def get_mutating_slices(self, index_class, n_children): + self.assertGreaterEqual(n_children, 10) + return [ + slice(index_class(), None, None), + slice(index_class(2), None, None), + slice(None, index_class(), None), + slice(None, index_class(2), None), + slice(0, 2, index_class(1)), + slice(0, 2, index_class(2)), + slice(0, n_children, index_class(1)), + slice(0, n_children, index_class(2)), + slice(0, 2 * n_children, index_class(1)), + slice(0, 2 * n_children, index_class(2)), + ] - e = ET.Element('elem') - for _ in range(10): - e.insert(0, ET.Element('child')) + def test_ass_subscr_with_mutating_iterable_value(self): + class V: + def __iter__(self): + e.clear() + return iter([ET.Element('a'), ET.Element('b')]) - e[0:10:X()] = [] # shouldn't crash + e = ET.Element('elem') + e.extend([ET.Element(f'c{i}') for i in range(10)]) + e[:] = V() def test_treebuilder_start(self): # Issue #27863 diff --git a/Misc/NEWS.d/next/Library/2025-12-27-15-41-27.gh-issue-143200.2hEUAl.rst b/Misc/NEWS.d/next/Library/2025-12-27-15-41-27.gh-issue-143200.2hEUAl.rst new file mode 100644 index 00000000000000..8b24decf098745 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-12-27-15-41-27.gh-issue-143200.2hEUAl.rst @@ -0,0 +1,4 @@ +:mod:`xml.etree.ElementTree`: fix use-after-free crashes in +:meth:`~object.__getitem__` and :meth:`~object.__setitem__` methods of +:class:`~xml.etree.ElementTree.Element` when the element is concurrently +mutated. Patch by Bénédikt Tran. diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 3173b52afb31b6..4e92008ed36120 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1801,6 +1801,8 @@ element_subscr(PyObject *op, PyObject *item) if (i == -1 && PyErr_Occurred()) { return NULL; } + // If PyNumber_AsSsize_t() clears 'self->extra', + // we want element_getitem() to raise IndexError. if (i < 0 && self->extra) i += self->extra->length; return element_getitem(op, i); @@ -1810,12 +1812,19 @@ element_subscr(PyObject *op, PyObject *item) size_t cur; PyObject* list; - if (!self->extra) - return PyList_New(0); - if (PySlice_Unpack(item, &start, &stop, &step) < 0) { return NULL; } + + // Even if PySlice_Unpack() may clear 'self->extra', + // using a slice should not raise an IndexError, so + // we do not need to ensure 'extra' to exist. + // + // See https://github.com/python/cpython/issues/143200. + if (self->extra == NULL) { + return PyList_New(0); + } + slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, step); @@ -1853,6 +1862,8 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) if (i == -1 && PyErr_Occurred()) { return -1; } + // If PyNumber_AsSsize_t() clears 'self->extra', + // we want element_setitem() to raise IndexError. if (i < 0 && self->extra) i += self->extra->length; return element_setitem(op, i, value); @@ -1864,16 +1875,20 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) PyObject* recycle = NULL; PyObject* seq; - if (!self->extra) { - if (create_extra(self, NULL) < 0) - return -1; - } - if (PySlice_Unpack(item, &start, &stop, &step) < 0) { return -1; } + // Since PySlice_Unpack() may clear 'self->extra', we need + // to ensure that it exists now (using a slice for assignment + // should not raise IndexError). + // + // See https://github.com/python/cpython/issues/143200. + if (self->extra == NULL && create_extra(self, NULL) < 0) { + return -1; + } + slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, - step); + step); if (value == NULL) { /* Delete slice */ @@ -1908,6 +1923,8 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) * Note that in the ith iteration, shifting is done i+i places down * because i children were already removed. */ + assert(self->extra != NULL); + assert(self->extra->children != NULL); for (cur = start, i = 0; cur < (size_t)stop; cur += step, ++i) { /* Compute how many children have to be moved, clipping at the * list end. @@ -1948,6 +1965,18 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) } newlen = PySequence_Fast_GET_SIZE(seq); + // Re-create 'self->extra' if PySequence_Fast() cleared it. + // See https://github.com/python/cpython/issues/143200. + if (self->extra == NULL) { + if (create_extra(self, NULL) < 0) { + Py_DECREF(seq); + return -1; + } + // re-compute the slice length since self->extra->length changed + slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, + step); + } + if (step != 1 && newlen != slicelen) { Py_DECREF(seq); @@ -1967,6 +1996,9 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) } } + assert(recycle == NULL); + assert(self->extra != NULL); + PyTypeObject *tp = Py_TYPE(self); elementtreestate *st = get_elementtree_state_by_type(tp); for (i = 0; i < newlen; i++) { From fbf0e2932d928e9cb0022aba3f04c88b95ddff22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 27 Dec 2025 17:35:47 +0100 Subject: [PATCH 2/4] address review --- Modules/_elementtree.c | 41 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 4e92008ed36120..9e42b65d4376fa 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1801,13 +1801,14 @@ element_subscr(PyObject *op, PyObject *item) if (i == -1 && PyErr_Occurred()) { return NULL; } - // If PyNumber_AsSsize_t() clears 'self->extra', - // we want element_getitem() to raise IndexError. if (i < 0 && self->extra) i += self->extra->length; return element_getitem(op, i); } else if (PySlice_Check(item)) { + // Note: 'slicelen' is computed once we are sure that 'self->extra' + // cannot be mutated by user-defined code. + // See https://github.com/python/cpython/issues/143200. Py_ssize_t start, stop, step, slicelen, i; size_t cur; PyObject* list; @@ -1816,15 +1817,9 @@ element_subscr(PyObject *op, PyObject *item) return NULL; } - // Even if PySlice_Unpack() may clear 'self->extra', - // using a slice should not raise an IndexError, so - // we do not need to ensure 'extra' to exist. - // - // See https://github.com/python/cpython/issues/143200. if (self->extra == NULL) { return PyList_New(0); } - slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, step); @@ -1862,13 +1857,14 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) if (i == -1 && PyErr_Occurred()) { return -1; } - // If PyNumber_AsSsize_t() clears 'self->extra', - // we want element_setitem() to raise IndexError. if (i < 0 && self->extra) i += self->extra->length; return element_setitem(op, i, value); } else if (PySlice_Check(item)) { + // Note: 'slicelen' is computed once we are sure that 'self->extra' + // cannot be mutated by user-defined code. + // See https://github.com/python/cpython/issues/143200. Py_ssize_t start, stop, step, slicelen, newlen, i; size_t cur; @@ -1878,23 +1874,22 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) if (PySlice_Unpack(item, &start, &stop, &step) < 0) { return -1; } + // Since PySlice_Unpack() may clear 'self->extra', we need // to ensure that it exists now (using a slice for assignment // should not raise IndexError). // // See https://github.com/python/cpython/issues/143200. - if (self->extra == NULL && create_extra(self, NULL) < 0) { - return -1; + if (self->extra == NULL) { + if (create_extra(self, NULL) < 0) { + return -1; + } } - slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, - step); - if (value == NULL) { /* Delete slice */ - size_t cur; - Py_ssize_t i; - + slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, + step); if (slicelen <= 0) return 0; @@ -1965,20 +1960,16 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) } newlen = PySequence_Fast_GET_SIZE(seq); - // Re-create 'self->extra' if PySequence_Fast() cleared it. - // See https://github.com/python/cpython/issues/143200. if (self->extra == NULL) { if (create_extra(self, NULL) < 0) { Py_DECREF(seq); return -1; } - // re-compute the slice length since self->extra->length changed - slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, - step); } + slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, + step); - if (step != 1 && newlen != slicelen) - { + if (step != 1 && newlen != slicelen) { Py_DECREF(seq); PyErr_Format(PyExc_ValueError, "attempt to assign sequence of size %zd " From 4509a3312dc211fbf7bcec5ba491dd106362cae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 27 Dec 2025 17:38:48 +0100 Subject: [PATCH 3/4] remove debugging assertions --- Modules/_elementtree.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index 9e42b65d4376fa..d93b23d18143a4 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1918,8 +1918,6 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) * Note that in the ith iteration, shifting is done i+i places down * because i children were already removed. */ - assert(self->extra != NULL); - assert(self->extra->children != NULL); for (cur = start, i = 0; cur < (size_t)stop; cur += step, ++i) { /* Compute how many children have to be moved, clipping at the * list end. @@ -1987,9 +1985,6 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) } } - assert(recycle == NULL); - assert(self->extra != NULL); - PyTypeObject *tp = Py_TYPE(self); elementtreestate *st = get_elementtree_state_by_type(tp); for (i = 0; i < newlen; i++) { From b7c7f71ec4b91bc996f189bcc8371c35e98a7be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sun, 28 Dec 2025 14:22:21 +0100 Subject: [PATCH 4/4] avoid creating useless extras --- Modules/_elementtree.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/Modules/_elementtree.c b/Modules/_elementtree.c index d93b23d18143a4..22d3205e6ad314 100644 --- a/Modules/_elementtree.c +++ b/Modules/_elementtree.c @@ -1875,19 +1875,11 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) return -1; } - // Since PySlice_Unpack() may clear 'self->extra', we need - // to ensure that it exists now (using a slice for assignment - // should not raise IndexError). - // - // See https://github.com/python/cpython/issues/143200. - if (self->extra == NULL) { - if (create_extra(self, NULL) < 0) { - return -1; - } - } - if (value == NULL) { /* Delete slice */ + if (self->extra == NULL) { + return 0; + } slicelen = PySlice_AdjustIndices(self->extra->length, &start, &stop, step); if (slicelen <= 0)