https://github.com/python/cpython/commit/1d70dd257b3c14fd9af88dfdb87741c573d11448 commit: 1d70dd257b3c14fd9af88dfdb87741c573d11448 branch: 3.12 author: Miss Islington (bot) <[email protected]> committer: Yhg1s <[email protected]> date: 2024-09-27T11:33:44-07:00 summary:
[3.12] gh-119004: fix a crash in equality testing between `OrderedDict` (GH-121329) (#124508) gh-119004: fix a crash in equality testing between `OrderedDict` (GH-121329) (cherry picked from commit 38a887dc3ec52c4a7222279bf4b3ca2431b86de9) Co-authored-by: Bénédikt Tran <[email protected]> files: A Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst M Doc/library/collections.rst M Lib/test/test_ordered_dict.py M Objects/odictobject.c diff --git a/Doc/library/collections.rst b/Doc/library/collections.rst index f7ee0ce743515d..0cb1a63b614603 100644 --- a/Doc/library/collections.rst +++ b/Doc/library/collections.rst @@ -1163,8 +1163,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 @@ -1180,7 +1183,7 @@ 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`. :class:`OrderedDict` Examples and Recipes diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 4571b23dfe7c1a..9f131a9110dccb 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -2,7 +2,9 @@ import contextlib import copy import gc +import operator import pickle +import re from random import randrange, shuffle import struct import sys @@ -739,11 +741,44 @@ def test_ordered_dict_items_result_gc(self): # when it's mutated and returned from __next__: self.assertTrue(gc.is_tracked(next(it))) + +class _TriggerSideEffectOnEqual: + count = 0 # number of calls to __eq__ + trigger = 1 # count value when to trigger side effect + + def __eq__(self, other): + if self.__class__.count == self.__class__.trigger: + self.side_effect() + self.__class__.count += 1 + return True + + def __hash__(self): + # all instances represent the same key + return -1 + + def side_effect(self): + raise NotImplementedError + class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase): module = py_coll OrderedDict = py_coll.OrderedDict + def test_issue119004_attribute_error(self): + class Key(_TriggerSideEffectOnEqual): + def side_effect(self): + del dict1[TODEL] + + TODEL = Key() + dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2))) + dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + # This causes an AttributeError due to the linked list being changed + msg = re.escape("'NoneType' object has no attribute 'key'") + self.assertRaisesRegex(AttributeError, msg, operator.eq, dict1, dict2) + self.assertEqual(Key.count, 2) + self.assertDictEqual(dict1, dict.fromkeys((0, 4.2))) + self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + class CPythonBuiltinDictTests(unittest.TestCase): """Builtin dict preserves insertion order. @@ -764,8 +799,85 @@ class CPythonBuiltinDictTests(unittest.TestCase): del method +class CPythonOrderedDictSideEffects: + + def check_runtime_error_issue119004(self, dict1, dict2): + msg = re.escape("OrderedDict mutated during iteration") + self.assertRaisesRegex(RuntimeError, msg, operator.eq, dict1, dict2) + + def test_issue119004_change_size_by_clear(self): + class Key(_TriggerSideEffectOnEqual): + def side_effect(self): + dict1.clear() + + dict1 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + self.check_runtime_error_issue119004(dict1, dict2) + self.assertEqual(Key.count, 2) + self.assertDictEqual(dict1, {}) + self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + + def test_issue119004_change_size_by_delete_key(self): + class Key(_TriggerSideEffectOnEqual): + def side_effect(self): + del dict1[TODEL] + + TODEL = Key() + dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2))) + dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + self.check_runtime_error_issue119004(dict1, dict2) + self.assertEqual(Key.count, 2) + self.assertDictEqual(dict1, dict.fromkeys((0, 4.2))) + self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + + def test_issue119004_change_linked_list_by_clear(self): + class Key(_TriggerSideEffectOnEqual): + def side_effect(self): + dict1.clear() + dict1['a'] = dict1['b'] = 'c' + + dict1 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + self.check_runtime_error_issue119004(dict1, dict2) + self.assertEqual(Key.count, 2) + self.assertDictEqual(dict1, dict.fromkeys(('a', 'b'), 'c')) + self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + + def test_issue119004_change_linked_list_by_delete_key(self): + class Key(_TriggerSideEffectOnEqual): + def side_effect(self): + del dict1[TODEL] + dict1['a'] = 'c' + + TODEL = Key() + dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2))) + dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + self.check_runtime_error_issue119004(dict1, dict2) + self.assertEqual(Key.count, 2) + self.assertDictEqual(dict1, {0: None, 'a': 'c', 4.2: None}) + self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + + def test_issue119004_change_size_by_delete_key_in_dict_eq(self): + class Key(_TriggerSideEffectOnEqual): + trigger = 0 + def side_effect(self): + del dict1[TODEL] + + TODEL = Key() + dict1 = self.OrderedDict(dict.fromkeys((0, TODEL, 4.2))) + dict2 = self.OrderedDict(dict.fromkeys((0, Key(), 4.2))) + self.assertEqual(Key.count, 0) + # the side effect is in dict.__eq__ and modifies the length + self.assertNotEqual(dict1, dict2) + self.assertEqual(Key.count, 2) + self.assertDictEqual(dict1, dict.fromkeys((0, 4.2))) + self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + + @unittest.skipUnless(c_coll, 'requires the C version of the collections module') -class CPythonOrderedDictTests(OrderedDictTests, unittest.TestCase): +class CPythonOrderedDictTests(OrderedDictTests, + CPythonOrderedDictSideEffects, + unittest.TestCase): module = c_coll OrderedDict = c_coll.OrderedDict diff --git a/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst b/Misc/NEWS.d/next/Library/2024-07-03-14-23-04.gh-issue-119004.L5MoUu.rst new file mode 100644 index 00000000000000..899bd163d36644 --- /dev/null +++ b/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. diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 39b0f684510578..cf364c13b91cb7 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -789,6 +789,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. */ @@ -799,24 +800,40 @@ _odict_keys_equal(PyODictObject *a, PyODictObject *b) { _ODictNode *node_a, *node_b; + // keep operands' state to detect undesired mutations + 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) + 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) + } + else if (node_a == NULL || node_b == NULL) { /* unequal length */ return 0; + } else { - int res = PyObject_RichCompareBool( - (PyObject *)_odictnode_KEY(node_a), - (PyObject *)_odictnode_KEY(node_b), - Py_EQ); - if (res < 0) + PyObject *key_a = Py_NewRef(_odictnode_KEY(node_a)); + PyObject *key_b = Py_NewRef(_odictnode_KEY(node_b)); + int res = PyObject_RichCompareBool(key_a, key_b, Py_EQ); + Py_DECREF(key_a); + Py_DECREF(key_b); + if (res < 0) { return res; - else if (res == 0) + } + else if (a->od_state != state_a || b->od_state != state_b) { + PyErr_SetString(PyExc_RuntimeError, + "OrderedDict mutated during iteration"); + return -1; + } + else if (res == 0) { + // This check comes after the check on the state + // in order for the exception to be set correctly. return 0; + } /* otherwise it must match, so move on to the next one */ node_a = _odictnode_NEXT(node_a); _______________________________________________ Python-checkins mailing list -- [email protected] To unsubscribe send an email to [email protected] https://mail.python.org/mailman3/lists/python-checkins.python.org/ Member address: [email protected]
