From 6cd67e413b9a6c5c81466c4e516bca6430d7297e Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Fri, 23 Aug 2024 01:37:26 +0900 Subject: [PATCH] =?UTF-8?q?[3.13]=20gh-123083:=20Fix=20a=20potential=20use?= =?UTF-8?q?-after-free=20in=20``STORE=5FATTR=5FWITH=E2=80=A6=20(#123235)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [3.13] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT`` (gh-123092) (cherry picked from commit 297f2e093ec95800ae2184330b8408c875523467) --- Lib/test/test_dict.py | 18 ++++++++++++++++++ ...4-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst | 1 + Objects/dictobject.c | 2 ++ Python/bytecodes.c | 9 +++++---- Python/generated_cases.c.h | 9 +++++---- 5 files changed, 31 insertions(+), 8 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 e5dba7cdc57..4030716efb5 100644 --- 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 index 00000000000..edc3f1ab6a8 --- /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 05600772db7..6586e0c5c27 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1762,6 +1762,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 cac9f67d7f2..c222fcdb22f 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2170,14 +2170,15 @@ dummy_func( new_version = _PyDict_NotifyEvent(tstate->interp, PyDict_EVENT_MODIFIED, dict, name, value); ep->me_value = value; } - Py_DECREF(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(value)) { _PyObject_GC_TRACK(dict); } - /* PEP 509 */ - dict->ma_version_tag = new_version; + 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_DECREF(old_value); + STAT_INC(STORE_ATTR, hit); Py_DECREF(owner); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index fe42e20f439..dd334044d55 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -5608,14 +5608,15 @@ new_version = _PyDict_NotifyEvent(tstate->interp, PyDict_EVENT_MODIFIED, dict, name, value); ep->me_value = value; } - Py_DECREF(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(value)) { _PyObject_GC_TRACK(dict); } - /* PEP 509 */ - dict->ma_version_tag = new_version; + 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_DECREF(old_value); + STAT_INC(STORE_ATTR, hit); Py_DECREF(owner); stack_pointer += -2; DISPATCH();