diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index a7db22007dedce..ed413f105e5b17 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 @@ -11,6 +12,7 @@ _testcapi = None from test import support +from test.support import threading_helper from test.support.script_helper import assert_python_ok @@ -329,6 +331,46 @@ def f(): if old_enabled: gc.enable() + @support.cpython_only + @threading_helper.requires_working_threading() + 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): 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. 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; }