From f6634aaed1c254d30be7294d1f088d21af9f43c2 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Mon, 3 Oct 2022 22:10:45 -0700 Subject: [PATCH 1/9] Add test for PyCode_NewEmpty construction --- Lib/test/test_code.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/Lib/test/test_code.py b/Lib/test/test_code.py index 2fdfdd0d309c5d..b64e7b788f9d20 100644 --- a/Lib/test/test_code.py +++ b/Lib/test/test_code.py @@ -132,6 +132,7 @@ import unittest import textwrap import weakref +import dis try: import ctypes @@ -682,6 +683,37 @@ def test_lines(self): self.check_lines(misshappen) self.check_lines(bug93662) + @cpython_only + def test_code_new_empty(self): + # If this test fails, it means that the construction of PyCode_NewEmpty + # needs to be modified! Please update this test *and* PyCode_NewEmpty, + # so that they both stay in sync. + def f(): + pass + f.__code__ = f.__code__.replace( + co_firstlineno=42, + co_code=bytes( + [ + dis.opmap["RESUME"], 0, + dis.opmap["LOAD_ASSERTION_ERROR"], 0, + dis.opmap["RAISE_VARARGS"], 1, + ] + ), + co_linetable=bytes( + [ + (1 << 7) + | (13 << 3) + | (3 - 1), + 0, + ] + ), + ) + self.assertRaises(AssertionError, f) + self.assertEqual( + list(f.__code__.co_positions()), + 3 * [(42, 42, None, None)], + ) + if check_impl_detail(cpython=True) and ctypes is not None: py = ctypes.pythonapi From 8aa5be01608a38e33fc2f3f4a12b069915bb4564 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Mon, 3 Oct 2022 22:11:45 -0700 Subject: [PATCH 2/9] Add a meaningful co_linetable to PyCode_NewEmpty --- Objects/codeobject.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 7d0d038f489a98..14d1d00684aedf 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -638,12 +638,22 @@ PyCode_New(int argcount, int kwonlyargcount, exceptiontable); } -static const char assert0[6] = { +// NOTE: When modifying the construction of PyCode_NewEmpty, please also change +// test.test_code.CodeLocationTest.test_code_new_empty to keep it in sync! + +static const uint8_t assert0[6] = { RESUME, 0, LOAD_ASSERTION_ERROR, 0, RAISE_VARARGS, 1 }; +static const uint8_t linetable[2] = { + (1 << 7) // New entry. + | (PY_CODE_LOCATION_INFO_NO_COLUMNS << 3) + | (3 - 1), // Three code units. + 0, // Offset from co_firstlineno. +}; + PyCodeObject * PyCode_NewEmpty(const char *filename, const char *funcname, int firstlineno) { @@ -651,6 +661,7 @@ PyCode_NewEmpty(const char *filename, const char *funcname, int firstlineno) PyObject *filename_ob = NULL; PyObject *funcname_ob = NULL; PyObject *code_ob = NULL; + PyObject *linetable_ob = NULL; PyCodeObject *result = NULL; nulltuple = PyTuple_New(0); @@ -665,10 +676,14 @@ PyCode_NewEmpty(const char *filename, const char *funcname, int firstlineno) if (filename_ob == NULL) { goto failed; } - code_ob = PyBytes_FromStringAndSize(assert0, 6); + code_ob = PyBytes_FromStringAndSize((const char *)assert0, 6); if (code_ob == NULL) { goto failed; } + linetable_ob = PyBytes_FromStringAndSize((const char *)linetable, 2); + if (linetable_ob == NULL) { + goto failed; + } #define emptystring (PyObject *)&_Py_SINGLETON(bytes_empty) struct _PyCodeConstructor con = { @@ -677,7 +692,7 @@ PyCode_NewEmpty(const char *filename, const char *funcname, int firstlineno) .qualname = funcname_ob, .code = code_ob, .firstlineno = firstlineno, - .linetable = emptystring, + .linetable = linetable_ob, .consts = nulltuple, .names = nulltuple, .localsplusnames = nulltuple, @@ -692,6 +707,7 @@ PyCode_NewEmpty(const char *filename, const char *funcname, int firstlineno) Py_XDECREF(funcname_ob); Py_XDECREF(filename_ob); Py_XDECREF(code_ob); + Py_XDECREF(linetable_ob); return result; } From 840fb6d624ed6a9094dc1a75f091f29845980538 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Mon, 3 Oct 2022 22:41:35 -0700 Subject: [PATCH 3/9] Make sure all PyFrameObjects are "complete" --- Objects/frameobject.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 2e377794312612..ab40e9ff581eed 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1094,6 +1094,9 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code, init_frame((_PyInterpreterFrame *)f->_f_frame_data, func, locals); f->f_frame = (_PyInterpreterFrame *)f->_f_frame_data; f->f_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT; + // This frame needs to be "complete", so act as if the first RESUME has run: + ((_PyInterpreterFrame *)f->_f_frame_data)->prev_instr = _PyCode_CODE(code); + assert(_Py_OPCODE(*f->f_frame->prev_instr) == RESUME); Py_DECREF(func); _PyObject_GC_TRACK(f); return f; From 65ab59af03e8f657475d80384fd7abfc1e2ddfc1 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Mon, 3 Oct 2022 22:42:31 -0700 Subject: [PATCH 4/9] Assert that all PyFrameObjects are "complete" --- Objects/frameobject.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index ab40e9ff581eed..d26fb0487b0097 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -588,6 +588,7 @@ first_line_not_before(int *lines, int len, int line) static PyFrameState _PyFrame_GetState(PyFrameObject *frame) { + assert(!_PyFrame_IsIncomplete(frame->f_frame)); if (frame->f_frame->stacktop == 0) { return FRAME_CLEARED; } @@ -1225,6 +1226,7 @@ _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { int PyFrame_FastToLocalsWithError(PyFrameObject *f) { + assert(!_PyFrame_IsIncomplete(f->f_frame)); if (f == NULL) { PyErr_BadInternalCall(); return -1; @@ -1240,7 +1242,7 @@ void PyFrame_FastToLocals(PyFrameObject *f) { int res; - + assert(!_PyFrame_IsIncomplete(f->f_frame)); assert(!PyErr_Occurred()); res = PyFrame_FastToLocalsWithError(f); @@ -1323,6 +1325,7 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) void PyFrame_LocalsToFast(PyFrameObject *f, int clear) { + assert(!_PyFrame_IsIncomplete(f->f_frame)); if (f && f->f_fast_as_locals && _PyFrame_GetState(f) != FRAME_CLEARED) { _PyFrame_LocalsToFast(f->f_frame, clear); f->f_fast_as_locals = 0; @@ -1333,6 +1336,7 @@ PyFrame_LocalsToFast(PyFrameObject *f, int clear) int _PyFrame_IsEntryFrame(PyFrameObject *frame) { assert(frame != NULL); + assert(!_PyFrame_IsIncomplete(frame->f_frame)); return frame->f_frame->is_entry; } @@ -1341,6 +1345,7 @@ PyCodeObject * PyFrame_GetCode(PyFrameObject *frame) { assert(frame != NULL); + assert(!_PyFrame_IsIncomplete(frame->f_frame)); PyCodeObject *code = frame->f_frame->f_code; assert(code != NULL); Py_INCREF(code); @@ -1352,6 +1357,7 @@ PyFrameObject* PyFrame_GetBack(PyFrameObject *frame) { assert(frame != NULL); + assert(!_PyFrame_IsIncomplete(frame->f_frame)); PyFrameObject *back = frame->f_back; if (back == NULL) { _PyInterpreterFrame *prev = frame->f_frame->previous; @@ -1369,24 +1375,28 @@ PyFrame_GetBack(PyFrameObject *frame) PyObject* PyFrame_GetLocals(PyFrameObject *frame) { + assert(!_PyFrame_IsIncomplete(frame->f_frame)); return frame_getlocals(frame, NULL); } PyObject* PyFrame_GetGlobals(PyFrameObject *frame) { + assert(!_PyFrame_IsIncomplete(frame->f_frame)); return frame_getglobals(frame, NULL); } PyObject* PyFrame_GetBuiltins(PyFrameObject *frame) { + assert(!_PyFrame_IsIncomplete(frame->f_frame)); return frame_getbuiltins(frame, NULL); } int PyFrame_GetLasti(PyFrameObject *frame) { + assert(!_PyFrame_IsIncomplete(frame->f_frame)); int lasti = _PyInterpreterFrame_LASTI(frame->f_frame); if (lasti < 0) { return -1; @@ -1397,6 +1407,7 @@ PyFrame_GetLasti(PyFrameObject *frame) PyObject * PyFrame_GetGenerator(PyFrameObject *frame) { + assert(!_PyFrame_IsIncomplete(frame->f_frame)); if (frame->f_frame->owner != FRAME_OWNED_BY_GENERATOR) { return NULL; } From db2526ae5b41a0193a001473397675490011d1de Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Tue, 4 Oct 2022 01:37:07 -0700 Subject: [PATCH 5/9] Forcibly "complete" all owned frames --- Objects/frameobject.c | 2 +- Python/frame.c | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index d26fb0487b0097..8faa343446eed8 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1097,7 +1097,7 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code, f->f_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT; // This frame needs to be "complete", so act as if the first RESUME has run: ((_PyInterpreterFrame *)f->_f_frame_data)->prev_instr = _PyCode_CODE(code); - assert(_Py_OPCODE(*f->f_frame->prev_instr) == RESUME); + assert(!_PyFrame_IsIncomplete((_PyInterpreterFrame *)f->_f_frame_data)); Py_DECREF(func); _PyObject_GC_TRACK(f); return f; diff --git a/Python/frame.c b/Python/frame.c index 05a8cffcb8a716..059435f9d299a4 100644 --- a/Python/frame.c +++ b/Python/frame.c @@ -70,6 +70,13 @@ take_ownership(PyFrameObject *f, _PyInterpreterFrame *frame) frame = (_PyInterpreterFrame *)f->_f_frame_data; f->f_frame = frame; frame->owner = FRAME_OWNED_BY_FRAME_OBJECT; + if (_PyFrame_IsIncomplete(frame)) { + // This may be a newly-created generator or coroutine frame. Since it's + // dead anyways, just pretend that the first RESUME has run: + PyCodeObject *code = frame->f_code; + frame->prev_instr = _PyCode_CODE(code) + code->_co_firsttraceable; + } + assert(!_PyFrame_IsIncomplete(frame)); assert(f->f_back == NULL); _PyInterpreterFrame *prev = frame->previous; while (prev && _PyFrame_IsIncomplete(prev)) { From b6d7e37ba439f68fdd65843c9b5f91b82021d155 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Tue, 4 Oct 2022 01:55:41 -0700 Subject: [PATCH 6/9] Fix completion in PyFrame_New --- Objects/frameobject.c | 8 ++++---- Python/frame.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 8faa343446eed8..ee92b8b271fc89 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1095,9 +1095,9 @@ PyFrame_New(PyThreadState *tstate, PyCodeObject *code, init_frame((_PyInterpreterFrame *)f->_f_frame_data, func, locals); f->f_frame = (_PyInterpreterFrame *)f->_f_frame_data; f->f_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT; - // This frame needs to be "complete", so act as if the first RESUME has run: - ((_PyInterpreterFrame *)f->_f_frame_data)->prev_instr = _PyCode_CODE(code); - assert(!_PyFrame_IsIncomplete((_PyInterpreterFrame *)f->_f_frame_data)); + // This frame needs to be "complete", so pretend that the first RESUME ran: + f->f_frame->prev_instr = _PyCode_CODE(code) + code->_co_firsttraceable; + assert(!_PyFrame_IsIncomplete(f->f_frame)); Py_DECREF(func); _PyObject_GC_TRACK(f); return f; @@ -1135,7 +1135,7 @@ _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { PyObject **fast; PyCodeObject *co; locals = frame->f_locals; - if (locals == NULL) { + if (locals == NULL) {Fix locals = frame->f_locals = PyDict_New(); if (locals == NULL) return -1; diff --git a/Python/frame.c b/Python/frame.c index 059435f9d299a4..96566de63a78fd 100644 --- a/Python/frame.c +++ b/Python/frame.c @@ -72,7 +72,7 @@ take_ownership(PyFrameObject *f, _PyInterpreterFrame *frame) frame->owner = FRAME_OWNED_BY_FRAME_OBJECT; if (_PyFrame_IsIncomplete(frame)) { // This may be a newly-created generator or coroutine frame. Since it's - // dead anyways, just pretend that the first RESUME has run: + // dead anyways, just pretend that the first RESUME ran: PyCodeObject *code = frame->f_code; frame->prev_instr = _PyCode_CODE(code) + code->_co_firsttraceable; } From ca40a03d3ff5b21e904b1364f28cf7b82dd32190 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Tue, 4 Oct 2022 01:58:40 -0700 Subject: [PATCH 7/9] Remove Fix ;) --- Objects/frameobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index ee92b8b271fc89..6a51a946ef35f0 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1135,7 +1135,7 @@ _PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame) { PyObject **fast; PyCodeObject *co; locals = frame->f_locals; - if (locals == NULL) {Fix + if (locals == NULL) { locals = frame->f_locals = PyDict_New(); if (locals == NULL) return -1; From c41a49cec6c55aa055cfffc9967b0a2488bfeb78 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Tue, 4 Oct 2022 02:00:24 -0700 Subject: [PATCH 8/9] blurb add --- .../2022-10-04-02-00-10.gh-issue-97779.f3N1hI.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-10-04-02-00-10.gh-issue-97779.f3N1hI.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-10-04-02-00-10.gh-issue-97779.f3N1hI.rst b/Misc/NEWS.d/next/Core and Builtins/2022-10-04-02-00-10.gh-issue-97779.f3N1hI.rst new file mode 100644 index 00000000000000..6115218088651b --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-10-04-02-00-10.gh-issue-97779.f3N1hI.rst @@ -0,0 +1 @@ +Ensure that all Python frame objects are backed by "complete" frames. From 289f6f2d70b60bd669c9b4d367896b7b11b8a150 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Tue, 4 Oct 2022 04:04:05 -0700 Subject: [PATCH 9/9] Feeback from code review --- Lib/test/test_code.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_code.py b/Lib/test/test_code.py index b64e7b788f9d20..4e4d82314a9fb8 100644 --- a/Lib/test/test_code.py +++ b/Lib/test/test_code.py @@ -690,6 +690,7 @@ def test_code_new_empty(self): # so that they both stay in sync. def f(): pass + PY_CODE_LOCATION_INFO_NO_COLUMNS = 13 f.__code__ = f.__code__.replace( co_firstlineno=42, co_code=bytes( @@ -702,7 +703,7 @@ def f(): co_linetable=bytes( [ (1 << 7) - | (13 << 3) + | (PY_CODE_LOCATION_INFO_NO_COLUMNS << 3) | (3 - 1), 0, ]