gh-94215: Fix reference count issue in exception_unwind

[tiran]

Zero-cost exception handling did not take frame_setlineno() into
account. It can pop off stacks. This can lead to a crash.

Exception unwinding now re-calculates the stack pointer.

Co-authored-by: Irit Katriel [email protected]

• Issue: GC crash _PyObject_AssertFailed with pdb #94215

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @tiran for commit 990279b062ac4d61361ef7587e1977879606de52 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[tiran]

The fix seems to introduce a reference leak:

test_sys_settrace leaked [17, 17, 17] references, sum=51
test_sys_setprofile leaked [4, 4, 4] references, sum=12

Leaking tests:

• test.test_sys_settrace.JumpTestCase.test_no_jump_between_except_blocks
• test.test_sys_settrace.JumpTestCase.test_no_jump_between_except_blocks_2
• test.test_sys_settrace.JumpTestCase.test_no_jump_into_bare_except_block_from_try_block
• test.test_sys_settrace.JumpTestCase.test_no_jump_into_qualified_except_block_from_try_block
• test.test_sys_settrace.JumpTestCase.test_no_jump_out_of_bare_except_block
• test.test_sys_settrace.JumpTestCase.test_no_jump_out_of_qualified_except_block
• test.test_sys_settrace.JumpTestCase.test_no_jump_to_except_1
• test.test_sys_settrace.JumpTestCase.test_no_jump_to_except_2
• test.test_sys_settrace.JumpTestCase.test_no_jump_to_except_3
• test.test_sys_settrace.JumpTestCase.test_no_jump_to_except_4
• test.test_sys_settrace.JumpTestCase.test_no_jump_within_except_block
• test.test_sys_setprofile.ProfileHookTestCase.test_raise_reraise
• test.test_sys_setprofile.ProfileHookTestCase.test_raise_twice

./python -m test test_sys_setprofile --list-cases | while read line; do ./python -m test test_sys_setprofile -R:: -m $line || echo $line >> leaks.txt; done

[tiran]

@encukou How did you figure out the leaking tests so quickly? I had to figure out that shell one-liner to run each test function separately.

[encukou]

Umm, manually: deleting chunks of the test suite & only keeping the deletion if the leaks stayed at [17, 17, 17] ;)

[tiran]

My approach takes longer, but is less manual work.

[brandtbucher]

My approach takes longer, but is less manual work.

There's also ./python -m test.bisect_cmd -R:, which I use pretty frequently for these sorts of things. It will get it down to as few tests as possible (even just one, if it can).

[ericsnowcurrently]

LGTM

At the least this seems to be an effective short-term solution. It may be valid for the long-term too. I've left some thoughts on the issue.

[brandtbucher]

@tiran, applying this diff to your branch fixes the leaks for me:

diff --git a/Python/ceval.c b/Python/ceval.c
index 083f881807..e23bfda725 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -5683,6 +5683,8 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
                     err = maybe_call_line_trace(tstate->c_tracefunc,
                                                 tstate->c_traceobj,
                                                 tstate, frame, instr_prev);
+                    stack_pointer = _PyFrame_GetStackPointer(frame);
+                    frame->stacktop = -1;
                     if (err) {
                         /* trace function raised an exception */
                         next_instr++;
@@ -5690,9 +5692,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
                     }
                     /* Reload possibly changed frame fields */
                     next_instr = frame->prev_instr;
-
-                    stack_pointer = _PyFrame_GetStackPointer(frame);
-                    frame->stacktop = -1;
                 }
             }
         }
@@ -5795,11 +5794,6 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
 
                 /* Pop remaining stack entries. */
                 PyObject **stackbase = _PyFrame_Stackbase(frame);
-                if (frame->stacktop != -1) {
-                    // frame_setlineno() may have popped off additional stacks in
-                    // frame_stack_pop(). Re-calculate stack pointer.
-                    stack_pointer = _PyFrame_GetStackPointer(frame);
-                }
                 while (stack_pointer > stackbase) {
                     PyObject *o = POP();
                     Py_XDECREF(o);

I'm not entirely sure why yet, but my understanding (gained over the last couple of hours) is that frame->stacktop is expected to always be -1 in this function, except when we're calling a trace function (since GC of frame objects needs to work correctly there). This looks like one place where we forget to set it back to -1 and reload the (possibly changed) stack pointer if an error is raised in while tracing...

...maybe?

[tiran]

superseded by #94681