continue and break in finally with return in try results with segfault

[isidentical]

BPO	37830
Nosy	@ncoghlan, @vstinner, @ericvsmith, @ethanfurman, @ambv, @markshannon, @serhiy-storchaka, @pppery, @pablogsal, @tirkarthi, @isidentical
PRs	bpo-37830: handle specific cases on FOR_ITER #15221 bpo-37830: Revert "bpo-32489: Allow 'continue' in 'finally' clause. (GH-5822)" #15230 bpo-37830: Optimize return statement bytecode with fixing segfaults #15247 bpo-37830: Fix compilation of break and continue in finally. #15320 [3.8] bpo-37830: Fix compilation of break and continue in finally. (GH-15320) #15456

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2019-08-24.10:57:26.563>
created_at = <Date 2019-08-12.09:33:33.151>
labels = ['interpreter-core', '3.8', '3.9', 'type-crash', 'release-blocker']
title = 'continue and break in finally with return in try results with segfault'
updated_at = <Date 2019-08-24.10:57:26.563>
user = 'https://github.com/isidentical'

bugs.python.org fields:

activity = <Date 2019-08-24.10:57:26.563>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2019-08-24.10:57:26.563>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2019-08-12.09:33:33.151>
creator = 'BTaskaya'
dependencies = []
files = []
hgrepos = []
issue_num = 37830
keywords = ['patch', '3.8regression']
message_count = 20.0
messages = ['349450', '349451', '349456', '349458', '349460', '349462', '349483', '349513', '349528', '349570', '349578', '349579', '349605', '349693', '349901', '350289', '350300', '350359', '350364', '350365']
nosy_count = 11.0
nosy_names = ['ncoghlan', 'vstinner', 'eric.smith', 'ethan.furman', 'lukasz.langa', 'Mark.Shannon', 'serhiy.storchaka', 'ppperry', 'pablogsal', 'xtreak', 'BTaskaya']
pr_nums = ['15221', '15230', '15247', '15320', '15456']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue37830'
versions = ['Python 3.8', 'Python 3.9']

[isidentical]

When you try to put a return statement with the iterated value inside of try/finally and if you have continue in finally it will result with segfault.

def simple():
    for number in range(2):
        try:
            return number
        finally:
            continue

simple()
SEGV

My first debugs shows TOS is the next value int(1) when it comes to FOR_ITER, instead of the range instance.
So when python try to call next() with (*iter->ob_type->tp_iternext)(iter) in FOR_ITER it gets a non iterator, int(1).

Adding an assert can prove it,
python: Python/ceval.c:3198: _PyEval_EvalFrameDefault: Assertion `PyIter_Check(iter)' failed.

For seeing how stack changed i enabled lltrace and formatted it little bit;

>>> STACK_SIZE          0                       
LOAD_GLOBAL             0                       
>>> STACK_SIZE          0                       
>>> push                <class 'range'>         
LOAD_CONST              1                       
>>> STACK_SIZE          1                       
>>> push                2                       
CALL_FUNCTION           1                       
>>> STACK_SIZE          2                       
>>> ext_pop             2                       
>>> ext_pop             <class 'range'>         
>>> push                range(0, 2)             
GET_ITER                None                    
>>> STACK_SIZE          1                       
FOR_ITER                24                      
>>> STACK_SIZE          1                       
>>> push                0                       
STORE_FAST              0                       
>>> STACK_SIZE          2                       
>>> pop                 0                       
SETUP_FINALLY           12                      
>>> STACK_SIZE          1                       
LOAD_FAST               0                       
>>> STACK_SIZE          1                       
>>> push                0                       
POP_BLOCK               None                    
>>> STACK_SIZE          2                       
CALL_FINALLY            6                       
>>> STACK_SIZE          2                       
>>> push                20                      
POP_FINALLY             0                       
>>> STACK_SIZE          3                       
>>> pop                 20                      
JUMP_ABSOLUTE           8                       
>>> STACK_SIZE          2                       
FOR_ITER                24                      
>>> STACK_SIZE          2
[SEGV]

And the oddity is STACK_SIZE should be 1 before the FOR_ITER but it is 2, then i said why dont i try the SECOND() as iter, and it worked. It means an instruction is pushing the value of previous iteration.

There are 3 things we can do;
=> raise a RuntimeError if TOS isn't an iterator (IMHO we should do that)
=> check if try/finally created inside of a function and an iterator. then check inside of try if a return happens with the iterated value and if so set preserve_tos value to false.
=> dont allow continue in finally

I want to fix this, and i prefer the first one. If you have any suggestion, i am open.

[tirkarthi]

Adding Serhiy since this change was introduced with bpo-32489.

[serhiy-storchaka]

Thank you for your report Batuhan.

But PR 15221 is not proper way to fix it. It will not work when return an iterable. This issue should be fixed at compiler level. The generated code is:

Disassembly of <code object simple at 0x7f2195c7d450, file "<stdin>", line 1>:
2 0 LOAD_GLOBAL 0 (range)
2 LOAD_CONST 1 (2)
4 CALL_FUNCTION 1
6 GET_ITER
>> 8 FOR_ITER 24 (to 34)
10 STORE_FAST 0 (number)

3 12 SETUP_FINALLY 12 (to 26)

4 14 LOAD_FAST 0 (number)
16 POP_BLOCK
18 CALL_FINALLY 6 (to 26)
20 ROT_TWO
22 POP_TOP
24 RETURN_VALUE

6 >> 26 POP_FINALLY 0
28 JUMP_ABSOLUTE 8
30 END_FINALLY
32 JUMP_ABSOLUTE 8
>> 34 LOAD_CONST 0 (None)
36 RETURN_VALUE

The return statement pushes a value at the stack (offset 14) and the continue statement jumps to the beginning of the loop (offset 28). The stack is not balanced here.

[serhiy-storchaka]

It looks to me, that it is not possible to solve this with the current bytecode. Perhaps the only way to fix a crash is to revert bpo-32489.

Implementing Mark's idea about inlining the code of a "finally" block may help to solve the issue with "continue" in "finally".

[isidentical]

Raising an error can handle this because it is impossible to reach to return so we can declare this as an illegal syntax?

[vstinner]

It looks to me, that it is not possible to solve this with the current bytecode. Perhaps the only way to fix a crash is to revert bpo-32489.

That sounds like a good compromise to unblock next Python 3.8 release. bpo-32489 can wait another release (Python 3.9), no?

[isidentical]

I closed the my PR in favor of Serhiy's PR.

On Mon, Aug 12, 2019, 9:18 PM Serhiy Storchaka <report@bugs.python.org>
wrote:

Change by Serhiy Storchaka <storchaka+cpython@gmail.com>:

----------
pull_requests: +14953
pull_request: #15230

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue37830\>

---

[pppery]

Unfortunately, there's a similar bug for break in a finally inside two nested loops, so just re-banning continue won't fix the crash.
The code below segfaults:

def simple():
    for number in range(2):
    	for number in range(2):
        	try:
            		return number
        	finally:
            		break
simple()

[isidentical]

serhiy can you review the solution i found?