From e1fb03f3e2d1e99d0f745e9e17a963a036d34476 Mon Sep 17 00:00:00 2001 From: Damien George Date: Wed, 2 Jan 2019 17:48:43 +1100 Subject: [PATCH] py: Fix VM crash with unwinding jump out of a finally block. This patch fixes a bug in the VM when breaking within a try-finally. The bug has to do with executing a break within the finally block of a try-finally statement. For example: def f(): for x in (1,): print('a', x) try: raise Exception finally: print(1) break print('b', x) f() Currently in uPy the above code will print: a 1 1 1 segmentation fault (core dumped) micropython Not only is there a seg fault, but the "1" in the finally block is printed twice. This is because when the VM executes a finally block it doesn't really know if that block was executed due to a fall-through of the try (no exception raised), or because an exception is active. In particular, for nested finallys the VM has no idea which of the nested ones have active exceptions and which are just fall-throughs. So when a break (or continue) is executed it tries to unwind all of the finallys, when in fact only some may be active. It's questionable whether break (or return or continue) should be allowed within a finally block, because they implicitly swallow any active exception, but nevertheless it's allowed by CPython (although almost never used in the standard library). And uPy should at least not crash in such a case. The solution here relies on the fact that exception and finally handlers always appear in the bytecode after the try body. Note: there was a similar bug with a return in a finally block, but that was previously fixed in b735208403a54774f9fd3d966f7c1a194c41870f --- py/compile.c | 5 +- py/emitbc.c | 1 - py/vm.c | 10 ++-- tests/basics/try_finally_break.py | 99 +++++++++++++++++++++++++++++++ tests/basics/try_return.py | 9 +++ tests/cmdline/cmd_showbc.py.exp | 2 - 6 files changed, 114 insertions(+), 12 deletions(-) create mode 100644 tests/basics/try_finally_break.py diff --git a/py/compile.c b/py/compile.c index 42222528e2..ca01d74785 100644 --- a/py/compile.c +++ b/py/compile.c @@ -1599,7 +1599,6 @@ STATIC void compile_try_except(compiler_t *comp, mp_parse_node_t pn_body, int n_ } compile_node(comp, pns_except->nodes[1]); // the if (qstr_exception_local != 0) { - EMIT(pop_block); EMIT_ARG(load_const_tok, MP_TOKEN_KW_NONE); EMIT_ARG(label_assign, l3); EMIT_ARG(load_const_tok, MP_TOKEN_KW_NONE); @@ -1635,7 +1634,6 @@ STATIC void compile_try_finally(compiler_t *comp, mp_parse_node_t pn_body, int n } else { compile_try_except(comp, pn_body, n_except, pn_except, pn_else); } - EMIT(pop_block); EMIT_ARG(load_const_tok, MP_TOKEN_KW_NONE); EMIT_ARG(label_assign, l_finally_block); compile_node(comp, pn_finally); @@ -1811,8 +1809,7 @@ STATIC void compile_async_with_stmt_helper(compiler_t *comp, int n, mp_parse_nod compile_async_with_stmt_helper(comp, n - 1, nodes + 1, body); EMIT_ARG(adjust_stack_size, -3); - // Finish the "try" block - EMIT(pop_block); + // We have now finished the "try" block and fall through to the "finally" // At this point, after the with body has executed, we have 3 cases: // 1. no exception, we just fall through to this point; stack: (..., ctx_mgr) diff --git a/py/emitbc.c b/py/emitbc.c index 6a46cfb593..65d6509051 100644 --- a/py/emitbc.c +++ b/py/emitbc.c @@ -738,7 +738,6 @@ void mp_emit_bc_setup_block(emit_t *emit, mp_uint_t label, int kind) { } void mp_emit_bc_with_cleanup(emit_t *emit, mp_uint_t label) { - mp_emit_bc_pop_block(emit); mp_emit_bc_load_const_tok(emit, MP_TOKEN_KW_NONE); mp_emit_bc_label_assign(emit, label); emit_bc_pre(emit, 2); // ensure we have enough stack space to call the __exit__ method diff --git a/py/vm.c b/py/vm.c index a0ee2e89a4..56494dfa14 100644 --- a/py/vm.c +++ b/py/vm.c @@ -633,8 +633,6 @@ dispatch_loop: // replacing it with None, which signals END_FINALLY to just // execute the finally handler normally. SET_TOP(mp_const_none); - assert(exc_sp >= exc_stack); - POP_EXC_BLOCK(); } else { // We need to re-raise the exception. We pop __exit__ handler // by copying the exception instance down to the new top-of-stack. @@ -654,7 +652,7 @@ unwind_jump:; while ((unum & 0x7f) > 0) { unum -= 1; assert(exc_sp >= exc_stack); - if (MP_TAGPTR_TAG1(exc_sp->val_sp)) { + 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 @@ -698,6 +696,8 @@ unwind_jump:; // if TOS is an integer, finishes coroutine and returns control to caller // if TOS is an exception, reraises the exception 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 @@ -1063,7 +1063,7 @@ unwind_jump:; unwind_return: // Search for and execute finally handlers that aren't already active while (exc_sp >= exc_stack) { - if (!currently_in_except_block && MP_TAGPTR_TAG1(exc_sp->val_sp)) { + if (!currently_in_except_block && 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) @@ -1419,7 +1419,7 @@ unwind_loop: mp_obj_exception_add_traceback(MP_OBJ_FROM_PTR(nlr.ret_val), source_file, source_line, block_name); } - while (currently_in_except_block) { + while (currently_in_except_block || (exc_sp >= exc_stack && exc_sp->handler <= code_state->ip)) { // nested exception assert(exc_sp >= exc_stack); diff --git a/tests/basics/try_finally_break.py b/tests/basics/try_finally_break.py new file mode 100644 index 0000000000..ae7226637d --- /dev/null +++ b/tests/basics/try_finally_break.py @@ -0,0 +1,99 @@ +# test break within (nested) finally + +# basic case with break in finally +def f(): + for _ in range(2): + print(1) + try: + pass + finally: + print(2) + break + print(3) + print(4) + print(5) +f() + +# where the finally swallows an exception +def f(): + lst = [1, 2, 3] + for x in lst: + print('a', x) + try: + raise Exception + finally: + print(1) + break + print('b', x) +f() + +# basic nested finally with break in inner finally +def f(): + for i in range(2): + print('iter', i) + try: + raise TypeError + finally: + print(1) + try: + raise ValueError + finally: + break +print(f()) + +# similar to above but more nesting +def f(): + for i in range(2): + try: + raise ValueError + finally: + print(1) + try: + raise TypeError + finally: + print(2) + try: + pass + finally: + break +print(f()) + +# lots of nesting +def f(): + for i in range(2): + try: + raise ValueError + finally: + print(1) + try: + raise TypeError + finally: + print(2) + try: + raise Exception + finally: + break +print(f()) + +# basic case combined with try-else +def f(arg): + for _ in range(2): + print(1) + try: + if arg == 1: + raise ValueError + elif arg == 2: + raise TypeError + except ValueError: + print(2) + else: + print(3) + finally: + print(4) + break + print(5) + print(6) + print(7) +f(0) # no exception, else should execute +f(1) # exception caught, else should be skipped +f(2) # exception not caught, finally swallows exception, else should be skipped diff --git a/tests/basics/try_return.py b/tests/basics/try_return.py index 492c18d95c..a24290c4fb 100644 --- a/tests/basics/try_return.py +++ b/tests/basics/try_return.py @@ -1,5 +1,14 @@ # test use of return with try-except +def f(): + try: + print(1) + return + except: + print(2) + print(3) +f() + def f(l, i): try: return l[i] diff --git a/tests/cmdline/cmd_showbc.py.exp b/tests/cmdline/cmd_showbc.py.exp index 1274cda00f..d119a6198b 100644 --- a/tests/cmdline/cmd_showbc.py.exp +++ b/tests/cmdline/cmd_showbc.py.exp @@ -265,7 +265,6 @@ Raw bytecode (code_info_size=\\d\+, bytecode_size=\\d\+): \\d\+ POP_EXCEPT \\d\+ JUMP \\d\+ \\d\+ END_FINALLY -\\d\+ POP_BLOCK \\d\+ LOAD_CONST_NONE \\d\+ LOAD_FAST 1 \\d\+ POP_TOP @@ -286,7 +285,6 @@ Raw bytecode (code_info_size=\\d\+, bytecode_size=\\d\+): \\d\+ POP_TOP \\d\+ LOAD_DEREF 14 \\d\+ POP_TOP -\\d\+ POP_BLOCK \\d\+ LOAD_CONST_NONE \\d\+ WITH_CLEANUP \\d\+ END_FINALLY