Skip to content

Commit

Permalink
pythonGH-99729: Unlink frames before clearing them (pythonGH-100030)
Browse files Browse the repository at this point in the history
  • Loading branch information
brandtbucher authored Dec 6, 2022
1 parent 85d5a7e commit b72014c
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 11 deletions.
42 changes: 42 additions & 0 deletions Lib/test/test_frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import re
import sys
import textwrap
import threading
import types
import unittest
import weakref
Expand All @@ -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


Expand Down Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
5 changes: 4 additions & 1 deletion Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
13 changes: 4 additions & 9 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions Python/frame.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 4 additions & 1 deletion Python/generated_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit b72014c

Please sign in to comment.