From 5f182009d1533babc139ffdec2eaa495cc13ce6e Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 29 Dec 2022 04:40:29 -0800 Subject: [PATCH 1/6] Add a failing regression test --- Lib/test/test_frame.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 40c734b6e33abe..0130166db4a64e 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -1,4 +1,5 @@ import gc +import operator import re import sys import textwrap @@ -372,6 +373,26 @@ def run(self): ) sneaky_frame_object = sneaky_frame_object.f_back + def test_entry_frames_are_invisible_during_teardown(self): + class C: + """A weakrefable class.""" + + def f(): + """Try to find globals and locals as this frame is being cleared.""" + ref = C() + # Ignore the fact that exec(C()) is a nonsense callback. We're only + # using exec here because it tries to access the current frame's + # globals and locals. If it's trying to get those from a shim frame, + # we'll crash before raising: + return weakref.ref(ref, exec) + + with support.catch_unraisable_exception() as catcher: + # Call from C, so there is a shim frame directly above f: + weak = operator.call(f) # BOOM! + # Cool, we didn't crash. Check that the callback actually happened: + self.assertIs(catcher.unraisable.exc_type, TypeError) + self.assertIsNone(weak()) + @unittest.skipIf(_testcapi is None, 'need _testcapi') class TestCAPI(unittest.TestCase): def getframe(self): From 4ff833e74dbaa6ac191f4cf0ca573d2ef8ed1868 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 29 Dec 2022 04:41:00 -0800 Subject: [PATCH 2/6] fixup --- Lib/test/test_frame.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 0130166db4a64e..6bb0144e9b1ed7 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -375,7 +375,7 @@ def run(self): def test_entry_frames_are_invisible_during_teardown(self): class C: - """A weakrefable class.""" + """A weakref'able class.""" def f(): """Try to find globals and locals as this frame is being cleared.""" From 04b77acc91c0214a1d4a1deb4ef76b327b2a2c6b Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 29 Dec 2022 04:41:22 -0800 Subject: [PATCH 3/6] Add _PyFrame_GetComplete/_PyThreadState_GetFrame --- Include/internal/pycore_frame.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h index f18723b303224f..216a860e39dc1b 100644 --- a/Include/internal/pycore_frame.h +++ b/Include/internal/pycore_frame.h @@ -153,6 +153,21 @@ _PyFrame_IsIncomplete(_PyInterpreterFrame *frame) frame->prev_instr < _PyCode_CODE(frame->f_code) + frame->f_code->_co_firsttraceable; } +static inline _PyInterpreterFrame * +_PyFrame_GetComplete(_PyInterpreterFrame *frame) +{ + while (frame && _PyFrame_IsIncomplete(frame)) { + frame = frame->previous; + } + return frame; +} + +static inline _PyInterpreterFrame * +_PyThreadState_GetFrame(PyThreadState *tstate) +{ + return _PyFrame_GetComplete(tstate->cframe->current_frame); +} + /* For use by _PyFrame_GetFrameObject Do not call directly. */ PyFrameObject * From 41c70e69f23515e72e48e2a7d54ad6fd9c1d9286 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 29 Dec 2022 04:41:59 -0800 Subject: [PATCH 4/6] Clean up how we access/walk the frame stack --- Modules/_tracemalloc.c | 13 +++---------- Modules/signalmodule.c | 5 +---- Objects/frameobject.c | 4 +--- Objects/genobject.c | 11 +++++++---- Objects/typeobject.c | 6 +++--- Python/ceval.c | 11 ++++------- Python/frame.c | 5 +---- Python/pystate.c | 9 ++------- Python/sysmodule.c | 5 +---- 9 files changed, 23 insertions(+), 46 deletions(-) diff --git a/Modules/_tracemalloc.c b/Modules/_tracemalloc.c index ac16626f2101ba..0ffa4c20fd92bc 100644 --- a/Modules/_tracemalloc.c +++ b/Modules/_tracemalloc.c @@ -347,14 +347,8 @@ traceback_get_frames(traceback_t *traceback) return; } - _PyInterpreterFrame *pyframe = tstate->cframe->current_frame; - for (;;) { - while (pyframe && _PyFrame_IsIncomplete(pyframe)) { - pyframe = pyframe->previous; - } - if (pyframe == NULL) { - break; - } + _PyInterpreterFrame *pyframe = _PyThreadState_GetFrame(tstate); + while (pyframe) { if (traceback->nframe < tracemalloc_config.max_nframe) { tracemalloc_get_frame(pyframe, &traceback->frames[traceback->nframe]); assert(traceback->frames[traceback->nframe].filename != NULL); @@ -363,8 +357,7 @@ traceback_get_frames(traceback_t *traceback) if (traceback->total_nframe < UINT16_MAX) { traceback->total_nframe++; } - - pyframe = pyframe->previous; + pyframe = _PyFrame_GetComplete(pyframe->previous); } } diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 538a7e85bc950c..44a5ecf63e9d1e 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -1803,10 +1803,7 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate) */ _Py_atomic_store(&is_tripped, 0); - _PyInterpreterFrame *frame = tstate->cframe->current_frame; - while (frame && _PyFrame_IsIncomplete(frame)) { - frame = frame->previous; - } + _PyInterpreterFrame *frame = _PyThreadState_GetFrame(tstate); signal_state_t *state = &signal_global_state; for (int i = 1; i < Py_NSIG; i++) { if (!_Py_atomic_load_relaxed(&Handlers[i].tripped)) { diff --git a/Objects/frameobject.c b/Objects/frameobject.c index eab85c08fc0165..10721790f6b206 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1408,9 +1408,7 @@ PyFrame_GetBack(PyFrameObject *frame) PyFrameObject *back = frame->f_back; if (back == NULL) { _PyInterpreterFrame *prev = frame->f_frame->previous; - while (prev && _PyFrame_IsIncomplete(prev)) { - prev = prev->previous; - } + prev = _PyFrame_GetComplete(prev); if (prev) { back = _PyFrame_GetFrameObject(prev); } diff --git a/Objects/genobject.c b/Objects/genobject.c index c006f1af2177f9..eb352dc257d653 100644 --- a/Objects/genobject.c +++ b/Objects/genobject.c @@ -903,8 +903,11 @@ _Py_MakeCoro(PyFunctionObject *func) if (origin_depth == 0) { ((PyCoroObject *)coro)->cr_origin_or_finalizer = NULL; } else { - assert(_PyEval_GetFrame()); - PyObject *cr_origin = compute_cr_origin(origin_depth, _PyEval_GetFrame()->previous); + _PyInterpreterFrame *frame = tstate->cframe->current_frame; + assert(frame); + assert(_PyFrame_IsIncomplete(frame)); + frame = _PyFrame_GetComplete(frame->previous); + PyObject *cr_origin = compute_cr_origin(origin_depth, frame); ((PyCoroObject *)coro)->cr_origin_or_finalizer = cr_origin; if (!cr_origin) { Py_DECREF(coro); @@ -1286,7 +1289,7 @@ compute_cr_origin(int origin_depth, _PyInterpreterFrame *current_frame) /* First count how many frames we have */ int frame_count = 0; for (; frame && frame_count < origin_depth; ++frame_count) { - frame = frame->previous; + frame = _PyFrame_GetComplete(frame->previous); } /* Now collect them */ @@ -1305,7 +1308,7 @@ compute_cr_origin(int origin_depth, _PyInterpreterFrame *current_frame) return NULL; } PyTuple_SET_ITEM(cr_origin, i, frameinfo); - frame = frame->previous; + frame = _PyFrame_GetComplete(frame->previous); } return cr_origin; diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 16b1a3035d56f1..c6f7ff05176a2d 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -9579,13 +9579,13 @@ super_init_impl(PyObject *self, PyTypeObject *type, PyObject *obj) { /* Call super(), without args -- fill in from __class__ and first local variable on the stack. */ PyThreadState *tstate = _PyThreadState_GET(); - _PyInterpreterFrame *cframe = tstate->cframe->current_frame; - if (cframe == NULL) { + _PyInterpreterFrame *frame = _PyThreadState_GetFrame(tstate); + if (frame == NULL) { PyErr_SetString(PyExc_RuntimeError, "super(): no current frame"); return -1; } - int res = super_init_without_args(cframe, cframe->f_code, &type, &obj); + int res = super_init_without_args(frame, frame->f_code, &type, &obj); if (res < 0) { return -1; diff --git a/Python/ceval.c b/Python/ceval.c index 45f42800d7ce58..f75d1be99de0ce 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2752,16 +2752,13 @@ _PyInterpreterFrame * _PyEval_GetFrame(void) { PyThreadState *tstate = _PyThreadState_GET(); - return tstate->cframe->current_frame; + return _PyThreadState_GetFrame(tstate); } PyFrameObject * PyEval_GetFrame(void) { _PyInterpreterFrame *frame = _PyEval_GetFrame(); - while (frame && _PyFrame_IsIncomplete(frame)) { - frame = frame->previous; - } if (frame == NULL) { return NULL; } @@ -2775,7 +2772,7 @@ PyEval_GetFrame(void) PyObject * _PyEval_GetBuiltins(PyThreadState *tstate) { - _PyInterpreterFrame *frame = tstate->cframe->current_frame; + _PyInterpreterFrame *frame = _PyThreadState_GetFrame(tstate); if (frame != NULL) { return frame->f_builtins; } @@ -2814,7 +2811,7 @@ PyObject * PyEval_GetLocals(void) { PyThreadState *tstate = _PyThreadState_GET(); - _PyInterpreterFrame *current_frame = tstate->cframe->current_frame; + _PyInterpreterFrame *current_frame = _PyThreadState_GetFrame(tstate); if (current_frame == NULL) { _PyErr_SetString(tstate, PyExc_SystemError, "frame does not exist"); return NULL; @@ -2833,7 +2830,7 @@ PyObject * PyEval_GetGlobals(void) { PyThreadState *tstate = _PyThreadState_GET(); - _PyInterpreterFrame *current_frame = tstate->cframe->current_frame; + _PyInterpreterFrame *current_frame = _PyThreadState_GetFrame(tstate); if (current_frame == NULL) { return NULL; } diff --git a/Python/frame.c b/Python/frame.c index b1525cca511224..2b4f3c714523ae 100644 --- a/Python/frame.c +++ b/Python/frame.c @@ -96,10 +96,7 @@ take_ownership(PyFrameObject *f, _PyInterpreterFrame *frame) } assert(!_PyFrame_IsIncomplete(frame)); assert(f->f_back == NULL); - _PyInterpreterFrame *prev = frame->previous; - while (prev && _PyFrame_IsIncomplete(prev)) { - prev = prev->previous; - } + _PyInterpreterFrame *prev = _PyFrame_GetComplete(frame->previous); frame->previous = NULL; if (prev) { assert(prev->owner != FRAME_OWNED_BY_CSTACK); diff --git a/Python/pystate.c b/Python/pystate.c index f52fc38b358689..ccd8cdd8854124 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1302,10 +1302,7 @@ PyFrameObject* PyThreadState_GetFrame(PyThreadState *tstate) { assert(tstate != NULL); - _PyInterpreterFrame *f = tstate->cframe->current_frame; - while (f && _PyFrame_IsIncomplete(f)) { - f = f->previous; - } + _PyInterpreterFrame *f = _PyThreadState_GetFrame(tstate); if (f == NULL) { return NULL; } @@ -1431,9 +1428,7 @@ _PyThread_CurrentFrames(void) PyThreadState *t; for (t = i->threads.head; t != NULL; t = t->next) { _PyInterpreterFrame *frame = t->cframe->current_frame; - while (frame && _PyFrame_IsIncomplete(frame)) { - frame = frame->previous; - } + frame = _PyFrame_GetComplete(frame); if (frame == NULL) { continue; } diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 3f0baf98890b44..99b37c29f03fed 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -1884,13 +1884,10 @@ sys__getframe_impl(PyObject *module, int depth) if (frame != NULL) { while (depth > 0) { - frame = frame->previous; + frame = _PyFrame_GetComplete(frame->previous); if (frame == NULL) { break; } - if (_PyFrame_IsIncomplete(frame)) { - continue; - } --depth; } } From 53d066efd626545769821e41664ca7a819cfd174 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 29 Dec 2022 04:42:04 -0800 Subject: [PATCH 5/6] blurb add --- .../2022-12-29-04-39-38.gh-issue-100126.pfFJd-.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-12-29-04-39-38.gh-issue-100126.pfFJd-.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-12-29-04-39-38.gh-issue-100126.pfFJd-.rst b/Misc/NEWS.d/next/Core and Builtins/2022-12-29-04-39-38.gh-issue-100126.pfFJd-.rst new file mode 100644 index 00000000000000..0ec14253ceff8f --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-12-29-04-39-38.gh-issue-100126.pfFJd-.rst @@ -0,0 +1,3 @@ +Fix an issue where "incomplete" frames could be briefly visible to C code +while other frames are being torn down, possibly resulting in corruption or +hard crashes of the interpreter while running finalizers. From 3c8830b0e8393c1fb6895bac08e7d181a87ca4e1 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Mon, 9 Jan 2023 11:56:08 -0800 Subject: [PATCH 6/6] _PyFrame_GetComplete -> _PyFrame_GetFirstComplete --- Include/internal/pycore_frame.h | 4 ++-- Modules/_tracemalloc.c | 2 +- Objects/frameobject.c | 2 +- Objects/genobject.c | 6 +++--- Python/frame.c | 2 +- Python/pystate.c | 2 +- Python/sysmodule.c | 2 +- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h index 26064c66287b66..f12b225ebfccf2 100644 --- a/Include/internal/pycore_frame.h +++ b/Include/internal/pycore_frame.h @@ -167,7 +167,7 @@ _PyFrame_IsIncomplete(_PyInterpreterFrame *frame) } static inline _PyInterpreterFrame * -_PyFrame_GetComplete(_PyInterpreterFrame *frame) +_PyFrame_GetFirstComplete(_PyInterpreterFrame *frame) { while (frame && _PyFrame_IsIncomplete(frame)) { frame = frame->previous; @@ -178,7 +178,7 @@ _PyFrame_GetComplete(_PyInterpreterFrame *frame) static inline _PyInterpreterFrame * _PyThreadState_GetFrame(PyThreadState *tstate) { - return _PyFrame_GetComplete(tstate->cframe->current_frame); + return _PyFrame_GetFirstComplete(tstate->cframe->current_frame); } /* For use by _PyFrame_GetFrameObject diff --git a/Modules/_tracemalloc.c b/Modules/_tracemalloc.c index 0ffa4c20fd92bc..9826ad2935beaa 100644 --- a/Modules/_tracemalloc.c +++ b/Modules/_tracemalloc.c @@ -357,7 +357,7 @@ traceback_get_frames(traceback_t *traceback) if (traceback->total_nframe < UINT16_MAX) { traceback->total_nframe++; } - pyframe = _PyFrame_GetComplete(pyframe->previous); + pyframe = _PyFrame_GetFirstComplete(pyframe->previous); } } diff --git a/Objects/frameobject.c b/Objects/frameobject.c index c90393185452e5..39ccca70f9cbf3 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1405,7 +1405,7 @@ PyFrame_GetBack(PyFrameObject *frame) PyFrameObject *back = frame->f_back; if (back == NULL) { _PyInterpreterFrame *prev = frame->f_frame->previous; - prev = _PyFrame_GetComplete(prev); + prev = _PyFrame_GetFirstComplete(prev); if (prev) { back = _PyFrame_GetFrameObject(prev); } diff --git a/Objects/genobject.c b/Objects/genobject.c index 8874ded77078f1..2adb1e4bf308e4 100644 --- a/Objects/genobject.c +++ b/Objects/genobject.c @@ -906,7 +906,7 @@ _Py_MakeCoro(PyFunctionObject *func) _PyInterpreterFrame *frame = tstate->cframe->current_frame; assert(frame); assert(_PyFrame_IsIncomplete(frame)); - frame = _PyFrame_GetComplete(frame->previous); + frame = _PyFrame_GetFirstComplete(frame->previous); PyObject *cr_origin = compute_cr_origin(origin_depth, frame); ((PyCoroObject *)coro)->cr_origin_or_finalizer = cr_origin; if (!cr_origin) { @@ -1289,7 +1289,7 @@ compute_cr_origin(int origin_depth, _PyInterpreterFrame *current_frame) /* First count how many frames we have */ int frame_count = 0; for (; frame && frame_count < origin_depth; ++frame_count) { - frame = _PyFrame_GetComplete(frame->previous); + frame = _PyFrame_GetFirstComplete(frame->previous); } /* Now collect them */ @@ -1308,7 +1308,7 @@ compute_cr_origin(int origin_depth, _PyInterpreterFrame *current_frame) return NULL; } PyTuple_SET_ITEM(cr_origin, i, frameinfo); - frame = _PyFrame_GetComplete(frame->previous); + frame = _PyFrame_GetFirstComplete(frame->previous); } return cr_origin; diff --git a/Python/frame.c b/Python/frame.c index 2b4f3c714523ae..6a287d4724051a 100644 --- a/Python/frame.c +++ b/Python/frame.c @@ -96,7 +96,7 @@ take_ownership(PyFrameObject *f, _PyInterpreterFrame *frame) } assert(!_PyFrame_IsIncomplete(frame)); assert(f->f_back == NULL); - _PyInterpreterFrame *prev = _PyFrame_GetComplete(frame->previous); + _PyInterpreterFrame *prev = _PyFrame_GetFirstComplete(frame->previous); frame->previous = NULL; if (prev) { assert(prev->owner != FRAME_OWNED_BY_CSTACK); diff --git a/Python/pystate.c b/Python/pystate.c index ccd8cdd8854124..f2f571faf401bf 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1428,7 +1428,7 @@ _PyThread_CurrentFrames(void) PyThreadState *t; for (t = i->threads.head; t != NULL; t = t->next) { _PyInterpreterFrame *frame = t->cframe->current_frame; - frame = _PyFrame_GetComplete(frame); + frame = _PyFrame_GetFirstComplete(frame); if (frame == NULL) { continue; } diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 99b37c29f03fed..acee794864f916 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -1884,7 +1884,7 @@ sys__getframe_impl(PyObject *module, int depth) if (frame != NULL) { while (depth > 0) { - frame = _PyFrame_GetComplete(frame->previous); + frame = _PyFrame_GetFirstComplete(frame->previous); if (frame == NULL) { break; }