Skip to content

Commit

Permalink
gh-104252: Immortalize Py_EMPTY_KEYS (gh-104253)
Browse files Browse the repository at this point in the history
This was missed in gh-19474.  It matters for with a per-interpreter GIL since PyDictKeysObject.dk_refcnt breaks isolation and leads to races.
  • Loading branch information
ericsnowcurrently authored May 10, 2023
1 parent 2dcb289 commit b8f7ab5
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 14 deletions.
5 changes: 5 additions & 0 deletions Include/internal/pycore_dict_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ struct _Py_dict_state {
PyDict_WatchCallback watchers[DICT_MAX_WATCHERS];
};

#define _dict_state_INIT \
{ \
.next_keys_version = 2, \
}


#ifdef __cplusplus
}
Expand Down
4 changes: 1 addition & 3 deletions Include/internal/pycore_runtime_init.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,7 @@ extern PyTypeObject _PyExc_MemoryError;
}, \
}, \
.dtoa = _dtoa_state_INIT(&(INTERP)), \
.dict_state = { \
.next_keys_version = 2, \
}, \
.dict_state = _dict_state_INIT, \
.func_state = { \
.next_version = 1, \
}, \
Expand Down
32 changes: 21 additions & 11 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,19 @@ _PyDict_DebugMallocStats(FILE *out)

static void free_keys_object(PyInterpreterState *interp, PyDictKeysObject *keys);

/* PyDictKeysObject has refcounts like PyObject does, so we have the
following two functions to mirror what Py_INCREF() and Py_DECREF() do.
(Keep in mind that PyDictKeysObject isn't actually a PyObject.)
Likewise a PyDictKeysObject can be immortal (e.g. Py_EMPTY_KEYS),
so we apply a naive version of what Py_INCREF() and Py_DECREF() do
for immortal objects. */

static inline void
dictkeys_incref(PyDictKeysObject *dk)
{
if (dk->dk_refcnt == _Py_IMMORTAL_REFCNT) {
return;
}
#ifdef Py_REF_DEBUG
_Py_IncRefTotal(_PyInterpreterState_GET());
#endif
Expand All @@ -312,6 +322,9 @@ dictkeys_incref(PyDictKeysObject *dk)
static inline void
dictkeys_decref(PyInterpreterState *interp, PyDictKeysObject *dk)
{
if (dk->dk_refcnt == _Py_IMMORTAL_REFCNT) {
return;
}
assert(dk->dk_refcnt > 0);
#ifdef Py_REF_DEBUG
_Py_DecRefTotal(_PyInterpreterState_GET());
Expand Down Expand Up @@ -447,7 +460,7 @@ estimate_log2_keysize(Py_ssize_t n)
* (which cannot fail and thus can do no allocation).
*/
static PyDictKeysObject empty_keys_struct = {
1, /* dk_refcnt */
_Py_IMMORTAL_REFCNT, /* dk_refcnt */
0, /* dk_log2_size */
0, /* dk_log2_index_bytes */
DICT_KEYS_UNICODE, /* dk_kind */
Expand Down Expand Up @@ -779,6 +792,7 @@ clone_combined_dict_keys(PyDictObject *orig)
assert(PyDict_Check(orig));
assert(Py_TYPE(orig)->tp_iter == (getiterfunc)dict_iter);
assert(orig->ma_values == NULL);
assert(orig->ma_keys != Py_EMPTY_KEYS);
assert(orig->ma_keys->dk_refcnt == 1);

size_t keys_size = _PyDict_KeysSize(orig->ma_keys);
Expand Down Expand Up @@ -833,7 +847,7 @@ PyObject *
PyDict_New(void)
{
PyInterpreterState *interp = _PyInterpreterState_GET();
dictkeys_incref(Py_EMPTY_KEYS);
/* We don't incref Py_EMPTY_KEYS here because it is immortal. */
return new_dict(interp, Py_EMPTY_KEYS, NULL, 0, 0);
}

Expand Down Expand Up @@ -1331,7 +1345,7 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp,
Py_DECREF(value);
return -1;
}
dictkeys_decref(interp, Py_EMPTY_KEYS);
/* We don't decref Py_EMPTY_KEYS here because it is immortal. */
mp->ma_keys = newkeys;
mp->ma_values = NULL;

Expand Down Expand Up @@ -1529,14 +1543,10 @@ dictresize(PyInterpreterState *interp, PyDictObject *mp,

// We can not use free_keys_object here because key's reference
// are moved already.
if (oldkeys != Py_EMPTY_KEYS) {
#ifdef Py_REF_DEBUG
_Py_DecRefTotal(_PyInterpreterState_GET());
_Py_DecRefTotal(_PyInterpreterState_GET());
#endif
if (oldkeys == Py_EMPTY_KEYS) {
oldkeys->dk_refcnt--;
assert(oldkeys->dk_refcnt > 0);
}
else {
assert(oldkeys->dk_kind != DICT_KEYS_SPLIT);
assert(oldkeys->dk_refcnt == 1);
#if PyDict_MAXFREELIST > 0
Expand Down Expand Up @@ -2080,8 +2090,8 @@ PyDict_Clear(PyObject *op)
dictkeys_decref(interp, oldkeys);
}
else {
assert(oldkeys->dk_refcnt == 1);
dictkeys_decref(interp, oldkeys);
assert(oldkeys->dk_refcnt == 1);
dictkeys_decref(interp, oldkeys);
}
ASSERT_CONSISTENT(mp);
}
Expand Down

0 comments on commit b8f7ab5

Please sign in to comment.