improve detection and tests

Author: picnixz

Lib/test/test_xml_etree.py

@@ -2686,17 +2686,13 @@ def __eq__(self, o):
                 del e[:]
                 return False
 
-        # 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
-
         e = ET.Element('foo')
         e.extend([X('bar')])
-        self.assertRaises(exc_type, e.remove, ET.Element('baz'))
+        self.assertRaises(ValueError, e.remove, ET.Element('baz'))
 
         e = ET.Element('foo')
         e.extend([ET.Element('bar')])
-        self.assertRaises(exc_type, e.remove, X('baz'))
+        self.assertRaises(ValueError, e.remove, X('baz'))
 
     def test_remove_with_clear_children(self):
         # See: https://github.com/python/cpython/issues/126033
@@ -2706,15 +2702,11 @@ def __eq__(self, o):
                 root.clear()
                 return False
 
-        # 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
-
         for foo_type, rem_type in [(X, ET.Element), (ET.Element, X)]:
             with self.subTest(foo_type=foo_type, rem_type=rem_type):
                 root = ET.Element('.')
                 root.extend([foo_type('foo'), rem_type('bar')])
-                self.assertRaises(exc_type, root.remove, rem_type('baz'))
+                self.assertRaises(ValueError, root.remove, rem_type('baz'))
 
     def test_remove_with_mutate_root(self):
         # See: https://github.com/python/cpython/issues/126033
@@ -2731,15 +2723,11 @@ def __eq__(self, o):
                 root.remove(first_element)
                 return False
 
-        # 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
-
         for bar_type, rem_type in [(X, ET.Element), (ET.Element, X), (X, X)]:
             with self.subTest(bar_type=bar_type, rem_type=rem_type):
                 root = ET.Element('.')
                 root.extend([first_element, bar_type('bar')])
-                self.assertRaises(exc_type, root.remove, rem_type('baz'))
+                self.assertRaises(ValueError, root.remove, rem_type('baz'))
 
     @support.infinite_recursion(25)
     def test_recursive_repr(self):

Modules/_elementtree.c

@@ -1634,38 +1634,36 @@ _elementtree_Element_remove_impl(ElementObject *self, PyObject *subelement)
 {
     if (self->extra == NULL) {
         /* element has no children, so raise exception */
-        goto not_found;
+        goto error;
     }
 
     Py_ssize_t i;
-    const Py_ssize_t length = self->extra->length;
+    // When iterating over the list of children, we need to check that the
+    // list is not cleared (self->extra != NULL) and that we are still within
+    // the correct bounds (i < self->extra->length).
+    //
+    // We deliberately avoid protecting against children lists that grow
+    // faster than the index since list objects do not protect against it.
     for (i = 0; self->extra && i < self->extra->length; i++) {
         if (self->extra->children[i] == subelement) {
             break;
         }
         PyObject *child = Py_NewRef(self->extra->children[i]);
         int rc = PyObject_RichCompareBool(child, subelement, Py_EQ);
         Py_DECREF(child);
-        if (self->extra ==  NULL || self->extra->length != length) {
-            goto fail;
+        if (rc < 0) {
+            return NULL;
         }
-        if (rc > 0) {
+        else if (rc > 0) {
             break;
         }
-        else if (rc < 0) {
-            return NULL;
-        }
     }
 
     // An extra check must be done if the mutation occurs at the very last
     // step and removes or clears the 'extra' list (the condition on the
     // length would not be satisfied any more).
-    if (self->extra == NULL || self->extra->length != length) {
-        goto fail;
-    }
-    else if (i >= self->extra->length) {
-        /* subelement is not in children, so raise exception */
-        goto not_found;
+    if (self->extra == NULL || i >= self->extra->length) {
+        goto error;
     }
 
     PyObject *found = self->extra->children[i];
@@ -1678,13 +1676,9 @@ _elementtree_Element_remove_impl(ElementObject *self, PyObject *subelement)
     Py_DECREF(found);
     Py_RETURN_NONE;
 
-not_found:
+error:
     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*