From c732019fa361af4aab842ee1e79b654db2f149cb Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Sat, 17 Aug 2024 16:58:59 +0900 Subject: [PATCH 1/3] gh-123083: Fix STORE_ATTR_WITH_HINT has potential use-after-free --- Lib/test/test_dict.py | 18 ++++++++++++++++++ ...4-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst | 1 + Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 2 +- Python/generated_cases.c.h | 2 +- 5 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index e5dba7cdc570a8..4030716efb51f9 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1476,6 +1476,24 @@ def test_dict_items_result_gc_reversed(self): 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 index 00000000000000..0fc12a33440911 --- /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 STORE_ATTR_WITH_HINT has potential use-after-free. diff --git a/Python/bytecodes.c b/Python/bytecodes.c index ec57c07104d239..ce17bedf50c103 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2234,12 +2234,12 @@ dummy_func( 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); - 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); } + Py_XDECREF(old_value); /* 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 d699e8d79942e2..ebb5285f742c7a 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2636,12 +2636,12 @@ 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); - 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); } + Py_XDECREF(old_value); /* PEP 509 */ dict->ma_version_tag = new_version; PyStackRef_CLOSE(owner); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 486d356dfb922c..7296312fe3a8e1 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -6895,12 +6895,12 @@ 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); - 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); } + Py_XDECREF(old_value); /* PEP 509 */ dict->ma_version_tag = new_version; PyStackRef_CLOSE(owner); From 59ea4f26945023dd979f3db2bbec7dc87c4c66d2 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 21 Aug 2024 00:28:06 +0900 Subject: [PATCH 2/3] Address code review --- Objects/dictobject.c | 2 ++ Python/bytecodes.c | 15 ++++++++------- Python/executor_cases.c.h | 15 ++++++++------- Python/generated_cases.c.h | 15 ++++++++------- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 3e9f982ae070a3..a30b3e37319ccf 100644 --- 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 ce17bedf50c103..38b5cf4d2b649a 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2230,18 +2230,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); - 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); - 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); } + 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); - /* PEP 509 */ - dict->ma_version_tag = new_version; + STAT_INC(STORE_ATTR, hit); PyStackRef_CLOSE(owner); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index ebb5285f742c7a..f266c0efbe3279 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2632,18 +2632,19 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - 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); - 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); } + 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); - /* PEP 509 */ - dict->ma_version_tag = new_version; + STAT_INC(STORE_ATTR, hit); 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 7296312fe3a8e1..8e76cfa649fcf8 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -6891,18 +6891,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); - 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); - 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); } + 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); - /* PEP 509 */ - dict->ma_version_tag = new_version; + STAT_INC(STORE_ATTR, hit); PyStackRef_CLOSE(owner); } stack_pointer += -2; From 4237506d2a30954f8f340c8399776d28ae9fe0c7 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 21 Aug 2024 00:30:12 +0900 Subject: [PATCH 3/3] Address code review --- .../2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 0fc12a33440911..edc3f1ab6a8e17 100644 --- 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 @@ -1 +1 @@ -Fix STORE_ATTR_WITH_HINT has potential use-after-free. +Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT``.