From 82c494a97e874912e7eb23d2f03f39212e343fb3 Mon Sep 17 00:00:00 2001 From: Damien George Date: Sat, 28 Sep 2019 00:07:21 +1000 Subject: [PATCH] py/vm: Fix handling of unwind jump out of active finally. Prior to this commit, when unwinding through an active finally the stack was not being correctly popped/folded, which resulting in the VM crashing for complicated unwinding of nested finallys. This should be fixed with this commit, and more tests for return/break/ continue within a finally have been added to exercise this. --- py/vm.c | 104 ++++++++++++++--------- tests/basics/try_finally_break2.py | 19 +++++ tests/basics/try_finally_continue.py | 17 ++++ tests/basics/try_finally_continue.py.exp | 9 ++ tests/basics/try_finally_return5.py | 17 ++++ 5 files changed, 127 insertions(+), 39 deletions(-) create mode 100644 tests/basics/try_finally_break2.py create mode 100644 tests/basics/try_finally_continue.py create mode 100644 tests/basics/try_finally_continue.py.exp create mode 100644 tests/basics/try_finally_return5.py diff --git a/py/vm.c b/py/vm.c index 7487ff61b4..6e5015fc43 100644 --- a/py/vm.c +++ b/py/vm.c @@ -109,6 +109,21 @@ exc_sp--; /* pop back to previous exception handler */ \ CLEAR_SYS_EXC_INFO() /* just clear sys.exc_info(), not compliant, but it shouldn't be used in 1st place */ +#define CANCEL_ACTIVE_FINALLY(sp) do { \ + if (mp_obj_is_small_int(sp[-1])) { \ + /* Stack: (..., prev_dest_ip, prev_cause, dest_ip) */ \ + /* Cancel the unwind through the previous finally, replace with current one */ \ + sp[-2] = sp[0]; \ + sp -= 2; \ + } else { \ + assert(sp[-1] == mp_const_none || mp_obj_is_exception_instance(sp[-1])); \ + /* Stack: (..., None/exception, dest_ip) */ \ + /* Silence the finally's exception value (may be None or an exception) */ \ + sp[-1] = sp[0]; \ + --sp; \ + } \ +} while (0) + #if MICROPY_PY_SYS_SETTRACE #define FRAME_SETUP() do { \ @@ -698,21 +713,28 @@ unwind_jump:; while ((unum & 0x7f) > 0) { unum -= 1; assert(exc_sp >= exc_stack); - if (MP_TAGPTR_TAG1(exc_sp->val_sp) && exc_sp->handler > ip) { - // Getting here the stack looks like: - // (..., X, dest_ip) - // where X is pointed to by exc_sp->val_sp and in the case - // of a "with" block contains the context manager info. - // We're going to run "finally" code as a coroutine - // (not calling it recursively). Set up a sentinel - // on the stack so it can return back to us when it is - // done (when WITH_CLEANUP or END_FINALLY reached). - // The sentinel is the number of exception handlers left to - // unwind, which is a non-negative integer. - PUSH(MP_OBJ_NEW_SMALL_INT(unum)); - ip = exc_sp->handler; // get exception handler byte code address - exc_sp--; // pop exception handler - goto dispatch_loop; // run the exception handler + + if (MP_TAGPTR_TAG1(exc_sp->val_sp)) { + if (exc_sp->handler > ip) { + // Found a finally handler that isn't active; run it. + // Getting here the stack looks like: + // (..., X, dest_ip) + // where X is pointed to by exc_sp->val_sp and in the case + // of a "with" block contains the context manager info. + assert(&sp[-1] == MP_TAGPTR_PTR(exc_sp->val_sp)); + // We're going to run "finally" code as a coroutine + // (not calling it recursively). Set up a sentinel + // on the stack so it can return back to us when it is + // done (when WITH_CLEANUP or END_FINALLY reached). + // The sentinel is the number of exception handlers left to + // unwind, which is a non-negative integer. + PUSH(MP_OBJ_NEW_SMALL_INT(unum)); + ip = exc_sp->handler; + goto dispatch_loop; + } else { + // Found a finally handler that is already active; cancel it. + CANCEL_ACTIVE_FINALLY(sp); + } } POP_EXC_BLOCK(); } @@ -740,9 +762,9 @@ unwind_jump:; // if TOS is None, just pops it and continues // if TOS is an integer, finishes coroutine and returns control to caller // if TOS is an exception, reraises the exception + assert(exc_sp >= exc_stack); + POP_EXC_BLOCK(); if (TOP() == mp_const_none) { - assert(exc_sp >= exc_stack); - POP_EXC_BLOCK(); sp--; } else if (mp_obj_is_small_int(TOP())) { // We finished "finally" coroutine and now dispatch back @@ -1113,28 +1135,32 @@ unwind_jump:; unwind_return: // Search for and execute finally handlers that aren't already active while (exc_sp >= exc_stack) { - if (MP_TAGPTR_TAG1(exc_sp->val_sp) && exc_sp->handler > ip) { - // Found a finally handler that isn't active. - // Getting here the stack looks like: - // (..., X, [iter0, iter1, ...,] ret_val) - // where X is pointed to by exc_sp->val_sp and in the case - // of a "with" block contains the context manager info. - // There may be 0 or more for-iterators between X and the - // return value, and these must be removed before control can - // pass to the finally code. We simply copy the ret_value down - // over these iterators, if they exist. If they don't then the - // following is a null operation. - mp_obj_t *finally_sp = MP_TAGPTR_PTR(exc_sp->val_sp); - finally_sp[1] = sp[0]; - sp = &finally_sp[1]; - // We're going to run "finally" code as a coroutine - // (not calling it recursively). Set up a sentinel - // on a stack so it can return back to us when it is - // done (when WITH_CLEANUP or END_FINALLY reached). - PUSH(MP_OBJ_NEW_SMALL_INT(-1)); - ip = exc_sp->handler; - POP_EXC_BLOCK(); - goto dispatch_loop; + if (MP_TAGPTR_TAG1(exc_sp->val_sp)) { + if (exc_sp->handler > ip) { + // Found a finally handler that isn't active; run it. + // Getting here the stack looks like: + // (..., X, [iter0, iter1, ...,] ret_val) + // where X is pointed to by exc_sp->val_sp and in the case + // of a "with" block contains the context manager info. + // There may be 0 or more for-iterators between X and the + // return value, and these must be removed before control can + // pass to the finally code. We simply copy the ret_value down + // over these iterators, if they exist. If they don't then the + // following is a null operation. + mp_obj_t *finally_sp = MP_TAGPTR_PTR(exc_sp->val_sp); + finally_sp[1] = sp[0]; + sp = &finally_sp[1]; + // We're going to run "finally" code as a coroutine + // (not calling it recursively). Set up a sentinel + // on a stack so it can return back to us when it is + // done (when WITH_CLEANUP or END_FINALLY reached). + PUSH(MP_OBJ_NEW_SMALL_INT(-1)); + ip = exc_sp->handler; + goto dispatch_loop; + } else { + // Found a finally handler that is already active; cancel it. + CANCEL_ACTIVE_FINALLY(sp); + } } POP_EXC_BLOCK(); } diff --git a/tests/basics/try_finally_break2.py b/tests/basics/try_finally_break2.py new file mode 100644 index 0000000000..086e92e902 --- /dev/null +++ b/tests/basics/try_finally_break2.py @@ -0,0 +1,19 @@ +def foo(x): + for i in range(x): + for j in range(x): + try: + print(x, i, j, 1) + finally: + try: + try: + print(x, i, j, 2) + finally: + try: + 1 / 0 + finally: + print(x, i, j, 3) + break + finally: + print(x, i, j, 4) + break +print(foo(4)) diff --git a/tests/basics/try_finally_continue.py b/tests/basics/try_finally_continue.py new file mode 100644 index 0000000000..50040e5de0 --- /dev/null +++ b/tests/basics/try_finally_continue.py @@ -0,0 +1,17 @@ +def foo(x): + for i in range(x): + try: + pass + finally: + try: + try: + print(x, i) + finally: + try: + 1 / 0 + finally: + return 42 + finally: + print('continue') + continue +print(foo(4)) diff --git a/tests/basics/try_finally_continue.py.exp b/tests/basics/try_finally_continue.py.exp new file mode 100644 index 0000000000..2901997b1a --- /dev/null +++ b/tests/basics/try_finally_continue.py.exp @@ -0,0 +1,9 @@ +4 0 +continue +4 1 +continue +4 2 +continue +4 3 +continue +None diff --git a/tests/basics/try_finally_return5.py b/tests/basics/try_finally_return5.py new file mode 100644 index 0000000000..aa2327e655 --- /dev/null +++ b/tests/basics/try_finally_return5.py @@ -0,0 +1,17 @@ +def foo(x): + for i in range(x): + try: + pass + finally: + try: + try: + print(x, i) + finally: + try: + 1 / 0 + finally: + return 42 + finally: + print('return') + return 43 +print(foo(4))