From b4f4d57f52b1208b73260f8b416919cd47ddf19a Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Wed, 8 Apr 2026 18:39:11 +0200 Subject: [PATCH 1/4] bug: prevent race condition in list.remove --- Objects/listobject.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Objects/listobject.c b/Objects/listobject.c index 4a98c8e54ab03f..12347963639aad 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3390,6 +3390,11 @@ list_remove_impl(PyListObject *self, PyObject *value) int cmp = PyObject_RichCompareBool(obj, value, Py_EQ); Py_DECREF(obj); if (cmp > 0) { + if (i >= Py_SIZE(self) || self->ob_item[i] != obj) { + PyErr_SetString(PyExc_RuntimeError, + "list mutated during remove"); + return NULL; + } if (list_ass_slice_lock_held(self, i, i+1, NULL) == 0) Py_RETURN_NONE; return NULL; From 6b97b0747f750dd93d6af3afdd2eddb48204ab7f Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Mon, 13 Apr 2026 16:50:37 -0400 Subject: [PATCH 2/4] test: fix test_xml_etree --- Lib/test/test_xml_etree.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index 93162f52ba0344..897395f4f57dbf 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -2816,15 +2816,15 @@ def test_remove_with_clear_assume_missing(self): def test_remove_with_clear_assume_existing(self): # gh-126033: Check that a concurrent clear() for an assumed-to-be - # existing element does not make the interpreter crash. + # existing element raises RuntimeError but does not crash. self.do_test_remove_with_clear(raises=False) def do_test_remove_with_clear(self, *, raises): - - # Until the discrepency between "del root[:]" and "root.clear()" is - # resolved, we need to keep two tests. Previously, using "del root[:]" - # did not crash with the reproducer of gh-126033 while "root.clear()" - # did. + # 'del root[:]' mutates the children list in-place, while + # 'root.clear()' replaces self._children with a new list. When + # raises=False (element "found"), the in-place mutation is detected + # by list.remove and raises RuntimeError, whereas root.clear() is + # invisible to list.remove (the old list is unchanged). class E(ET.Element): """Local class to be able to mock E.__eq__ for introspection.""" @@ -2839,16 +2839,23 @@ def __eq__(self, o): root.clear() return not raises - if raises: - get_checker_context = lambda: self.assertRaises(ValueError) - else: - get_checker_context = nullcontext - self.assertIs(E.__eq__, object.__eq__) + get_checker_context = nullcontext for Z, side_effect in [(X, 'del root[:]'), (Y, 'root.clear()')]: self.enterContext(self.subTest(side_effect=side_effect)) + # X uses 'del root[:]' which mutates the list in-place; this is + # detected by list.remove when raises=False (element "found"). + # Y uses 'root.clear()' which swaps out self._children; the old + # list that list.remove is iterating is untouched, so no error. + if raises: + get_checker_context = lambda: self.assertRaises(ValueError) + elif Z is X: + get_checker_context = lambda: self.assertRaises(RuntimeError) + else: + get_checker_context = nullcontext + # test removing R() from [U()] for R, U, description in [ (E, Z, "remove missing E() from [Z()]"), @@ -2883,7 +2890,6 @@ def __eq__(self, o): g.assert_not_called() # Test removing root[1] (of type R) from [U(), R()]. - is_special = is_python_implementation() and raises and Z is Y if is_python_implementation() and raises and Z is Y: # In pure Python, using root.clear() sets the children # list to [] without calling list.clear(). @@ -2913,7 +2919,7 @@ def test_remove_with_mutate_root_assume_missing(self): def test_remove_with_mutate_root_assume_existing(self): # gh-126033: Check that a concurrent mutation for an assumed-to-be - # existing element does not make the interpreter crash. + # existing element raises RuntimeError. self.do_test_remove_with_mutate_root(raises=False) def do_test_remove_with_mutate_root(self, *, raises): @@ -2927,7 +2933,7 @@ def __eq__(self, o): if raises: get_checker_context = lambda: self.assertRaises(ValueError) else: - get_checker_context = nullcontext + get_checker_context = lambda: self.assertRaises(RuntimeError) # test removing R() from [U(), V()] cases = self.cases_for_remove_missing_with_mutations(E, Z) From c30ea141f6cc80b463b80bf16168dc9324a8a061 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Mon, 13 Apr 2026 16:53:17 -0400 Subject: [PATCH 3/4] news: add entry --- .../2026-04-13-20-51-34.gh-issue-148259.76UyK8x8.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-20-51-34.gh-issue-148259.76UyK8x8.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-20-51-34.gh-issue-148259.76UyK8x8.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-20-51-34.gh-issue-148259.76UyK8x8.rst new file mode 100644 index 00000000000000..3da424b2523c72 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-20-51-34.gh-issue-148259.76UyK8x8.rst @@ -0,0 +1,4 @@ +:meth:`list.remove` now raises :exc:`RuntimeError` if the list is mutated +during the ``__eq__`` rich comparison and a matching element was found. This +prevents silent data corruption where the wrong element could be removed from +a list under concurrent mutation. From 648c3f65f946fa01897f31372ac38fb1c904c26d Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Tue, 14 Apr 2026 08:19:29 -0400 Subject: [PATCH 4/4] test: update test_xml_etree --- Lib/test/test_xml_etree.py | 43 +++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index 897395f4f57dbf..24c2ceb9b19415 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -2812,17 +2812,17 @@ class Y(X, ET.Element): def test_remove_with_clear_assume_missing(self): # gh-126033: Check that a concurrent clear() for an assumed-to-be # missing element does not make the interpreter crash. - self.do_test_remove_with_clear(raises=True) + self.do_test_remove_with_clear(existing=False) def test_remove_with_clear_assume_existing(self): # gh-126033: Check that a concurrent clear() for an assumed-to-be # existing element raises RuntimeError but does not crash. - self.do_test_remove_with_clear(raises=False) + self.do_test_remove_with_clear(existing=True) - def do_test_remove_with_clear(self, *, raises): + def do_test_remove_with_clear(self, *, existing): # 'del root[:]' mutates the children list in-place, while # 'root.clear()' replaces self._children with a new list. When - # raises=False (element "found"), the in-place mutation is detected + # existing=True (element "found"), the in-place mutation is detected # by list.remove and raises RuntimeError, whereas root.clear() is # invisible to list.remove (the old list is unchanged). @@ -2832,12 +2832,12 @@ class E(ET.Element): class X(E): def __eq__(self, o): del root[:] - return not raises + return existing class Y(E): def __eq__(self, o): root.clear() - return not raises + return existing self.assertIs(E.__eq__, object.__eq__) @@ -2846,12 +2846,14 @@ def __eq__(self, o): self.enterContext(self.subTest(side_effect=side_effect)) # X uses 'del root[:]' which mutates the list in-place; this is - # detected by list.remove when raises=False (element "found"). + # detected by list.remove (pure Python implementation) when existing=False. + # The C implementation does not detect this mutation and silently + # returns None. # Y uses 'root.clear()' which swaps out self._children; the old # list that list.remove is iterating is untouched, so no error. - if raises: + if not existing: get_checker_context = lambda: self.assertRaises(ValueError) - elif Z is X: + elif Z is X and is_python_implementation(): get_checker_context = lambda: self.assertRaises(RuntimeError) else: get_checker_context = nullcontext @@ -2890,7 +2892,7 @@ def __eq__(self, o): g.assert_not_called() # Test removing root[1] (of type R) from [U(), R()]. - if is_python_implementation() and raises and Z is Y: + if is_python_implementation() and not existing and Z is Y: # In pure Python, using root.clear() sets the children # list to [] without calling list.clear(). # @@ -2915,25 +2917,32 @@ def __eq__(self, o): def test_remove_with_mutate_root_assume_missing(self): # gh-126033: Check that a concurrent mutation for an assumed-to-be # missing element does not make the interpreter crash. - self.do_test_remove_with_mutate_root(raises=True) + self.do_test_remove_with_mutate_root(existing=False) def test_remove_with_mutate_root_assume_existing(self): # gh-126033: Check that a concurrent mutation for an assumed-to-be - # existing element raises RuntimeError. - self.do_test_remove_with_mutate_root(raises=False) + # existing element raises RuntimeError when using the pure Python + # implementation; the C implementation silently returns None). + self.do_test_remove_with_mutate_root(existing=True) - def do_test_remove_with_mutate_root(self, *, raises): + def do_test_remove_with_mutate_root(self, *, existing): E = ET.Element class Z(E): def __eq__(self, o): del root[0] - return not raises + return existing - if raises: + if not existing: get_checker_context = lambda: self.assertRaises(ValueError) - else: + elif is_python_implementation(): + # The Python implementation is based on list.remove, which raises + # RuntimeError if the list is mutated during the __eq__ comparison. get_checker_context = lambda: self.assertRaises(RuntimeError) + else: + # The C implementation does not detect in-place mutations during + # remove and silently returns None. + get_checker_context = nullcontext # test removing R() from [U(), V()] cases = self.cases_for_remove_missing_with_mutations(E, Z)