gh-143200: fix UAFs in Element.__{set,get}item__

Author: picnixz

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

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.

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++) {