[Python-checkins] [3.11] GH-99729: Unlink frames before clearing them (#100047)

[pablogsal]

https://github.com/python/cpython/commit/2182a71eedbc8e95c3cf2d8c0aa2fd66c7a93db4
commit: 2182a71eedbc8e95c3cf2d8c0aa2fd66c7a93db4
branch: 3.11
author: Brandt Bucher <brandtbucher at microsoft.com>
committer: pablogsal <Pablogsal at gmail.com>
date: 2022-12-06T17:02:19Z
summary:

[3.11] GH-99729: Unlink frames before clearing them (#100047)

files:
A Misc/NEWS.d/next/Core and Builtins/2022-11-26-05-34-00.gh-issue-99729.A3ovwQ.rst
M Lib/test/test_frame.py
M Python/ceval.c
M Python/frame.c

diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py
index 4b86a60d2f4c..9cb2686f1759 100644
--- a/Lib/test/test_frame.py
+++ b/Lib/test/test_frame.py
@@ -2,11 +2,13 @@
 import re
 import sys
 import textwrap
+import threading
 import types
 import unittest
 import weakref
 
 from test import support
+from test.support import threading_helper
 from test.support.script_helper import assert_python_ok
 
 
@@ -325,6 +327,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
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-11-26-05-34-00.gh-issue-99729.A3ovwQ.rst b/Misc/NEWS.d/next/Core and Builtins/2022-11-26-05-34-00.gh-issue-99729.A3ovwQ.rst
new file mode 100644
index 000000000000..3fe21a8a21bf
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2022-11-26-05-34-00.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/ceval.c b/Python/ceval.c
index 8cbe838ddf2b..a34e4ffb72f8 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -1617,14 +1617,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;
-}
-
 /* It is only between the PRECALL instruction and the following CALL,
  * that this has any meaning.
  */
@@ -2441,7 +2433,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
             DTRACE_FUNCTION_EXIT();
             _Py_LeaveRecursiveCallTstate(tstate);
             if (!frame->is_entry) {
-                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;
             }
@@ -5833,7 +5828,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
         assert(tstate->cframe->current_frame == frame->previous);
         return NULL;
     }
-    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);
 
 resume_with_error:
     SET_LOCALS_FROM_FRAME();
diff --git a/Python/frame.c b/Python/frame.c
index d8f2f801f38c..3ea3a2ced40d 100644
--- a/Python/frame.c
+++ b/Python/frame.c
@@ -123,6 +123,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;