gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT`` (gh-123092)

diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py
index e5dba7cdc570a85ada8664e3f922554ae72f34e8..4030716efb51f9cc893910e8963c4a6b231f8cce 100644 (file)
--- a/Lib/test/test_dict.py
+++ b/Lib/test/test_dict.py
@@ -1476,6 +1476,24 @@ class DictTest(unittest.TestCase):
         gc.collect()
         self.assertTrue(gc.is_tracked(next(it)))
 
+    def test_store_evilattr(self):
+        class EvilAttr:
+            def __init__(self, d):
+                self.d = d
+
+            def __del__(self):
+                if 'attr' in self.d:
+                    del self.d['attr']
+                gc.collect()
+
+        class Obj:
+            pass
+
+        obj = Obj()
+        obj.__dict__ = {}
+        for _ in range(10):
+            obj.attr = EvilAttr(obj.__dict__)
+
     def test_str_nonstr(self):
         # cpython uses a different lookup function if the dict only contains
         # `str` keys. Make sure the unoptimized path is used when a non-`str`

diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst
new file mode 100644 (file)
index 0000000..edc3f1a
--- /dev/null
+++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst
@@ -0,0 +1 @@
+Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT``.

diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index 3e9f982ae070a302450dbca55f3f5a275fbd00f4..a30b3e37319ccf8a38adbf99985a5eecfa929f27 100644 (file)
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -1703,6 +1703,8 @@ insert_split_value(PyInterpreterState *interp, PyDictObject *mp, PyObject *key,
         uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_MODIFIED, mp, key, value);
         STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));
         mp->ma_version_tag = new_version;
+        // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault,
+        // when dict only holds the strong reference to value in ep->me_value.
         Py_DECREF(old_value);
     }
     ASSERT_CONSISTENT(mp);

diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 838af3ee3ab18f7767ce0dd77073b8026ab57709..bc418137a9f9e9830aa7bfe1bc09e9724d99615d 100644 (file)
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -2235,18 +2235,19 @@ dummy_func(
             DEOPT_IF(!DK_IS_UNICODE(dict->ma_keys));
             PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint;
             DEOPT_IF(ep->me_key != name);
+            /* Ensure dict is GC tracked if it needs to be */
+            if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
+                _PyObject_GC_TRACK(dict);
+            }
             old_value = ep->me_value;
             PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED;
             new_version = _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value));
             ep->me_value = PyStackRef_AsPyObjectSteal(value);
+            dict->ma_version_tag = new_version; // PEP 509
+            // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault,
+            // when dict only holds the strong reference to value in ep->me_value.
             Py_XDECREF(old_value);
             STAT_INC(STORE_ATTR, hit);
-            /* Ensure dict is GC tracked if it needs to be */
-            if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
-                _PyObject_GC_TRACK(dict);
-            }
-            /* PEP 509 */
-            dict->ma_version_tag = new_version;
             PyStackRef_CLOSE(owner);
         }
 

diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h
index 55b06a0e235dac2e398cc8edc2b0ca2d64d3de54..4274d51b3fa39c6956a85029dee99e30c29bbf12 100644 (file)
--- a/Python/executor_cases.c.h
+++ b/Python/executor_cases.c.h
@@ -2637,18 +2637,19 @@
                 UOP_STAT_INC(uopcode, miss);
                 JUMP_TO_JUMP_TARGET();
             }
+            /* Ensure dict is GC tracked if it needs to be */
+            if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
+                _PyObject_GC_TRACK(dict);
+            }
             old_value = ep->me_value;
             PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED;
             new_version = _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value));
             ep->me_value = PyStackRef_AsPyObjectSteal(value);
+            dict->ma_version_tag = new_version; // PEP 509
+            // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault,
+            // when dict only holds the strong reference to value in ep->me_value.
             Py_XDECREF(old_value);
             STAT_INC(STORE_ATTR, hit);
-            /* Ensure dict is GC tracked if it needs to be */
-            if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
-                _PyObject_GC_TRACK(dict);
-            }
-            /* PEP 509 */
-            dict->ma_version_tag = new_version;
             PyStackRef_CLOSE(owner);
             stack_pointer += -2;
             assert(WITHIN_STACK_BOUNDS());

diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
index 67bde83e055edecb75ebbc3f70215ed0a90e1055..181940d87fff704530ee6a6f5ee45c059d1e9fe0 100644 (file)
--- a/Python/generated_cases.c.h
+++ b/Python/generated_cases.c.h
@@ -6917,18 +6917,19 @@
                 DEOPT_IF(!DK_IS_UNICODE(dict->ma_keys), STORE_ATTR);
                 PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint;
                 DEOPT_IF(ep->me_key != name, STORE_ATTR);
+                /* Ensure dict is GC tracked if it needs to be */
+                if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
+                    _PyObject_GC_TRACK(dict);
+                }
                 old_value = ep->me_value;
                 PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED;
                 new_version = _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value));
                 ep->me_value = PyStackRef_AsPyObjectSteal(value);
+                dict->ma_version_tag = new_version; // PEP 509
+                // old_value should be DECREFed after GC track checking is done, if not, it could raise a segmentation fault,
+                // when dict only holds the strong reference to value in ep->me_value.
                 Py_XDECREF(old_value);
                 STAT_INC(STORE_ATTR, hit);
-                /* Ensure dict is GC tracked if it needs to be */
-                if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
-                    _PyObject_GC_TRACK(dict);
-                }
-                /* PEP 509 */
-                dict->ma_version_tag = new_version;
                 PyStackRef_CLOSE(owner);
             }
             stack_pointer += -2;