Use after free in `PyDict_Clear()` due to re-entrancy

[colesbury]

Crash report

The implementation of PyDict_Clear() isn't safe for dictionaries with values that are embedded in an object:

The calls to Py_CLEAR(oldvalues->values[i]) can execute arbitrary code via object destructors. oldvalues may no longer be valid because the object its embedded in may be freed during the Py_CLEAR() call.

cpython/Objects/dictobject.c

Lines 2868 to 2880 in 0ef4ffe

	n = oldkeys->dk_nentries;
	for (i = 0; i < n; i++) {
	Py_CLEAR(oldvalues->values[i]);
	}
	if (oldvalues->embedded) {
	oldvalues->size = 0;
	}
	else {
	set_values(mp, NULL);
	set_keys(mp, Py_EMPTY_KEYS);
	free_values(oldvalues, IS_DICT_SHARED(mp));
	dictkeys_decref(interp, oldkeys, false);
	}

Repro

./configure --with-address-sanitizer --with-pydebug --without-pymalloc

class MyObj:
    pass

class DelXOnDelete:
    def __del__(self):
        global x
        del x

x = MyObj()
x.a = DelXOnDelete()

d = x.__dict__
d.clear()

==3794958==ERROR: AddressSanitizer: heap-use-after-free on address 0x513000022ec2 at pc 0x56320dddb439 bp 0x7fff8205bb90 sp 0x7fff8205bb88
READ of size 1 at 0x513000022ec2 thread T0
    #0 0x56320dddb438 in clear_lock_held /raid/sgross/cpython/Objects/dictobject.c:2872:24
    #1 0x56320dddb438 in PyDict_Clear /raid/sgross/cpython/Objects/dictobject.c:2889:5
    #2 0x56320ddf7e38 in dict_clear_impl /raid/sgross/cpython/Objects/dictobject.c:4452:5
    #3 0x56320ddf7e38 in dict_clear /raid/sgross/cpython/Objects/clinic/dictobject.c.h:157:12
...

freed by thread T0 here:
    #0 0x56320db400e6 in free (/raid/sgross/cpython/python+0x2fd0e6) (BuildId: f9a13329ff2de3d6fd6a01eae843cc226d0d8f7c)
    #1 0x56320de99e6a in subtype_dealloc /raid/sgross/cpython/Objects/typeobject.c:2611:5
    #2 0x56320de31448 in _Py_Dealloc /raid/sgross/cpython/Objects/object.c:3004:5
    #3 0x56320dddc0e6 in Py_DECREF /raid/sgross/cpython/./Include/refcount.h:393:9
    #4 0x56320dddc0e6 in _PyDict_Pop_KnownHash /raid/sgross/cpython/Objects/dictobject.c:3028:9
    #5 0x56320e064bd4 in _PyEval_EvalFrameDefault /raid/sgross/cpython/Python/generated_cases.c.h:5006:23
...

cc @DinoV @markshannon

[colesbury]

I don't see a way to fix this without copying oldvalues to a temporary array.

[DinoV]

We should detach the object from the dictionary when the object is freed (which should involve copying the values and assigning a new values).

So couldn't we check in the clear loop if the values have changed from oldvalues and if so then start over with the new oldvalues? Maybe only do that once in the embedded case as there's probably also recursion issues where clearing inserts into the dictionary and causes it to resize :P

[colesbury]

That might work, but I'm a bit unsure about it:

• The list.clear(), set.clear(), and dict.clear() prior to 3.13 ensured that the container was empty before decref'ing any previous elements. That makes the clear more atomic both with respect to re-entrancy and other threads.
• oldvalues may be a dangling pointer, in which case a check like oldvalues != mp->ma_values would technically be undefined behavior. I'm also not sure what would happen if the new mp->ma_values happened to be allocated at the same address as the now freed oldvalues.
• The dict seems like it'd be in an inconsistent state: do we set PyDictValues.size to zero before or after we start clearing elements?