gh-119004: fix a crash in equality testing between OrderedDict

Doc/library/collections.rst

@@ -1169,8 +1169,11 @@ Some differences from :class:`dict` still remain:
 In addition to the usual mapping methods, ordered dictionaries also support
 reverse iteration using :func:`reversed`.
 
+.. _collections_OrderedDict__eq__:
+
 Equality tests between :class:`OrderedDict` objects are order-sensitive
-and are implemented as ``list(od1.items())==list(od2.items())``.
+and are roughly equivalent to ``list(od1.items())==list(od2.items())``.
+
 Equality tests between :class:`OrderedDict` objects and other
 :class:`~collections.abc.Mapping` objects are order-insensitive like regular
 dictionaries.  This allows :class:`OrderedDict` objects to be substituted
@@ -1186,7 +1189,11 @@ anywhere a regular dictionary is used.
    method.
 
 .. versionchanged:: 3.9
-    Added merge (``|``) and update (``|=``) operators, specified in :pep:`584`.
+   Added merge (``|``) and update (``|=``) operators, specified in :pep:`584`.
+
+.. versionchanged:: 3.14
+   Mutating :class:`OrderedDict` objects during an equality comparison raises
+   a :exc:`RuntimeError`.
 
 
 :class:`OrderedDict` Examples and Recipes

Lib/collections/__init__.py

@@ -103,6 +103,7 @@ def __new__(cls, /, *args, **kwds):
         self.__root = root = _proxy(self.__hardroot)
         root.prev = root.next = root
         self.__map = {}
+        self.__state = 0
         return self
 
     def __init__(self, other=(), /, **kwds):
@@ -123,6 +124,7 @@ def __setitem__(self, key, value,
             link.prev, link.next, link.key = last, root, key
             last.next = link
             root.prev = proxy(link)
+            self.__state += 1
         dict_setitem(self, key, value)
 
     def __delitem__(self, key, dict_delitem=dict.__delitem__):
@@ -137,6 +139,7 @@ def __delitem__(self, key, dict_delitem=dict.__delitem__):
         link_next.prev = link_prev
         link.prev = None
         link.next = None
+        self.__state += 1
 
     def __iter__(self):
         'od.__iter__() <==> iter(od)'
@@ -160,6 +163,7 @@ def clear(self):
         'od.clear() -> None.  Remove all items from od.'
         root = self.__root
         root.prev = root.next = root
+        self.__state += 1
         self.__map.clear()
         dict.clear(self)
 
@@ -184,6 +188,7 @@ def popitem(self, last=True):
         key = link.key
         del self.__map[key]
         value = dict.pop(self, key)
+        self.__state += 1
         return key, value
 
     def move_to_end(self, key, last=True):
@@ -210,6 +215,7 @@ def move_to_end(self, key, last=True):
             link.next = first
             first.prev = soft_link
             root.next = link
+        self.__state += 1
 
     def __sizeof__(self):
         sizeof = _sys.getsizeof
@@ -218,6 +224,7 @@ def __sizeof__(self):
         size += sizeof(self.__map) * 2          # internal dict and inherited dict
         size += sizeof(self.__hardroot) * n     # link objects
         size += sizeof(self.__root) * n         # proxy objects
+        size += sizeof(self.__state)            # linked list state
         return size
 
     update = __update = _collections_abc.MutableMapping.update
@@ -255,6 +262,7 @@ def pop(self, key, default=__marker):
             link_next.prev = link_prev
             link.prev = None
             link.next = None
+            self.__state += 1
             return result
         if default is marker:
             raise KeyError(key)
@@ -314,8 +322,36 @@ def __eq__(self, other):
         while comparison to a regular mapping is order-insensitive.
 
         '''
+        # The Python implementation is not optimal but matches
+        # the C implementation and the exceptions it may raise.
         if isinstance(other, OrderedDict):
-            return dict.__eq__(self, other) and all(map(_eq, self, other))
+            if not dict.__eq__(self, other):
+                return False
+
+            count_a, count_b = len(self), len(other)
+            if count_a != count_b:
+                return False
+
+            state_a, state_b = self.__state, other.__state
+            root_a, root_b = self.__root, other.__root
+            curr_a, curr_b = root_a.next, root_b.next
+
+            while (curr_a is not root_a and curr_b is not root_b):
+                # With the C implementation, calling '==' might have side
+                # effects that would end in segmentation faults, thus the
+                # state and size of the operands need to be checked.
+                res = curr_a.key == curr_b.key
+                if self.__state != state_a or other.__state != state_b:
+                    # changing the size always changes the state
+                    if len(self) != count_a or len(other) != count_b:
+                        raise RuntimeError("OrderedDict changed size during iteration")
+                    raise RuntimeError("OrderedDict mutated during iteration")
+                elif not res:
+                    return False
+                curr_a, curr_b = curr_a.next, curr_b.next
+            # we stopped simultaneously (size was unchanged)
+            assert curr_a is root_a and curr_b is root_b
+            return True
         return dict.__eq__(self, other)
 
     def __ior__(self, other):

Lib/test/test_ordered_dict.py

@@ -2,11 +2,14 @@
 import contextlib
 import copy
 import gc
+import operator
 import pickle
+import re
 from random import randrange, shuffle
 import struct
 import sys
 import unittest
+import unittest.mock
 import weakref
 from collections.abc import MutableMapping
 from test import mapping_tests, support
@@ -612,6 +615,62 @@ def test_issue24667(self):
                 key = c0 + c1
                 od[key] = key
 
+    def test_issue119004_change_size(self):
+        count = 0
+        class ChangeExternSize:
+            def __eq__(self, other):
+                nonlocal count
+                if count == 1:
+                    lhs.clear()
+                    rhs.clear()
+                count += 1
+                return True
+            def __hash__(self):
+                return -1
+
+        lhs = self.OrderedDict(dict.fromkeys((0, ChangeExternSize())))
+        rhs = self.OrderedDict(dict.fromkeys((0, ChangeExternSize())))
+        msg = re.escape("OrderedDict changed size during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        # There are two calls to ChangeExternSize.__eq__: one when doing
+        # value comparisons (without order) by dict.__eq__ and one when
+        # doing key comparisons (for the order).
+        self.assertEqual(count, 2)
+        with unittest.mock.patch.object(ChangeExternSize, '__eq__') as fn:
+            self.assertDictEqual(dict(lhs), {})
+            fn.assert_not_called()
+            self.assertDictEqual(dict(rhs), {})
+            fn.assert_not_called()
+
+    def test_issue119004_change_item(self):
+        count = 0
+        class ChangeExternItem:
+            def __eq__(self, other):
+                nonlocal count
+                if count == 1:
+                    # change the layout of the underlying linked list,
+                    # but only in the second call (not in the first call)
+                    del lhs[self], rhs[other]
+                    lhs['a'] = rhs['b'] = 'c'
+                count += 1
+                return True
+            def __hash__(self):
+                return -1
+
+        lhs = self.OrderedDict(dict.fromkeys((0, ChangeExternItem())))
+        rhs = self.OrderedDict(dict.fromkeys((0, ChangeExternItem())))
+        msg = re.escape("OrderedDict mutated during iteration")
+        self.assertRaisesRegex(RuntimeError, msg, operator.eq, lhs, rhs)
+        # There are two calls to ChangeExternItem.__eq__: one when doing
+        # value comparisons (without order) by dict.__eq__ and one when
+        # doing key comparisons (for the order).
+        self.assertEqual(count, 2)
+        with unittest.mock.patch.object(ChangeExternItem, '__eq__') as fn:
+            self.assertDictEqual(dict(lhs), {0: None, 'a': 'c'})
+            fn.assert_not_called()
+            self.assertDictEqual(dict(rhs), {0: None, 'b': 'c'})
+            fn.assert_not_called()
+
     # Direct use of dict methods
 
     def test_dict_setitem(self):

Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst

@@ -0,0 +1,2 @@
+Fix a crash in :ref:`OrderedDict.__eq__ <collections_OrderedDict__eq__>`
+when operands are mutated during the check.  Patch by Bénédikt Tran.

Objects/odictobject.c

@@ -699,7 +699,7 @@ _odict_add_new_node(PyODictObject *od, PyObject *key, Py_hash_t hash)
 
     _odictnode_KEY(node) = key;
     _odictnode_HASH(node) = hash;
-    _odict_add_tail(od, node);
+    _odict_add_tail(od, node); // this updates 'od_state'
     od->od_fast_nodes[i] = node;
     return 0;
 }
@@ -773,7 +773,7 @@ _odict_clear_node(PyODictObject *od, _ODictNode *node, PyObject *key,
 
     // Now clear the node.
     od->od_fast_nodes[i] = NULL;
-    _odict_remove_node(od, node);
+    _odict_remove_node(od, node); // this updates 'od_state'
     _odictnode_DEALLOC(node);
     return 0;
 }
@@ -796,6 +796,7 @@ _odict_clear_nodes(PyODictObject *od)
         _odictnode_DEALLOC(node);
         node = next;
     }
+    od->od_state++;
 }
 
 /* There isn't any memory management of nodes past this point. */
@@ -806,30 +807,50 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b)
 {
     _ODictNode *node_a, *node_b;
 
+    // keep operands' state and size to detect undesired mutations
+    const Py_ssize_t size_a = PyODict_SIZE(a);
+    const Py_ssize_t size_b = PyODict_SIZE(b);
+    if (size_a == -1 || size_b == -1) {
+        return -1;
+    }
+    if (size_a != size_b) {
+        // dictionaries with different number of keys can never be equal
+        return 0;
+    }
+
+    const size_t state_a = a->od_state;
+    const size_t state_b = b->od_state;
+
     node_a = _odict_FIRST(a);
     node_b = _odict_FIRST(b);
-    while (1) {
-        if (node_a == NULL && node_b == NULL)
-            /* success: hit the end of each at the same time */
-            return 1;
-        else if (node_a == NULL || node_b == NULL)
-            /* unequal length */
+    while (node_a != NULL && node_b != NULL) {
+        int res = PyObject_RichCompareBool((PyObject *)_odictnode_KEY(node_a),
+                                           (PyObject *)_odictnode_KEY(node_b),
+                                           Py_EQ);
+        if (res < 0) {
+            return res;
+        }
+        else if (a->od_state != state_a || b->od_state != state_b) {
+            // If the size changed, then the state must also have changed
+            // since the former changes only if keys are added or removed,
+            // which in turn updates the state.
+            PyErr_SetString(PyExc_RuntimeError,
+                            (size_a != PyODict_SIZE(a) || size_b != PyODict_SIZE(b))
+                                ? "OrderedDict changed size during iteration"
+                                : "OrderedDict mutated during iteration");
+            return -1;
+        }
+        else if (res == 0) {
             return 0;
-        else {
-            int res = PyObject_RichCompareBool(
-                                            (PyObject *)_odictnode_KEY(node_a),
-                                            (PyObject *)_odictnode_KEY(node_b),
-                                            Py_EQ);
-            if (res < 0)
-                return res;
-            else if (res == 0)
-                return 0;
-
-            /* otherwise it must match, so move on to the next one */
-            node_a = _odictnode_NEXT(node_a);
-            node_b = _odictnode_NEXT(node_b);
         }
+
+        /* otherwise it must match, so move on to the next one */
+        node_a = _odictnode_NEXT(node_a);
+        node_b = _odictnode_NEXT(node_b);
     }
+    assert(node_a == NULL && node_b == NULL);
+    /* success: hit the end of each at the same time */
+    return 1;
 }