Skip to content

Commit d4bc6e8

Browse files
committed
gh-130555: Fix use-after-free in dict.clear() with embedded values
1 parent 4401f23 commit d4bc6e8

File tree

3 files changed

+88
-9
lines changed

3 files changed

+88
-9
lines changed

Lib/test/test_dict.py

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1680,6 +1680,69 @@ def test_hash_collision_remove_add(self):
16801680
self.assertEqual(len(d), len(items), d)
16811681
self.assertEqual(d, dict(items))
16821682

1683+
def test_clear_reentrant_embedded(self):
1684+
# gh-130555: dict.clear() must be safe when values are embedded
1685+
# in an object and a destructor mutates the dict.
1686+
class MyObj: pass
1687+
class ClearOnDelete:
1688+
def __del__(self):
1689+
nonlocal x
1690+
del x
1691+
1692+
x = MyObj()
1693+
x.a = ClearOnDelete()
1694+
1695+
d = x.__dict__
1696+
d.clear()
1697+
1698+
def test_clear_reentrant_cycle(self):
1699+
# gh-130555: dict.clear() must be safe for embedded dicts when the
1700+
# object is part of a reference cycle and the last reference to the
1701+
# dict is via the cycle.
1702+
class MyObj: pass
1703+
obj = MyObj()
1704+
obj.f = obj
1705+
obj.attr = "attr"
1706+
1707+
d = obj.__dict__
1708+
del obj
1709+
1710+
d.clear()
1711+
1712+
def test_clear_reentrant_force_combined(self):
1713+
# gh-130555: dict.clear() must be safe when a destructor forces the
1714+
# dict from embedded/split to combined (setting ma_values to NULL).
1715+
class MyObj: pass
1716+
class ForceConvert:
1717+
def __del__(self):
1718+
d[1] = "trigger"
1719+
1720+
x = MyObj()
1721+
x.a = ForceConvert()
1722+
x.b = "other"
1723+
1724+
d = x.__dict__
1725+
d.clear()
1726+
1727+
def test_clear_reentrant_delete(self):
1728+
# gh-130555: dict.clear() must be safe when a destructor deletes
1729+
# a key from the same embedded dict.
1730+
class MyObj: pass
1731+
class DelKey:
1732+
def __del__(self):
1733+
try:
1734+
del d['b']
1735+
except KeyError:
1736+
pass
1737+
1738+
x = MyObj()
1739+
x.a = DelKey()
1740+
x.b = "value_b"
1741+
x.c = "value_c"
1742+
1743+
d = x.__dict__
1744+
d.clear()
1745+
16831746

16841747
class CAPITest(unittest.TestCase):
16851748

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix use-after-free in :meth:`dict.clear` when the dictionary values are
2+
embedded in an object and a destructor causes re-entrant mutation of the
3+
dictionary.

Objects/dictobject.c

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2969,6 +2969,21 @@ _PyDict_DelItemIf(PyObject *op, PyObject *key,
29692969
return res;
29702970
}
29712971

2972+
static void
2973+
clear_embedded_values(PyDictValues *values, Py_ssize_t nentries)
2974+
{
2975+
PyObject *refs[SHARED_KEYS_MAX_SIZE];
2976+
assert(nentries <= SHARED_KEYS_MAX_SIZE);
2977+
for (Py_ssize_t i = 0; i < nentries; i++) {
2978+
refs[i] = values->values[i];
2979+
values->values[i] = NULL;
2980+
}
2981+
values->size = 0;
2982+
for (Py_ssize_t i = 0; i < nentries; i++) {
2983+
Py_XDECREF(refs[i]);
2984+
}
2985+
}
2986+
29722987
static void
29732988
clear_lock_held(PyObject *op)
29742989
{
@@ -2997,20 +3012,18 @@ clear_lock_held(PyObject *op)
29973012
assert(oldkeys->dk_refcnt == 1);
29983013
dictkeys_decref(oldkeys, IS_DICT_SHARED(mp));
29993014
}
3015+
else if (oldvalues->embedded) {
3016+
clear_embedded_values(oldvalues, oldkeys->dk_nentries);
3017+
}
30003018
else {
3019+
set_values(mp, NULL);
3020+
set_keys(mp, Py_EMPTY_KEYS);
30013021
n = oldkeys->dk_nentries;
30023022
for (i = 0; i < n; i++) {
30033023
Py_CLEAR(oldvalues->values[i]);
30043024
}
3005-
if (oldvalues->embedded) {
3006-
oldvalues->size = 0;
3007-
}
3008-
else {
3009-
set_values(mp, NULL);
3010-
set_keys(mp, Py_EMPTY_KEYS);
3011-
free_values(oldvalues, IS_DICT_SHARED(mp));
3012-
dictkeys_decref(oldkeys, false);
3013-
}
3025+
free_values(oldvalues, IS_DICT_SHARED(mp));
3026+
dictkeys_decref(oldkeys, false);
30143027
}
30153028
ASSERT_CONSISTENT(mp);
30163029
}

0 commit comments

Comments
 (0)