[Python-checkins] GH-99729: Unlink frames before clearing them (GH-100030)
pablogsal
webhook-mailer at python.org
Tue Dec 6 09:01:50 EST 2022
https://github.com/python/cpython/commit/b72014c783e5698beb18ee1249597e510b8bcb5a
commit: b72014c783e5698beb18ee1249597e510b8bcb5a
branch: main
author: Brandt Bucher <brandtbucher at microsoft.com>
committer: pablogsal <Pablogsal at gmail.com>
date: 2022-12-06T14:01:38Z
summary:
GH-99729: Unlink frames before clearing them (GH-100030)
files:
A Misc/NEWS.d/next/Core and Builtins/2022-11-26-04-00-41.gh-issue-99729.A3ovwQ.rst
M Lib/test/test_frame.py
M Python/bytecodes.c
M Python/ceval.c
M Python/frame.c
M Python/generated_cases.c.h
diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py
index a7db22007ded..ed413f105e5b 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 000000000000..3fe21a8a21bf
--- /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 41dd1acc937d..d0480ac01eb6 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 80bfa21ad0b6..9e4179e56071 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 52f6ef428291..b1525cca5112 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 3a403824b499..0805386866b3 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;
}
More information about the Python-checkins
mailing list