From 2775028ca663f8cd70dc9d4d5db3bd8471126a59 Mon Sep 17 00:00:00 2001 From: fatelei Date: Mon, 22 Dec 2025 15:03:46 +0800 Subject: [PATCH 1/4] gh-142734: fix OrderedDict copy heap-use-after-free security issue --- Lib/test/test_ordered_dict.py | 27 +++++++++++ ...-12-22-15-02-16.gh-issue-142734.IxVAQh.rst | 1 + Objects/odictobject.c | 45 ++++++++++++++++--- 3 files changed, 67 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 4204a6a47d2a81..81f8101fa1334f 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -10,6 +10,7 @@ import sys import unittest import weakref + from collections.abc import MutableMapping from test import mapping_tests, support from test.support import import_helper @@ -729,6 +730,32 @@ def test_merge_operator(self): with self.assertRaises(ValueError): a |= "BAD" + def test_getitem_re_entrant_clear_during_copy(self): + class Evil(self.OrderedDict): + def __getitem__(self, key): + super().clear() + return None + + evil_dict = Evil([(i, i) for i in range(4)]) + result = evil_dict.copy() + + self.assertEqual(len(result), 4) + + def test_getitem_re_entrant_modify_during_copy(self): + class Modifier(self.OrderedDict): + def __getitem__(self, key): + self['new_key'] = 'new_value' + return super().__getitem__(key) + + original = Modifier([(1, 'one'), (2, 'two')]) + result = original.copy() + + self.assertIn(1, result) + self.assertIn(2, result) + self.assertEqual(result[1], 'one') + self.assertEqual(result[2], 'two') + self.assertEqual(result["new_key"], "new_value") + @support.cpython_only def test_ordered_dict_items_result_gc(self): # bpo-42536: OrderedDict.items's tuple-reuse speed trick breaks the GC's diff --git a/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst b/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst new file mode 100644 index 00000000000000..33d561e12c8599 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst @@ -0,0 +1 @@ +fix ordereddict copy heap-use-after-free security issue diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 25928028919c9c..fa573de9366b94 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1266,18 +1266,51 @@ OrderedDict_copy_impl(PyObject *od) } } else { + Py_ssize_t i, size = PyODict_Size((PyODictObject *)od); + PyObject **keys; + if (size < 0) { + goto fail; + } + + if (size == 0) { + return od_copy; + } + + keys = PyMem_Malloc(size * sizeof(PyObject *)); + if (keys == NULL) { + PyErr_NoMemory(); + goto fail; + } + + i = 0; _odict_FOREACH(od, node) { + keys[i] = _odictnode_KEY(node); + Py_INCREF(keys[i]); + i++; + } + + for (i = 0; i < size; i++) { int res; - PyObject *value = PyObject_GetItem((PyObject *)od, - _odictnode_KEY(node)); - if (value == NULL) + PyObject *value = PyObject_GetItem((PyObject *)od, keys[i]); + if (value == NULL) { + for (Py_ssize_t j = 0; j < size; j++) { + Py_DECREF(keys[j]); + } + PyMem_Free(keys); goto fail; - res = PyObject_SetItem((PyObject *)od_copy, - _odictnode_KEY(node), value); + } + res = PyObject_SetItem((PyObject *)od_copy, keys[i], value); Py_DECREF(value); - if (res != 0) + Py_DECREF(keys[i]); + if (res != 0) { + for (; i < size; i++) { + Py_DECREF(keys[i]); + } + PyMem_Free(keys); goto fail; + } } + PyMem_Free(keys); } return od_copy; From 63a52d4a37797c5ea5c580cb902d9eb7d7cd66ff Mon Sep 17 00:00:00 2001 From: fatelei Date: Sun, 28 Dec 2025 16:49:26 +0800 Subject: [PATCH 2/4] refactor: OrderedDict copy implement --- Lib/test/test_ordered_dict.py | 46 +++++++++++-------------- Objects/odictobject.c | 64 ++++++++++++++--------------------- 2 files changed, 46 insertions(+), 64 deletions(-) diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 81f8101fa1334f..6d99192daff488 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -730,32 +730,6 @@ def test_merge_operator(self): with self.assertRaises(ValueError): a |= "BAD" - def test_getitem_re_entrant_clear_during_copy(self): - class Evil(self.OrderedDict): - def __getitem__(self, key): - super().clear() - return None - - evil_dict = Evil([(i, i) for i in range(4)]) - result = evil_dict.copy() - - self.assertEqual(len(result), 4) - - def test_getitem_re_entrant_modify_during_copy(self): - class Modifier(self.OrderedDict): - def __getitem__(self, key): - self['new_key'] = 'new_value' - return super().__getitem__(key) - - original = Modifier([(1, 'one'), (2, 'two')]) - result = original.copy() - - self.assertIn(1, result) - self.assertIn(2, result) - self.assertEqual(result[1], 'one') - self.assertEqual(result[2], 'two') - self.assertEqual(result["new_key"], "new_value") - @support.cpython_only def test_ordered_dict_items_result_gc(self): # bpo-42536: OrderedDict.items's tuple-reuse speed trick breaks the GC's @@ -900,6 +874,26 @@ def side_effect(self): self.assertDictEqual(dict1, dict.fromkeys((0, 4.2))) self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) + def test_getitem_re_entrant_clear_during_copy(self): + class Evil(self.OrderedDict): + def __getitem__(self, key): + super().clear() + return None + + evil_dict = Evil([(i, i) for i in range(4)]) + with self.assertRaises(RuntimeError): + result = evil_dict.copy() + + def test_getitem_re_entrant_modify_during_copy(self): + class Modifier(self.OrderedDict): + def __getitem__(self, key): + self['new_key'] = 'new_value' + return super().__getitem__(key) + + original = Modifier([(1, 'one'), (2, 'two')]) + with self.assertRaises(RuntimeError): + result = original.copy() + @unittest.skipUnless(c_coll, 'requires the C version of the collections module') class CPythonOrderedDictTests(OrderedDictTests, diff --git a/Objects/odictobject.c b/Objects/odictobject.c index fa573de9366b94..d9811f8f63a605 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1266,51 +1266,39 @@ OrderedDict_copy_impl(PyObject *od) } } else { - Py_ssize_t i, size = PyODict_Size((PyODictObject *)od); - PyObject **keys; - if (size < 0) { - goto fail; - } - - if (size == 0) { - return od_copy; - } - - keys = PyMem_Malloc(size * sizeof(PyObject *)); - if (keys == NULL) { - PyErr_NoMemory(); - goto fail; - } + PyODictObject *self = _PyODictObject_CAST(od); + size_t state = self->od_state; + _ODictNode *cur = _odict_FIRST(od); - i = 0; - _odict_FOREACH(od, node) { - keys[i] = _odictnode_KEY(node); - Py_INCREF(keys[i]); - i++; - } - - for (i = 0; i < size; i++) { - int res; - PyObject *value = PyObject_GetItem((PyObject *)od, keys[i]); + while (cur != NULL) { + if (self->od_state != state) { + PyErr_SetString(PyExc_RuntimeError, + "OrderedDict mutated during iteration"); + goto fail; + } + PyObject *key = Py_NewRef(_odictnode_KEY(cur)); + PyObject *value = PyObject_GetItem((PyObject *)od, key); if (value == NULL) { - for (Py_ssize_t j = 0; j < size; j++) { - Py_DECREF(keys[j]); - } - PyMem_Free(keys); + Py_DECREF(key); goto fail; } - res = PyObject_SetItem((PyObject *)od_copy, keys[i], value); - Py_DECREF(value); - Py_DECREF(keys[i]); - if (res != 0) { - for (; i < size; i++) { - Py_DECREF(keys[i]); - } - PyMem_Free(keys); + if (self->od_state != state) { + Py_DECREF(key); + Py_DECREF(value); + PyErr_SetString(PyExc_RuntimeError, + "OrderedDict mutated during iteration"); goto fail; } + if (PyObject_SetItem((PyObject *)od_copy, key, value) != 0) { + Py_DECREF(key); + Py_DECREF(value); + goto fail; + } + Py_DECREF(key); + Py_DECREF(value); + + cur = _odictnode_NEXT(cur); } - PyMem_Free(keys); } return od_copy; From e656b343bf1067e26a5c10e8b1570f0034dc0234 Mon Sep 17 00:00:00 2001 From: fatelei Date: Sun, 28 Dec 2025 22:23:18 +0800 Subject: [PATCH 3/4] chore: resolve review comment --- Lib/test/test_ordered_dict.py | 91 +++++++++++++++++++++++++++-------- Objects/odictobject.c | 17 +++---- 2 files changed, 78 insertions(+), 30 deletions(-) diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 6d99192daff488..eabbe070e772b9 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -874,26 +874,6 @@ def side_effect(self): self.assertDictEqual(dict1, dict.fromkeys((0, 4.2))) self.assertDictEqual(dict2, dict.fromkeys((0, Key(), 4.2))) - def test_getitem_re_entrant_clear_during_copy(self): - class Evil(self.OrderedDict): - def __getitem__(self, key): - super().clear() - return None - - evil_dict = Evil([(i, i) for i in range(4)]) - with self.assertRaises(RuntimeError): - result = evil_dict.copy() - - def test_getitem_re_entrant_modify_during_copy(self): - class Modifier(self.OrderedDict): - def __getitem__(self, key): - self['new_key'] = 'new_value' - return super().__getitem__(key) - - original = Modifier([(1, 'one'), (2, 'two')]) - with self.assertRaises(RuntimeError): - result = original.copy() - @unittest.skipUnless(c_coll, 'requires the C version of the collections module') class CPythonOrderedDictTests(OrderedDictTests, @@ -991,6 +971,77 @@ def test_weakref_list_is_not_traversed(self): gc.collect() + def test_getitem_re_entrant_clear_during_copy(self): + class Evil(self.OrderedDict): + def __getitem__(self, key): + super().clear() + return None + + evil_dict = Evil([(i, i) for i in range(4)]) + with self.assertRaises(RuntimeError): + result = evil_dict.copy() + + def test_getitem_re_entrant_modify_during_copy(self): + class Modifier(self.OrderedDict): + def __getitem__(self, key): + self['new_key'] = 'new_value' + return super().__getitem__(key) + + original = Modifier([(1, 'one'), (2, 'two')]) + with self.assertRaises(RuntimeError): + result = original.copy() + + def test_getitem_re_entrant_delete_during_copy(self): + class Deleter(self.OrderedDict): + call_count = 0 + def __getitem__(self, key): + Deleter.call_count += 1 + if Deleter.call_count == 1: + del self[3] + return super().__getitem__(key) + + original = Deleter([(1, 'one'), (2, 'two'), (3, 'three')]) + with self.assertRaises(RuntimeError): + result = original.copy() + + def test_getitem_re_entrant_add_during_copy(self): + class MultiAdder(self.OrderedDict): + def __getitem__(self, key): + self['new_key1'] = 'new_value1' + return super().__getitem__(key) + + original = MultiAdder([(1, 'one'), (2, 'two'), (3, 'three')]) + with self.assertRaises(RuntimeError): + result = original.copy() + + def test_getitem_re_entrant_pop_during_copy(self): + class Popper(self.OrderedDict): + call_count = 0 + def __getitem__(self, key): + Popper.call_count += 1 + if Popper.call_count == 1: + self.pop(3, None) + return super().__getitem__(key) + + original = Popper([(1, 'one'), (2, 'two'), (3, 'three')]) + with self.assertRaises(RuntimeError): + result = original.copy() + + def test_getitem_re_entrant_mixed_mutation_during_copy(self): + class MixedMutator(self.OrderedDict): + call_count = 0 + def __getitem__(self, key): + MixedMutator.call_count += 1 + if MixedMutator.call_count == 1: + del self[3] + elif MixedMutator.call_count == 2: + self['new_key'] = 'new_value' + return super().__getitem__(key) + + original = MixedMutator([(1, 'one'), (2, 'two'), (3, 'three'), (4, 'four')]) + with self.assertRaises(RuntimeError): + result = original.copy() + class PurePythonOrderedDictSubclassTests(PurePythonOrderedDictTests): diff --git a/Objects/odictobject.c b/Objects/odictobject.c index d9811f8f63a605..ab1f9310be72c6 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1268,16 +1268,16 @@ OrderedDict_copy_impl(PyObject *od) else { PyODictObject *self = _PyODictObject_CAST(od); size_t state = self->od_state; - _ODictNode *cur = _odict_FIRST(od); + _ODictNode *cur; - while (cur != NULL) { + _odict_FOREACH(od, cur) { if (self->od_state != state) { PyErr_SetString(PyExc_RuntimeError, "OrderedDict mutated during iteration"); goto fail; } PyObject *key = Py_NewRef(_odictnode_KEY(cur)); - PyObject *value = PyObject_GetItem((PyObject *)od, key); + PyObject *value = PyObject_GetItem(od, key); if (value == NULL) { Py_DECREF(key); goto fail; @@ -1289,15 +1289,12 @@ OrderedDict_copy_impl(PyObject *od) "OrderedDict mutated during iteration"); goto fail; } - if (PyObject_SetItem((PyObject *)od_copy, key, value) != 0) { - Py_DECREF(key); - Py_DECREF(value); - goto fail; - } + int rc = PyObject_SetItem(od_copy, key, value); Py_DECREF(key); Py_DECREF(value); - - cur = _odictnode_NEXT(cur); + if (rc != 0) { + goto fail; + } } } return od_copy; From 1c4ba1101d9351a12177bf9a4076ee6f21b5c55e Mon Sep 17 00:00:00 2001 From: fatelei Date: Sun, 28 Dec 2025 23:03:49 +0800 Subject: [PATCH 4/4] chore: resolve review comment --- Lib/test/test_ordered_dict.py | 81 +++++++++---------- ...-12-22-15-02-16.gh-issue-142734.IxVAQh.rst | 2 +- Objects/odictobject.c | 18 ++--- 3 files changed, 46 insertions(+), 55 deletions(-) diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index eabbe070e772b9..4b7a09f0bf9d7f 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -971,76 +971,73 @@ def test_weakref_list_is_not_traversed(self): gc.collect() - def test_getitem_re_entrant_clear_during_copy(self): - class Evil(self.OrderedDict): + def test_copy_concurrent_clear_in__getitem__(self): + class MyOD(self.OrderedDict): def __getitem__(self, key): super().clear() return None - evil_dict = Evil([(i, i) for i in range(4)]) - with self.assertRaises(RuntimeError): - result = evil_dict.copy() + od = MyOD([(i, i) for i in range(4)]) + self.assertRaises(RuntimeError, od.copy) - def test_getitem_re_entrant_modify_during_copy(self): - class Modifier(self.OrderedDict): + def test_copy_concurrent_insertion_in__getitem__(self): + class MyOD(self.OrderedDict): def __getitem__(self, key): self['new_key'] = 'new_value' return super().__getitem__(key) - original = Modifier([(1, 'one'), (2, 'two')]) - with self.assertRaises(RuntimeError): - result = original.copy() + od = MyOD([(1, 'one'), (2, 'two')]) + self.assertRaises(RuntimeError, od.copy) - def test_getitem_re_entrant_delete_during_copy(self): - class Deleter(self.OrderedDict): + def test_copy_concurrent_deletion_by_del_in__getitem__(self): + class MyOD(self.OrderedDict): call_count = 0 def __getitem__(self, key): - Deleter.call_count += 1 - if Deleter.call_count == 1: + MyOD.call_count += 1 + if MyOD.call_count == 1: del self[3] return super().__getitem__(key) - original = Deleter([(1, 'one'), (2, 'two'), (3, 'three')]) - with self.assertRaises(RuntimeError): - result = original.copy() + od = MyOD([(1, 'one'), (2, 'two'), (3, 'three')]) + self.assertRaises(RuntimeError, od.copy) - def test_getitem_re_entrant_add_during_copy(self): - class MultiAdder(self.OrderedDict): - def __getitem__(self, key): - self['new_key1'] = 'new_value1' - return super().__getitem__(key) - - original = MultiAdder([(1, 'one'), (2, 'two'), (3, 'three')]) - with self.assertRaises(RuntimeError): - result = original.copy() - - def test_getitem_re_entrant_pop_during_copy(self): - class Popper(self.OrderedDict): + def test_copy_concurrent_deletion_by_pop_in__getitem__(self): + class MyOD(self.OrderedDict): call_count = 0 def __getitem__(self, key): - Popper.call_count += 1 - if Popper.call_count == 1: + MyOD.call_count += 1 + if MyOD.call_count == 1: self.pop(3, None) return super().__getitem__(key) - original = Popper([(1, 'one'), (2, 'two'), (3, 'three')]) - with self.assertRaises(RuntimeError): - result = original.copy() + od = MyOD([(1, 'one'), (2, 'two'), (3, 'three')]) + self.assertRaises(RuntimeError, od.copy) - def test_getitem_re_entrant_mixed_mutation_during_copy(self): - class MixedMutator(self.OrderedDict): + def test_copy_concurrent_deletion_and_set_in__getitem__(self): + class MyOD(self.OrderedDict): call_count = 0 def __getitem__(self, key): - MixedMutator.call_count += 1 - if MixedMutator.call_count == 1: + MyOD.call_count += 1 + if MyOD.call_count == 1: del self[3] - elif MixedMutator.call_count == 2: + elif MyOD.call_count == 2: self['new_key'] = 'new_value' return super().__getitem__(key) - original = MixedMutator([(1, 'one'), (2, 'two'), (3, 'three'), (4, 'four')]) - with self.assertRaises(RuntimeError): - result = original.copy() + od = MyOD([(1, 'one'), (2, 'two'), (3, 'three'), (4, 'four')]) + self.assertRaises(RuntimeError, od.copy) + + def test_copy_concurrent_mutation_in__setitem__(self): + od = None + + class MyOD(self.OrderedDict): + def __setitem__(self, key, value): + if od is not None and len(od) > 1: + del od[3] + return super().__setitem__(key, value) + + od = MyOD([(1, 'one'), (2, 'two'), (3, 'three')]) + self.assertRaises(RuntimeError, od.copy) class PurePythonOrderedDictSubclassTests(PurePythonOrderedDictTests): diff --git a/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst b/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst index 33d561e12c8599..67f697622378e8 100644 --- a/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst +++ b/Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst @@ -1 +1 @@ -fix ordereddict copy heap-use-after-free security issue +:mod:`collections`: fix use-after-free crashes in :meth:`OrderedDict.copy ` when the dictionary to copy is concurrently mutated. diff --git a/Objects/odictobject.c b/Objects/odictobject.c index ab1f9310be72c6..f579e151aa890d 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1271,34 +1271,28 @@ OrderedDict_copy_impl(PyObject *od) _ODictNode *cur; _odict_FOREACH(od, cur) { - if (self->od_state != state) { - PyErr_SetString(PyExc_RuntimeError, - "OrderedDict mutated during iteration"); - goto fail; - } PyObject *key = Py_NewRef(_odictnode_KEY(cur)); PyObject *value = PyObject_GetItem(od, key); if (value == NULL) { Py_DECREF(key); goto fail; } - if (self->od_state != state) { - Py_DECREF(key); - Py_DECREF(value); - PyErr_SetString(PyExc_RuntimeError, - "OrderedDict mutated during iteration"); - goto fail; - } int rc = PyObject_SetItem(od_copy, key, value); Py_DECREF(key); Py_DECREF(value); if (rc != 0) { goto fail; } + if (self->od_state != state) { + goto invalid_state; + } } } return od_copy; +invalid_state: + PyErr_SetString(PyExc_RuntimeError, + "OrderedDict mutated during iteration"); fail: Py_DECREF(od_copy); return NULL;