[Python-checkins] gh-104252: Immortalize Py_EMPTY_KEYS (gh-104253)

[ericsnowcurrently]

https://github.com/python/cpython/commit/b8f7ab5783b370004757af5a4c6e70c63dc5fe7a
commit: b8f7ab5783b370004757af5a4c6e70c63dc5fe7a
branch: main
author: Eric Snow <ericsnowcurrently at gmail.com>
committer: ericsnowcurrently <ericsnowcurrently at gmail.com>
date: 2023-05-10T07:28:40-06:00
summary:

gh-104252: Immortalize Py_EMPTY_KEYS (gh-104253)

This was missed in gh-19474.  It matters for with a per-interpreter GIL since PyDictKeysObject.dk_refcnt breaks isolation and leads to races.

files:
M Include/internal/pycore_dict_state.h
M Include/internal/pycore_runtime_init.h
M Objects/dictobject.c

diff --git a/Include/internal/pycore_dict_state.h b/Include/internal/pycore_dict_state.h
index d608088ed2d7..ece0f10ca251 100644
--- a/Include/internal/pycore_dict_state.h
+++ b/Include/internal/pycore_dict_state.h
@@ -38,6 +38,11 @@ struct _Py_dict_state {
     PyDict_WatchCallback watchers[DICT_MAX_WATCHERS];
 };
 
+#define _dict_state_INIT \
+    { \
+        .next_keys_version = 2, \
+    }
+
 
 #ifdef __cplusplus
 }
diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h
index a48461c07428..3b1444f3429b 100644
--- a/Include/internal/pycore_runtime_init.h
+++ b/Include/internal/pycore_runtime_init.h
@@ -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, \
         }, \
diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index 2ef520044340..7436c113f37c 100644
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -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
@@ -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());
@@ -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 */
@@ -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);
@@ -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);
 }
 
@@ -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;
 
@@ -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
@@ -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);
 }