From 5010796d514382e420dfce8c4f4efcc8142313f6 Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Thu, 11 Jan 2024 14:13:09 +0100 Subject: [PATCH 01/13] Frame clear, clear locals f_locals might still contain references to the local vars. Fix #113939. --- Objects/frameobject.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index cafe4ef6141d9a..a914c61aac2fd5 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -926,6 +926,7 @@ frame_tp_clear(PyFrameObject *f) Py_CLEAR(locals[i]); } f->f_frame->stacktop = 0; + Py_CLEAR(f->f_frame->f_locals); return 0; } From aa78bdc9383a562bb5e0def52968baec065c702e Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 16:24:53 +0100 Subject: [PATCH 02/13] DELETE_FAST: clear f_locals --- Python/bytecodes.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index f53ddae8df985a..7d6c0b41e3397f 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1508,6 +1508,7 @@ dummy_func( PyObject *v = GETLOCAL(oparg); ERROR_IF(v == NULL, unbound_local_error); SETLOCAL(oparg, NULL); + Py_CLEAR(LOCALS()); } inst(MAKE_CELL, (--)) { From 8cc7537718e52434be33c929b5c93ec4d4e151a3 Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 16:27:10 +0100 Subject: [PATCH 03/13] make regen-cases --- Include/internal/pycore_opcode_metadata.h | 2 +- Include/internal/pycore_uop_metadata.h | 2 +- Python/executor_cases.c.h | 1 + Python/generated_cases.c.h | 1 + 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index a9d698da25a1db..2eca92a4bd7289 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1000,7 +1000,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[268] = { [COPY_FREE_VARS] = { true, INSTR_FMT_IB, HAS_ARG_FLAG }, [DELETE_ATTR] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_DEREF] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, - [DELETE_FAST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG }, + [DELETE_FAST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_GLOBAL] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_NAME] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_SUBSCR] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, diff --git a/Include/internal/pycore_uop_metadata.h b/Include/internal/pycore_uop_metadata.h index ab498e9cefde22..07c7648694a60b 100644 --- a/Include/internal/pycore_uop_metadata.h +++ b/Include/internal/pycore_uop_metadata.h @@ -90,7 +90,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = { [_GUARD_BUILTINS_VERSION] = HAS_DEOPT_FLAG, [_LOAD_GLOBAL_MODULE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, [_LOAD_GLOBAL_BUILTINS] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, - [_DELETE_FAST] = HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG, + [_DELETE_FAST] = HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_MAKE_CELL] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_DELETE_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_LOAD_FROM_DICT_OR_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index ea4caa9a97ab39..0e34f80b6afd55 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1190,6 +1190,7 @@ PyObject *v = GETLOCAL(oparg); if (v == NULL) goto unbound_local_error_tier_two; SETLOCAL(oparg, NULL); + Py_CLEAR(LOCALS()); break; } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index e693e3e2560e7b..e1b42cba4affa1 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2210,6 +2210,7 @@ PyObject *v = GETLOCAL(oparg); if (v == NULL) goto unbound_local_error; SETLOCAL(oparg, NULL); + Py_CLEAR(LOCALS()); DISPATCH(); } From fee8ad6bf6ca1c7fdd9ba65b7867ae74f3435f5d Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 16:40:15 +0100 Subject: [PATCH 04/13] add news entry --- .../2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst b/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst new file mode 100644 index 00000000000000..5068f7173145b2 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst @@ -0,0 +1,7 @@ +frame.clear(), clear locals, not just fast locals. This is relevant once +frame.f_locals was accessed, which would contain also references to all the +locals. + +Also, if frame.f_locals was accessed of the current frame, e.g. like +exception handling code might do, for any DELETE_FAST, reset frame.f_locals, +such that objects can be freed, e.g. the exception. From 01b277414cca5b4b7acd7786ae85cf9fdc914957 Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 16:59:29 +0100 Subject: [PATCH 05/13] test for locals, exception going out of scope --- Lib/test/test_frame.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 7f17666a8d9697..e744a3463ecf94 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -442,5 +442,27 @@ def dummy(): # The following line should not cause a segmentation fault. self.assertIsNone(frame.f_back) +class LocalsTest(unittest.TestCase): + """ + Tests for locals. + """ + + def test_locals_cur_frame_issue113939(self): + class C: + pass + wr = None + def inner(): + nonlocal wr + c = C() + wr = weakref.ref(c) + 1/0 + try: + inner() + except ZeroDivisionError as exc: + self.assertIsNotNone(wr()) + print(exc.__traceback__.tb_frame.f_locals) + support.gc_collect() + self.assertIsNone(wr()) + if __name__ == "__main__": unittest.main() From b384b6e2158c6015dc797ec41a1ff91450f88e8d Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 17:03:09 +0100 Subject: [PATCH 06/13] test for frame.clear --- Lib/test/test_frame.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index e744a3463ecf94..b02d121568ac74 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -55,6 +55,24 @@ class C: # The reference was released by .clear() self.assertIs(None, wr()) + def test_clear_locals_cur_frame_issue113939(self): + class C: + pass + wr = None + def inner(): + nonlocal wr + c = C() + wr = weakref.ref(c) + 1/0 + try: + inner() + except ZeroDivisionError as exc: + self.assertIsNotNone(wr()) + print(exc.__traceback__.tb_next.tb_frame.f_locals) + exc.__traceback__.tb_next.tb_frame.clear() + support.gc_collect() + self.assertIsNone(wr()) + def test_clear_does_not_clear_specials(self): class C: pass From c908815248bc5de90a2c512c9491cd82fac71002 Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 22:15:34 +0100 Subject: [PATCH 07/13] test_clear_locals_issue113939 better --- Lib/test/test_frame.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index b02d121568ac74..0aa46240305b7d 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -55,7 +55,7 @@ class C: # The reference was released by .clear() self.assertIs(None, wr()) - def test_clear_locals_cur_frame_issue113939(self): + def test_clear_locals_issue113939(self): class C: pass wr = None @@ -67,6 +67,7 @@ def inner(): try: inner() except ZeroDivisionError as exc: + support.gc_collect() self.assertIsNotNone(wr()) print(exc.__traceback__.tb_next.tb_frame.f_locals) exc.__traceback__.tb_next.tb_frame.clear() From ecde7e514782bf6e6348d07d31a2f40acb219f30 Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Fri, 12 Jan 2024 22:16:14 +0100 Subject: [PATCH 08/13] test_locals_cur_frame_issue113939 better --- Lib/test/test_frame.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 0aa46240305b7d..7f360ac5769f98 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -478,6 +478,7 @@ def inner(): try: inner() except ZeroDivisionError as exc: + support.gc_collect() self.assertIsNotNone(wr()) print(exc.__traceback__.tb_frame.f_locals) support.gc_collect() From 7b068e49c256273f8c2eb7f4589c5a3f83250e5e Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Mon, 15 Jan 2024 16:16:16 +0100 Subject: [PATCH 09/13] rename test_locals_cleared_after_exception_handled Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> --- Lib/test/test_frame.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 7f360ac5769f98..6fc2098e9abda7 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -466,7 +466,8 @@ class LocalsTest(unittest.TestCase): Tests for locals. """ - def test_locals_cur_frame_issue113939(self): + def test_locals_cleared_after_exception_handled(self): + # see gh-113939 class C: pass wr = None From 7a2fa8e136835b59bae623ff709d86bf2b24e35f Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Mon, 15 Jan 2024 16:19:04 +0100 Subject: [PATCH 10/13] rename test_clear_locals_after_f_locals_access --- Lib/test/test_frame.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 6fc2098e9abda7..5318ec061ecee2 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -55,7 +55,8 @@ class C: # The reference was released by .clear() self.assertIs(None, wr()) - def test_clear_locals_issue113939(self): + def test_clear_locals_after_f_locals_access(self): + # see gh-113939 class C: pass wr = None From fae8b78a7afbcc24e8e3e677b5cf0cf70ed3e7fa Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Mon, 15 Jan 2024 16:25:33 +0100 Subject: [PATCH 11/13] reformulate news entry --- ...024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst b/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst index 5068f7173145b2..b70565958a7c39 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst @@ -1,7 +1,11 @@ -frame.clear(), clear locals, not just fast locals. This is relevant once -frame.f_locals was accessed, which would contain also references to all the -locals. +frame.clear(): +Clear frame.f_locals as well, and not only the fast locals. +This is relevant once frame.f_locals was accessed, +which would contain also references to all the locals. -Also, if frame.f_locals was accessed of the current frame, e.g. like -exception handling code might do, for any DELETE_FAST, reset frame.f_locals, -such that objects can be freed, e.g. the exception. +Opcode DELETE_FAST: +Also clear frame.f_locals, and not only the fast local. +If frame.f_locals was accessed of the current frame, +e.g. like exception handling code might do, +frame.f_locals still holds references to the locals, +even though you would expect that they are freed. From d701ebb87944f9ddb728403a859510770748733b Mon Sep 17 00:00:00 2001 From: Albert Zeyer Date: Sat, 27 Jan 2024 23:58:48 +0100 Subject: [PATCH 12/13] revert DELETE_FAST change --- Include/internal/pycore_opcode_metadata.h | 2 +- Include/internal/pycore_uop_metadata.h | 2 +- Lib/test/test_frame.py | 24 ------------------- ...-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst | 7 ------ Python/bytecodes.c | 1 - Python/executor_cases.c.h | 1 - Python/generated_cases.c.h | 1 - 7 files changed, 2 insertions(+), 36 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 2eca92a4bd7289..a9d698da25a1db 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1000,7 +1000,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[268] = { [COPY_FREE_VARS] = { true, INSTR_FMT_IB, HAS_ARG_FLAG }, [DELETE_ATTR] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_DEREF] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, - [DELETE_FAST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, + [DELETE_FAST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG }, [DELETE_GLOBAL] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_NAME] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [DELETE_SUBSCR] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, diff --git a/Include/internal/pycore_uop_metadata.h b/Include/internal/pycore_uop_metadata.h index 07c7648694a60b..ab498e9cefde22 100644 --- a/Include/internal/pycore_uop_metadata.h +++ b/Include/internal/pycore_uop_metadata.h @@ -90,7 +90,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = { [_GUARD_BUILTINS_VERSION] = HAS_DEOPT_FLAG, [_LOAD_GLOBAL_MODULE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, [_LOAD_GLOBAL_BUILTINS] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, - [_DELETE_FAST] = HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, + [_DELETE_FAST] = HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG, [_MAKE_CELL] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_DELETE_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_LOAD_FROM_DICT_OR_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 5318ec061ecee2..75418cf6135384 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -462,29 +462,5 @@ def dummy(): # The following line should not cause a segmentation fault. self.assertIsNone(frame.f_back) -class LocalsTest(unittest.TestCase): - """ - Tests for locals. - """ - - def test_locals_cleared_after_exception_handled(self): - # see gh-113939 - class C: - pass - wr = None - def inner(): - nonlocal wr - c = C() - wr = weakref.ref(c) - 1/0 - try: - inner() - except ZeroDivisionError as exc: - support.gc_collect() - self.assertIsNotNone(wr()) - print(exc.__traceback__.tb_frame.f_locals) - support.gc_collect() - self.assertIsNone(wr()) - if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst b/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst index b70565958a7c39..28b8e4bdda6be4 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2024-01-12-16-40-07.gh-issue-113939.Yi3L-e.rst @@ -2,10 +2,3 @@ frame.clear(): Clear frame.f_locals as well, and not only the fast locals. This is relevant once frame.f_locals was accessed, which would contain also references to all the locals. - -Opcode DELETE_FAST: -Also clear frame.f_locals, and not only the fast local. -If frame.f_locals was accessed of the current frame, -e.g. like exception handling code might do, -frame.f_locals still holds references to the locals, -even though you would expect that they are freed. diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 7d6c0b41e3397f..f53ddae8df985a 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1508,7 +1508,6 @@ dummy_func( PyObject *v = GETLOCAL(oparg); ERROR_IF(v == NULL, unbound_local_error); SETLOCAL(oparg, NULL); - Py_CLEAR(LOCALS()); } inst(MAKE_CELL, (--)) { diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 0e34f80b6afd55..ea4caa9a97ab39 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1190,7 +1190,6 @@ PyObject *v = GETLOCAL(oparg); if (v == NULL) goto unbound_local_error_tier_two; SETLOCAL(oparg, NULL); - Py_CLEAR(LOCALS()); break; } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index e1b42cba4affa1..e693e3e2560e7b 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -2210,7 +2210,6 @@ PyObject *v = GETLOCAL(oparg); if (v == NULL) goto unbound_local_error; SETLOCAL(oparg, NULL); - Py_CLEAR(LOCALS()); DISPATCH(); } From 227c495a2158e9d49c7e153693284875713cc666 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Wed, 31 Jan 2024 18:11:03 +0000 Subject: [PATCH 13/13] whitespace --- Lib/test/test_frame.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_frame.py b/Lib/test/test_frame.py index 75418cf6135384..244ce8af7cdf08 100644 --- a/Lib/test/test_frame.py +++ b/Lib/test/test_frame.py @@ -59,12 +59,14 @@ def test_clear_locals_after_f_locals_access(self): # see gh-113939 class C: pass + wr = None def inner(): nonlocal wr c = C() wr = weakref.ref(c) 1/0 + try: inner() except ZeroDivisionError as exc: