diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 9a9e738a7cf76e..ea697a5a920f77 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1835,6 +1835,45 @@ def __del__(self): d = x.__dict__ d.clear() + def test_clear_reentrant_detached_split(self): + # gh-158198 follow-up: dict.clear() on a detached split-table dict + # must not expose (Py_EMPTY_KEYS, non-NULL ma_values) to a + # finalizer. A reentrant insert must see a consistent empty dict. + class MyObj: pass + class Reinsert: + def __del__(self): + d['reentrant'] = 1 + + x = MyObj() + x.a = Reinsert() + x.b = 'other' + + d = x.__dict__ + # Detach: the old dict becomes a heap-allocated split table + # (ma_values non-NULL, embedded == 0). + x.__dict__ = {} + d.clear() + self.assertEqual(d, {'reentrant': 1}) + + def test_clear_reentrant_detached_split_update(self): + # Same as above but with a reentrant update large enough to force + # a resize. On the buggy code this frees the values array that + # clear() is still iterating over (use-after-free / segfault). + class MyObj: pass + other = {f'k{i}': i for i in range(20)} + class Reinsert: + def __del__(self): + d.update(other) + + x = MyObj() + x.a = Reinsert() + x.b = 'other' + + d = x.__dict__ + x.__dict__ = {} + d.clear() + self.assertEqual(d, other) + class CAPITest(unittest.TestCase): diff --git a/Lib/test/test_free_threading/test_dict.py b/Lib/test/test_free_threading/test_dict.py index 2180613f689c89..a7a24759f495e4 100644 --- a/Lib/test/test_free_threading/test_dict.py +++ b/Lib/test/test_free_threading/test_dict.py @@ -5,7 +5,7 @@ from ast import Or from functools import partial -from threading import Barrier, Thread +from threading import Barrier, Event, Thread from unittest import TestCase try: @@ -437,5 +437,44 @@ def writer(): threading_helper.run_concurrently([reader, writer]) + def test_racing_dealloc_and_split_dict_resize(self): + # gh-158198: an object's inline-values __dict__ escapes to another + # thread, which converts it to a combined table (by adding a non-str + # key) while the owning thread drops the last reference to the object. + NATTRS = 20 + + class C: + def __init__(self): + for i in range(NATTRS): + setattr(self, f"a{i}", i) + + def make_pair(): + slot = [None] + done = Event() + + def owner(): + try: + for j in range(2000): + o = C() + for i in range(NATTRS): + self.assertEqual(getattr(o, f"a{i}"), i) + slot[0] = o.__dict__ + for _ in range(j % 16): + pass + del o + finally: + done.set() + + def converter(): + while not done.is_set(): + d = slot[0] + if d is not None: + d[0] = 0 + slot[0] = None + + return [owner, converter] + + threading_helper.run_concurrently(make_pair() + make_pair()) + if __name__ == "__main__": unittest.main() diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 41b90f9277d0f2..811a958d91c72b 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2276,7 +2276,6 @@ dictresize(PyDictObject *mp, UNLOCK_KEYS(oldkeys); set_keys(mp, newkeys); dictkeys_decref(oldkeys, IS_DICT_SHARED(mp)); - set_values(mp, NULL); if (oldvalues->embedded) { assert(oldvalues->embedded == 1); assert(oldvalues->valid == 1); @@ -2285,6 +2284,7 @@ dictresize(PyDictObject *mp, else { free_values(oldvalues, IS_DICT_SHARED(mp)); } + set_values(mp, NULL); } else { // oldkeys is combined. if (oldkeys->dk_kind == DICT_KEYS_GENERAL) { @@ -8016,7 +8016,7 @@ PyObject_ClearManagedDict(PyObject *obj) clear_inline_values(_PyObject_InlineValues(obj)); return; } - else if (FT_ATOMIC_LOAD_PTR_RELAXED(dict->ma_values) == + if (FT_ATOMIC_LOAD_PTR_ACQUIRE(dict->ma_values) == _PyObject_InlineValues(obj)) { // We have a materialized object which points at the inline // values. We need to materialize the keys. Nothing can modify