From be6338e7c74061f9e0732f77ea5447c33702a994 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Sun, 12 Apr 2026 14:29:59 +0200 Subject: [PATCH 1/6] fix: eliminate race condition in list rich comparison --- Objects/listobject.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 38dc38dd277b97..9120bbc5a4d47e 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3454,10 +3454,17 @@ list_richcompare_impl(PyObject *v, PyObject *w, int op) Py_RETURN_TRUE; } - /* Search for the first index where items are different */ + /* Search for the first index where items are different. + * Keep vitem/witem alive across the break so that the final ordering + * comparison uses the same differing objects. + * This is required as PyObject_RichCompareBool may release the GIL and + * the list may be mutated in the meantime. + */ + PyObject *vitem = NULL; + PyObject *witem = NULL; for (i = 0; i < Py_SIZE(vl) && i < Py_SIZE(wl); i++) { - PyObject *vitem = vl->ob_item[i]; - PyObject *witem = wl->ob_item[i]; + vitem = vl->ob_item[i]; + witem = wl->ob_item[i]; if (vitem == witem) { continue; } @@ -3465,33 +3472,40 @@ list_richcompare_impl(PyObject *v, PyObject *w, int op) Py_INCREF(vitem); Py_INCREF(witem); int k = PyObject_RichCompareBool(vitem, witem, Py_EQ); - Py_DECREF(vitem); - Py_DECREF(witem); - if (k < 0) + if (k < 0) { + Py_DECREF(vitem); + Py_DECREF(witem); return NULL; - if (!k) + } + if (!k) { + /* keep vitem/witem alive for the final comparison */ break; + } + + Py_DECREF(vitem); + Py_DECREF(witem); + vitem = witem = NULL; } - if (i >= Py_SIZE(vl) || i >= Py_SIZE(wl)) { - /* No more items to compare -- compare sizes */ + if (vitem == NULL) { + /* All compared elements were equal -- compare sizes */ Py_RETURN_RICHCOMPARE(Py_SIZE(vl), Py_SIZE(wl), op); } /* We have an item that differs -- shortcuts for EQ/NE */ if (op == Py_EQ) { + Py_DECREF(vitem); + Py_DECREF(witem); Py_RETURN_FALSE; } if (op == Py_NE) { + Py_DECREF(vitem); + Py_DECREF(witem); Py_RETURN_TRUE; } - /* Compare the final item again using the proper operator */ - PyObject *vitem = vl->ob_item[i]; - PyObject *witem = wl->ob_item[i]; - Py_INCREF(vitem); - Py_INCREF(witem); - PyObject *result = PyObject_RichCompare(vl->ob_item[i], wl->ob_item[i], op); + /* Compare the differing items using the proper operator */ + PyObject *result = PyObject_RichCompare(vitem, witem, op); Py_DECREF(vitem); Py_DECREF(witem); return result; From 8d42bf6721d63a3dc0ec3461b03b63dbf516b47f Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Mon, 13 Apr 2026 17:30:28 -0400 Subject: [PATCH 2/6] refactor: hoist the comparison logic --- Objects/listobject.c | 55 ++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 9120bbc5a4d47e..cc4cd9da53c91b 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3455,16 +3455,12 @@ list_richcompare_impl(PyObject *v, PyObject *w, int op) } /* Search for the first index where items are different. - * Keep vitem/witem alive across the break so that the final ordering - * comparison uses the same differing objects. - * This is required as PyObject_RichCompareBool may release the GIL and - * the list may be mutated in the meantime. + * We incref vitem/witem before calling PyObject_RichCompareBool, which may + * release the GIL and allow the list to be mutated in the meantime. */ - PyObject *vitem = NULL; - PyObject *witem = NULL; for (i = 0; i < Py_SIZE(vl) && i < Py_SIZE(wl); i++) { - vitem = vl->ob_item[i]; - witem = wl->ob_item[i]; + PyObject *vitem = vl->ob_item[i]; + PyObject *witem = wl->ob_item[i]; if (vitem == witem) { continue; } @@ -3478,37 +3474,30 @@ list_richcompare_impl(PyObject *v, PyObject *w, int op) return NULL; } if (!k) { - /* keep vitem/witem alive for the final comparison */ - break; + /* We have a differing item -- shortcuts for EQ/NE */ + if (op == Py_EQ) { + Py_DECREF(vitem); + Py_DECREF(witem); + Py_RETURN_FALSE; + } + if (op == Py_NE) { + Py_DECREF(vitem); + Py_DECREF(witem); + Py_RETURN_TRUE; + } + /* Compare the differing items using the proper operator */ + PyObject *result = PyObject_RichCompare(vitem, witem, op); + Py_DECREF(vitem); + Py_DECREF(witem); + return result; } Py_DECREF(vitem); Py_DECREF(witem); - vitem = witem = NULL; - } - - if (vitem == NULL) { - /* All compared elements were equal -- compare sizes */ - Py_RETURN_RICHCOMPARE(Py_SIZE(vl), Py_SIZE(wl), op); } - /* We have an item that differs -- shortcuts for EQ/NE */ - if (op == Py_EQ) { - Py_DECREF(vitem); - Py_DECREF(witem); - Py_RETURN_FALSE; - } - if (op == Py_NE) { - Py_DECREF(vitem); - Py_DECREF(witem); - Py_RETURN_TRUE; - } - - /* Compare the differing items using the proper operator */ - PyObject *result = PyObject_RichCompare(vitem, witem, op); - Py_DECREF(vitem); - Py_DECREF(witem); - return result; + /* All compared elements were equal -- compare sizes */ + Py_RETURN_RICHCOMPARE(Py_SIZE(vl), Py_SIZE(wl), op); } static PyObject * From 21706fc55c6bf6b9e7ae79e48adaebfb37f518f1 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Tue, 14 Apr 2026 08:58:07 -0400 Subject: [PATCH 3/6] test: update test_equal_operator_modifying_operand --- Lib/test/test_list.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py index 642b54d34849da..fb0fc945761e05 100644 --- a/Lib/test/test_list.py +++ b/Lib/test/test_list.py @@ -245,7 +245,7 @@ def __eq__(self, other): list1 = [X()] list2 = [Y()] - self.assertTrue(list1 == list2) + self.assertFalse(list1 == list2) list3 = [Z()] list4 = [1] From 51ddb474495ba360b653105f622670b5a9ea17ad Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Mon, 13 Apr 2026 17:32:27 -0400 Subject: [PATCH 4/6] news: add entry --- .../2026-04-13-00-00-00.gh-issue-148442.aBcDeF.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-00-00-00.gh-issue-148442.aBcDeF.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-00-00-00.gh-issue-148442.aBcDeF.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-00-00-00.gh-issue-148442.aBcDeF.rst new file mode 100644 index 00000000000000..e30a3dee6f54dd --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-13-00-00-00.gh-issue-148442.aBcDeF.rst @@ -0,0 +1,4 @@ +Fix a race condition in ``list.__lt__``, ``list.__le__``, ``list.__gt__``, +and ``list.__ge__``. Previously, under concurrent list mutation, the items +being compared could be replaced with other objects before the final comparison, +causing the final comparison result to be incorrect. From b4ebf3486c20c55b39bd93427cdf25b41d3c13fd Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Sat, 30 May 2026 17:39:33 +0200 Subject: [PATCH 5/6] test: add regression test --- Lib/test/test_free_threading/test_list.py | 121 ++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/Lib/test/test_free_threading/test_list.py b/Lib/test/test_free_threading/test_list.py index 0ede4df103f728..d0d8fa33944f84 100644 --- a/Lib/test/test_free_threading/test_list.py +++ b/Lib/test/test_free_threading/test_list.py @@ -1,3 +1,4 @@ +import threading import unittest from threading import Thread, Barrier @@ -171,5 +172,125 @@ def size_function(): pass + def test_richcompare_stale_element_list1(self) -> None: + # gh-148442: list_richcompare_impl must keep references to the + # captured items for the final ordering comparison, not re-read the + # list slots after the critical section may have been suspended. + # + # list1 = [x, 1], list2 = [0, 0] with x > 0. + # During list1 > list2, list1[0] is mutated from x to 0. + # The result must be True (x > 0), not False (0 > 0). + + class Value: + def __init__(self, v: int) -> None: + self.v = v + + def __eq__(self, other: object) -> bool: + if isinstance(other, Value): + return self.v == other.v + return NotImplemented + + def __gt__(self, other: object) -> bool: + if isinstance(other, Value): + return self.v > other.v + return NotImplemented + + def __hash__(self) -> int: + return hash(self.v) + + eq_started = threading.Event() + swap_done = threading.Event() + + class SignalingValue(Value): + # Value whose __eq__ signals eq_started then waits for swap_done, + # giving another thread the window to mutate the list slot. + def __eq__(self, other: object) -> bool: + eq_started.set() + swap_done.wait() + return super().__eq__(other) + + x = SignalingValue(5) # list1[0]; x > 0 + list1: list[Value] = [x, Value(1)] + list2: list[Value] = [Value(0), Value(0)] + + result: list[bool] = [] + + def compare() -> None: + result.append(list1 > list2) + + def swap() -> None: + eq_started.wait() + list1[0] = Value(0) # replace x(5) with 0 -- must not change result + swap_done.set() + + t_cmp = Thread(target=compare) + t_swp = Thread(target=swap) + t_cmp.start() + t_swp.start() + t_cmp.join() + t_swp.join() + + # x(5) > Value(0) is True; old code would compare Value(0) > Value(0) -> False + self.assertTrue(result[0]) + + def test_richcompare_stale_element_list2(self) -> None: + # Same as test_richcompare_stale_element_list1 but list2[0] is mutated + # to a value larger than x, which would flip the result under the old code. + # + # list1 = [x, 1], list2 = [0, 0] with x > 0. + # During list1 > list2, list2[0] is mutated from 0 to 100 (> x). + # The result must be True (x > 0), not False (x > 100). + + class Value: + def __init__(self, v: int) -> None: + self.v = v + + def __eq__(self, other: object) -> bool: + if isinstance(other, Value): + return self.v == other.v + return NotImplemented + + def __gt__(self, other: object) -> bool: + if isinstance(other, Value): + return self.v > other.v + return NotImplemented + + def __hash__(self) -> int: + return hash(self.v) + + eq_started = threading.Event() + swap_done = threading.Event() + + class SignalingValue(Value): + def __eq__(self, other: object) -> bool: + eq_started.set() + swap_done.wait() + return super().__eq__(other) + + x = SignalingValue(5) # list1[0]; x > 0 + list1: list[Value] = [x, Value(1)] + list2: list[Value] = [Value(0), Value(0)] + + result: list[bool] = [] + + def compare() -> None: + result.append(list1 > list2) + + def swap() -> None: + eq_started.wait() + list2[0] = Value(100) # replace 0 with 100 (> x) -- must not change result + swap_done.set() + + t_cmp = Thread(target=compare) + t_swp = Thread(target=swap) + t_cmp.start() + t_swp.start() + t_cmp.join() + t_swp.join() + + # x(5) > Value(0) is True; old code would compare Value(5) > Value(100) -> False + self.assertTrue(result[0]) + + if __name__ == "__main__": unittest.main() From 96681112dbf79d2b569b383070d5e02b5a8ecc31 Mon Sep 17 00:00:00 2001 From: Thomas Kowalski Date: Sun, 31 May 2026 16:16:39 +0200 Subject: [PATCH 6/6] test: do not use threads --- Lib/test/test_free_threading/test_list.py | 121 ---------------------- Lib/test/test_list.py | 48 +++++++++ 2 files changed, 48 insertions(+), 121 deletions(-) diff --git a/Lib/test/test_free_threading/test_list.py b/Lib/test/test_free_threading/test_list.py index d0d8fa33944f84..0ede4df103f728 100644 --- a/Lib/test/test_free_threading/test_list.py +++ b/Lib/test/test_free_threading/test_list.py @@ -1,4 +1,3 @@ -import threading import unittest from threading import Thread, Barrier @@ -172,125 +171,5 @@ def size_function(): pass - def test_richcompare_stale_element_list1(self) -> None: - # gh-148442: list_richcompare_impl must keep references to the - # captured items for the final ordering comparison, not re-read the - # list slots after the critical section may have been suspended. - # - # list1 = [x, 1], list2 = [0, 0] with x > 0. - # During list1 > list2, list1[0] is mutated from x to 0. - # The result must be True (x > 0), not False (0 > 0). - - class Value: - def __init__(self, v: int) -> None: - self.v = v - - def __eq__(self, other: object) -> bool: - if isinstance(other, Value): - return self.v == other.v - return NotImplemented - - def __gt__(self, other: object) -> bool: - if isinstance(other, Value): - return self.v > other.v - return NotImplemented - - def __hash__(self) -> int: - return hash(self.v) - - eq_started = threading.Event() - swap_done = threading.Event() - - class SignalingValue(Value): - # Value whose __eq__ signals eq_started then waits for swap_done, - # giving another thread the window to mutate the list slot. - def __eq__(self, other: object) -> bool: - eq_started.set() - swap_done.wait() - return super().__eq__(other) - - x = SignalingValue(5) # list1[0]; x > 0 - list1: list[Value] = [x, Value(1)] - list2: list[Value] = [Value(0), Value(0)] - - result: list[bool] = [] - - def compare() -> None: - result.append(list1 > list2) - - def swap() -> None: - eq_started.wait() - list1[0] = Value(0) # replace x(5) with 0 -- must not change result - swap_done.set() - - t_cmp = Thread(target=compare) - t_swp = Thread(target=swap) - t_cmp.start() - t_swp.start() - t_cmp.join() - t_swp.join() - - # x(5) > Value(0) is True; old code would compare Value(0) > Value(0) -> False - self.assertTrue(result[0]) - - def test_richcompare_stale_element_list2(self) -> None: - # Same as test_richcompare_stale_element_list1 but list2[0] is mutated - # to a value larger than x, which would flip the result under the old code. - # - # list1 = [x, 1], list2 = [0, 0] with x > 0. - # During list1 > list2, list2[0] is mutated from 0 to 100 (> x). - # The result must be True (x > 0), not False (x > 100). - - class Value: - def __init__(self, v: int) -> None: - self.v = v - - def __eq__(self, other: object) -> bool: - if isinstance(other, Value): - return self.v == other.v - return NotImplemented - - def __gt__(self, other: object) -> bool: - if isinstance(other, Value): - return self.v > other.v - return NotImplemented - - def __hash__(self) -> int: - return hash(self.v) - - eq_started = threading.Event() - swap_done = threading.Event() - - class SignalingValue(Value): - def __eq__(self, other: object) -> bool: - eq_started.set() - swap_done.wait() - return super().__eq__(other) - - x = SignalingValue(5) # list1[0]; x > 0 - list1: list[Value] = [x, Value(1)] - list2: list[Value] = [Value(0), Value(0)] - - result: list[bool] = [] - - def compare() -> None: - result.append(list1 > list2) - - def swap() -> None: - eq_started.wait() - list2[0] = Value(100) # replace 0 with 100 (> x) -- must not change result - swap_done.set() - - t_cmp = Thread(target=compare) - t_swp = Thread(target=swap) - t_cmp.start() - t_swp.start() - t_cmp.join() - t_swp.join() - - # x(5) > Value(0) is True; old code would compare Value(5) > Value(100) -> False - self.assertTrue(result[0]) - - if __name__ == "__main__": unittest.main() diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py index fb0fc945761e05..b5ecf064e46bdb 100644 --- a/Lib/test/test_list.py +++ b/Lib/test/test_list.py @@ -262,6 +262,54 @@ def __lt__(self, other): with self.assertRaises(TypeError): a[0] < a + def test_richcompare_stale_element_list_vitem(self): + # gh-148442: list_richcompare_impl must use the captured vitem for + # the final ordering comparison, not re-read list1's slot after __eq__ + # may have mutated it. + # + # x.__eq__(0) puts AlwaysLT() into list1[0] and returns False. + class AlwaysLT: + def __eq__(self, other: object) -> bool: + return False + + def __gt__(self, other: object) -> bool: + return False + + class Mutating: + def __eq__(self, other: object) -> bool: + list1[0] = AlwaysLT() + return False + + def __gt__(self, other: object) -> bool: + return True + + list1 = [Mutating(), 0] + list2 = [0, 0] + self.assertTrue(list1 > list2) + + def test_richcompare_stale_element_list_witem(self): + # gh-148442: list_richcompare_impl must use the captured witem for + # the final ordering comparison, not re-read list2's slot after __eq__ + # may have mutated it. + # + # x.__eq__(0) puts AlwaysGT() into list2[0] and returns False. + class AlwaysGT: + pass + + class Mutating: + def __eq__(self, other: object) -> bool: + list2[0] = AlwaysGT() + return False + + def __gt__(self, other: object) -> bool: + if isinstance(other, AlwaysGT): + return False # pretend AlwaysGT beats us + return True # beat everything else (including 0) + + list1 = [Mutating(), 0] + list2 = [0, 0] + self.assertTrue(list1 > list2) + def test_list_index_modifing_operand(self): # See gh-120384 class evil: