gh-126033: fix a crash in xml.etree.ElementTree.Element.remove when concurrent mutations happen

Lib/test/test_xml_etree.py

@@ -2680,18 +2680,51 @@ class Y(X, ET.Element):
         e = ET.Element('foo')
         e.extend(L)
 
-    def test_remove_with_mutating(self):
-        class X(ET.Element):
+    def test_remove_with_clear_children(self):
+        # See: https://github.com/python/cpython/issues/126033
+
+        class EvilElement(ET.Element):
             def __eq__(self, o):
-                del e[:]
+                root.clear()
                 return False
-        e = ET.Element('foo')
-        e.extend([X('bar')])
-        self.assertRaises(ValueError, e.remove, ET.Element('baz'))
 
-        e = ET.Element('foo')
-        e.extend([ET.Element('bar')])
-        self.assertRaises(ValueError, e.remove, X('baz'))
+        # The pure Python implementation raises a ValueError but the C
+        # implementation raises a RuntimeError (like OrderedDict does).
+        exc_type = ValueError if ET is pyET else RuntimeError
+
+        root = ET.Element('.')
+        root.append(EvilElement('foo'))
+        root.append(ET.Element('bar'))
+        self.assertRaises(exc_type, root.remove, ET.Element('pouet'))
+
+        root = ET.Element('.')
+        root.append(ET.Element('foo'))
+        root.append(EvilElement('bar'))
+        self.assertRaises(exc_type, root.remove, EvilElement('pouet'))
+
+    def test_remove_with_mutate_children(self):
+        # See: https://github.com/python/cpython/issues/126033
+
+        class EvilElement(ET.Element):
+            def __eq__(self, o):
+                # Remove the first element so that the list size changes.
+                # This causes an infinite recursion error in the Python
+                # implementation, but we do not really care about it.
+                root.remove(ET.Element('foo'))
+                return False
+
+        # The pure Python implementation raises a ValueError (or hits the
+        # recursion limit) but the C implementation raises a RuntimeError
+        # (like OrderedDict does).
+        exc_type = (RecursionError, ValueError) if ET is pyET else RuntimeError
+
+        root = ET.Element('.')
+        root.extend([ET.Element('foo'), EvilElement('bar')])
+        self.assertRaises(exc_type, root.remove, ET.Element('baz'))
+
+        root = ET.Element('.')
+        root.extend([ET.Element('foo'), EvilElement('bar')])
+        self.assertRaises(exc_type, root.remove, EvilElement('baz'))
 
     @support.infinite_recursion(25)
     def test_recursive_repr(self):

Lib/test/test_xml_etree_c.py

@@ -234,7 +234,7 @@ class SizeofTest(unittest.TestCase):
     def setUp(self):
         self.elementsize = support.calcobjsize('5P')
         # extra
-        self.extra = struct.calcsize('PnnP4P')
+        self.extra = struct.calcsize('PnnP4PN')
 
     check_sizeof = support.check_sizeof
 

Misc/NEWS.d/next/Library/2024-10-29-12-59-45.gh-issue-126033.sM3uCn.rst

@@ -0,0 +1,3 @@
+Fix a crash in :meth:`Element.remove <xml.etree.ElementTree.Element.remove>`
+when either the element to remove or one of the children implements an evil
+:meth:`~object.__eq__` method. Patch by Bénédikt Tran.

Modules/_elementtree.c

@@ -230,6 +230,8 @@ typedef struct {
 
     PyObject* _children[STATIC_CHILDREN];
 
+    /* incremented whenever the object is externally mutated */
+    size_t version;
 } ElementObjectExtra;
 
 typedef struct {
@@ -279,6 +281,8 @@ create_extra(ElementObject* self, PyObject* attrib)
     self->extra->allocated = STATIC_CHILDREN;
     self->extra->children = self->extra->_children;
 
+    self->extra->version = 0;
+
     return 0;
 }
 
@@ -506,6 +510,7 @@ element_resize(ElementObject* self, Py_ssize_t extra)
         }
         self->extra->children = children;
         self->extra->allocated = size;
+        self->extra->version++;
     }
 
     return 0;
@@ -539,6 +544,7 @@ element_add_subelement(elementtreestate *st, ElementObject *self,
     self->extra->children[self->extra->length] = Py_NewRef(element);
 
     self->extra->length++;
+    self->extra->version++;
 
     return 0;
 }
@@ -780,6 +786,7 @@ _elementtree_Element___copy___impl(ElementObject *self, PyTypeObject *cls)
 
         assert(!element->extra->length);
         element->extra->length = self->extra->length;
+        element->extra->version = self->extra->version;
     }
 
     return (PyObject*) element;
@@ -847,6 +854,7 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
         if (element_resize(element, self->extra->length) < 0)
             goto error;
 
+        // TODO(picnixz): check for an evil child's __deepcopy__ on 'self'
         for (i = 0; i < self->extra->length; i++) {
             PyObject* child = deepcopy(st, self->extra->children[i], memo);
             if (!child || !Element_Check(st, child)) {
@@ -862,6 +870,7 @@ _elementtree_Element___deepcopy___impl(ElementObject *self, PyObject *memo)
 
         assert(!element->extra->length);
         element->extra->length = self->extra->length;
+        element->extra->version = 0;
     }
 
     /* add object to memo dictionary (so deepcopy won't visit it again) */
@@ -1548,6 +1557,7 @@ _elementtree_Element_insert_impl(ElementObject *self, Py_ssize_t index,
     self->extra->children[index] = Py_NewRef(subelement);
 
     self->extra->length++;
+    self->extra->version++;
 
     Py_RETURN_NONE;
 }
@@ -1632,46 +1642,58 @@ static PyObject *
 _elementtree_Element_remove_impl(ElementObject *self, PyObject *subelement)
 /*[clinic end generated code: output=38fe6c07d6d87d1f input=6133e1d05597d5ee]*/
 {
-    Py_ssize_t i;
-    int rc;
-    PyObject *found;
-
-    if (!self->extra) {
+    if (self->extra == NULL) {
         /* element has no children, so raise exception */
-        PyErr_SetString(
-            PyExc_ValueError,
-            "list.remove(x): x not in list"
-            );
-        return NULL;
+        goto not_found;
     }
 
-    for (i = 0; i < self->extra->length; i++) {
-        if (self->extra->children[i] == subelement)
+    Py_ssize_t i;
+    size_t old_version = self->extra->version;
+    for (i = 0; self->extra && i < self->extra->length; i++) {
+        if (old_version != self->extra->version) {
+            goto fail;
+        }
+        else if (self->extra->children[i] == subelement) {
             break;
-        rc = PyObject_RichCompareBool(self->extra->children[i], subelement, Py_EQ);
-        if (rc > 0)
+        }
+        PyObject *child = Py_NewRef(self->extra->children[i]);
+        int rc = PyObject_RichCompareBool(child, subelement, Py_EQ);
+        Py_DECREF(child);
+        if (rc > 0) {
             break;
-        if (rc < 0)
+        }
+        else if (rc < 0) {
             return NULL;
+        }
     }
 
-    if (i >= self->extra->length) {
+    // An extra check must be done if the mutation occurs at the very last step.
+    if (self->extra == NULL || old_version != self->extra->version) {
+        goto fail;
+    }
+    else if (i >= self->extra->length) {
         /* subelement is not in children, so raise exception */
-        PyErr_SetString(
-            PyExc_ValueError,
-            "list.remove(x): x not in list"
-            );
-        return NULL;
+        goto not_found;
     }
 
-    found = self->extra->children[i];
+    PyObject *found = self->extra->children[i];
 
     self->extra->length--;
-    for (; i < self->extra->length; i++)
+    for (; i < self->extra->length; i++) {
         self->extra->children[i] = self->extra->children[i+1];
+    }
+    self->extra->version++;
 
     Py_DECREF(found);
     Py_RETURN_NONE;
+
+not_found:
+    PyErr_SetString(PyExc_ValueError, "list.remove(x): x not in list");
+    return NULL;
+
+fail:
+    PyErr_SetString(PyExc_RuntimeError, "children mutated during iteration");
+    return NULL;
 }
 
 static PyObject*
@@ -1724,6 +1746,7 @@ _elementtree_Element_set_impl(ElementObject *self, PyObject *key,
     if (PyDict_SetItem(attrib, key, value) < 0)
         return NULL;
 
+    self->extra->version++;
     Py_RETURN_NONE;
 }
 
@@ -1757,6 +1780,7 @@ element_setitem(PyObject* self_, Py_ssize_t index, PyObject* item)
             self->extra->children[i] = self->extra->children[i+1];
     }
 
+    self->extra->version++;
     Py_DECREF(old);
 
     return 0;
@@ -1907,6 +1931,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value)
             }
 
             self->extra->length -= slicelen;
+            self->extra->version++;
 
             /* Discard the recycle list with all the deleted sub-elements */
             Py_DECREF(recycle);
@@ -1982,6 +2007,7 @@ element_ass_subscr(PyObject* self_, PyObject* item, PyObject* value)
         }
 
         self->extra->length += newlen - slicelen;
+        self->extra->version++;
 
         Py_DECREF(seq);
 
@@ -2078,6 +2104,7 @@ element_attrib_setter(ElementObject *self, PyObject *value, void *closure)
             return -1;
     }
     Py_XSETREF(self->extra->attrib, Py_NewRef(value));
+    self->extra->version++;
     return 0;
 }