From 5070ff9ff797e4bc84987f4dcda4c80e47d8a606 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Sat, 26 Nov 2022 03:55:03 -0800 Subject: [PATCH 1/4] Add failing regression test --- Lib/test/test_frame.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index a7db22007dedce..fe28aa14174638 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -2,6 +2,7 @@ import re import sys import textwrap +import threading import types import unittest import weakref @@ -329,6 +330,45 @@ def f(): if old_enabled: gc.enable() + @support.cpython_only + def test_sneaky_frame_object_teardown(self): + + class SneakyDel: + def __del__(self): + """ + Stash a reference to the entire stack for walking later. + + It may look crazy, but you'd be surprised how common this is + when using a test runner (like pytest). The typical recipe is: + ResourceWarning + -Werror + a custom sys.unraisablehook. + """ + nonlocal sneaky_frame_object + sneaky_frame_object = sys._getframe() + + class SneakyThread(threading.Thread): + """ + A separate thread isn't needed to make this code crash, but it does + make crashes more consistent, since it means sneaky_frame_object is + backed by freed memory after the thread completes! + """ + + def run(self): + """Run SneakyDel.__del__ as this frame is popped.""" + ref = SneakyDel() + + sneaky_frame_object = None + t = SneakyThread() + t.start() + t.join() + # sneaky_frame_object can be anything, really, but it's crucial that + # SneakyThread.run's frame isn't anywhere on the stack while it's being + # torn down: + self.assertIsNotNone(sneaky_frame_object) + while sneaky_frame_object is not None: + self.assertIsNot( + sneaky_frame_object.f_code, SneakyThread.run.__code__ + ) + sneaky_frame_object = sneaky_frame_object.f_back @unittest.skipIf(_testcapi is None, 'need _testcapi') class TestCAPI(unittest.TestCase): From ca87470e56a1c51862a21c58f35b72a3ac9310d9 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Sat, 26 Nov 2022 03:58:42 -0800 Subject: [PATCH 2/4] Unlink frames before clearing them --- Python/bytecodes.c | 5 ++++- Python/ceval.c | 13 ++++--------- Python/frame.c | 3 +++ Python/generated_cases.c.h | 5 ++++- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 41dd1acc937d71..d0480ac01eb610 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -619,7 +619,10 @@ dummy_func( DTRACE_FUNCTION_EXIT(); _Py_LeaveRecursiveCallPy(tstate); assert(frame != &entry_frame); - frame = cframe.current_frame = pop_frame(tstate, frame); + // GH-99729: We need to unlink the frame *before* clearing it: + _PyInterpreterFrame *dying = frame; + frame = cframe.current_frame = dying->previous; + _PyEvalFrameClearAndPop(tstate, dying); _PyFrame_StackPush(frame, retval); goto resume_frame; } diff --git a/Python/ceval.c b/Python/ceval.c index 80bfa21ad0b6f0..9e4179e56071a0 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1009,14 +1009,6 @@ trace_function_exit(PyThreadState *tstate, _PyInterpreterFrame *frame, PyObject return 0; } -static _PyInterpreterFrame * -pop_frame(PyThreadState *tstate, _PyInterpreterFrame *frame) -{ - _PyInterpreterFrame *prev_frame = frame->previous; - _PyEvalFrameClearAndPop(tstate, frame); - return prev_frame; -} - int _Py_CheckRecursiveCallPy( PyThreadState *tstate) @@ -1432,7 +1424,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int assert(_PyErr_Occurred(tstate)); _Py_LeaveRecursiveCallPy(tstate); assert(frame != &entry_frame); - frame = cframe.current_frame = pop_frame(tstate, frame); + // GH-99729: We need to unlink the frame *before* clearing it: + _PyInterpreterFrame *dying = frame; + frame = cframe.current_frame = dying->previous; + _PyEvalFrameClearAndPop(tstate, dying); if (frame == &entry_frame) { /* Restore previous cframe and exit */ tstate->cframe = cframe.previous; diff --git a/Python/frame.c b/Python/frame.c index 52f6ef428291c5..b1525cca511224 100644 --- a/Python/frame.c +++ b/Python/frame.c @@ -127,6 +127,9 @@ _PyFrame_Clear(_PyInterpreterFrame *frame) * to have cleared the enclosing generator, if any. */ assert(frame->owner != FRAME_OWNED_BY_GENERATOR || _PyFrame_GetGenerator(frame)->gi_frame_state == FRAME_CLEARED); + // GH-99729: Clearing this frame can expose the stack (via finalizers). It's + // crucial that this frame has been unlinked, and is no longer visible: + assert(_PyThreadState_GET()->cframe->current_frame != frame); if (frame->frame_obj) { PyFrameObject *f = frame->frame_obj; frame->frame_obj = NULL; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 3a403824b49958..0805386866b318 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -628,7 +628,10 @@ DTRACE_FUNCTION_EXIT(); _Py_LeaveRecursiveCallPy(tstate); assert(frame != &entry_frame); - frame = cframe.current_frame = pop_frame(tstate, frame); + // GH-99729: We need to unlink the frame *before* clearing it: + _PyInterpreterFrame *dying = frame; + frame = cframe.current_frame = dying->previous; + _PyEvalFrameClearAndPop(tstate, dying); _PyFrame_StackPush(frame, retval); goto resume_frame; } From 6210626a968919eae65d9408c9f71edd9cf902af Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Sat, 26 Nov 2022 04:00:59 -0800 Subject: [PATCH 3/4] blurb add --- .../2022-11-26-04-00-41.gh-issue-99729.A3ovwQ.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-11-26-04-00-41.gh-issue-99729.A3ovwQ.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-11-26-04-00-41.gh-issue-99729.A3ovwQ.rst b/Misc/NEWS.d/next/Core and Builtins/2022-11-26-04-00-41.gh-issue-99729.A3ovwQ.rst new file mode 100644 index 00000000000000..3fe21a8a21bfe3 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-11-26-04-00-41.gh-issue-99729.A3ovwQ.rst @@ -0,0 +1,3 @@ +Fix an issue that could cause frames to be visible to Python code as they +are being torn down, possibly leading to memory corruption or hard crashes +of the interpreter. From af155c7633d69a29c987a5f17cf0614fb3687fc6 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Sat, 26 Nov 2022 05:55:59 -0800 Subject: [PATCH 4/4] Require working threading for new test --- Lib/test/test_frame.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index fe28aa14174638..ed413f105e5b17 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -12,6 +12,7 @@ _testcapi = None from test import support +from test.support import threading_helper from test.support.script_helper import assert_python_ok @@ -331,6 +332,7 @@ def f(): gc.enable() @support.cpython_only + @threading_helper.requires_working_threading() def test_sneaky_frame_object_teardown(self): class SneakyDel: