classification
Title: continue and break in finally with return in try results with segfault
Type: crash Stage: patch review
Components: Interpreter Core Versions: Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: BTaskaya, Mark.Shannon, eric.smith, ethan.furman, lukasz.langa, ncoghlan, pablogsal, ppperry, serhiy.storchaka, vstinner, xtreak
Priority: release blocker Keywords: 3.8regression, patch

Created on 2019-08-12 09:33 by BTaskaya, last changed 2019-08-17 16:35 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 15221 closed BTaskaya, 2019-08-12 09:36
PR 15230 open serhiy.storchaka, 2019-08-12 18:18
PR 15247 closed BTaskaya, 2019-08-13 06:51
PR 15320 open serhiy.storchaka, 2019-08-17 16:13
Messages (15)
msg349450 - (view) Author: Batuhan (BTaskaya) * Date: 2019-08-12 09:33
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.
msg349451 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-08-12 10:34
Adding Serhiy since this change was introduced with issue32489.
msg349456 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-12 11:32
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.
msg349458 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-12 11:56
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 issue32489.

Implementing Mark's idea about inlining the code of a "finally" block may help to solve the issue with "continue" in "finally".
msg349460 - (view) Author: Batuhan (BTaskaya) * Date: 2019-08-12 12:01
Raising an error can handle this because it is impossible to reach to return so we can declare this as an illegal syntax?
msg349462 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-08-12 12:35
> 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 issue32489.

That sounds like a good compromise to unblock next Python 3.8 release. issue32489 can wait another release (Python 3.9), no?
msg349483 - (view) Author: Batuhan (BTaskaya) * Date: 2019-08-12 18:26
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: https://github.com/python/cpython/pull/15230
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue37830>
> _______________________________________
>
msg349513 - (view) Author: (ppperry) Date: 2019-08-13 01:22
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()
```
msg349528 - (view) Author: Batuhan (BTaskaya) * Date: 2019-08-13 06:52
serhiy can you review the solution i found?
msg349570 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2019-08-13 15:24
My apologies if I missed something, but do we have a consensus on the desired solution?

My understanding of `try/finally` is that whatever happens in the `finally` clause should:

- always happen
- win any conflicts with `try` clause

For example:

  try:
     a = 2
  finally:
     a = 3
  print(a)
  # 3

and

  def f():
     try:
        return 5
     finally:
        return 7
  print(f())
  # 7

So it seems like the ideal solution to:

  def mult(thing):
      print(thing*2)
      return thing * 2

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

would be:

  0
  2
  None
msg349578 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-13 16:09
Batuhan, unfortunately your second solution does not work too, because continue and break can be conditional. For example:

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

print(simple(0))
print(simple(1))

It should print:

0
None


Ethan, your understanding is correct.
msg349579 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-13 16:18
Note, that we have a regression in 3.8. There is a use case for "break" in "finally", and such code is even used in the stdlib. And who know in what third-party code it is used. In specific circumstances (see msg349513) it now can cause a crash. Other example:

import contextlib
def simple():
    with contextlib.nullcontext():
        for number in range(2):
            try:
                return number
            finally:
                break

simple()

It just raise an exception in 3.8, not crash:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 7, in simple
TypeError: 'range_iterator' object is not callable
msg349605 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-13 19:03
There may be a solution that does not require a significant change to the code generator, but small changes in many places with keeping the general structure. But it is a difficult task, so it takes some time. Don't worry, I'm working on it.
msg349693 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-08-14 13:02
Even though it doesn't fully resolve this crash, I'd still like us to consider reverting the change to allow "continue" in try/finally blocks.

It doesn't seem to have a compelling practical motivation behind it (beyond the fact that it's nice not to impose arbitrary restriction on users), and even if it's feasible in CPython now, it still creates a non-trivial amount of work for other Python implementations that are trying to remain consistent with what CPython allows.
msg349901 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-17 16:35
PR 15320 fixes a regression by changing the compiler. None is now always pushed on the stack before entering a try...finally block. The "return" statement with a non-constant value replaces it, so the stack is balanced.
History
Date User Action Args
2019-08-17 16:35:48serhiy.storchakasetmessages: + msg349901
2019-08-17 16:13:28serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request15039
2019-08-14 13:02:18ncoghlansetnosy: + ncoghlan
messages: + msg349693
2019-08-13 19:03:12serhiy.storchakasetmessages: + msg349605
2019-08-13 16:18:41serhiy.storchakasettitle: continue in finally with return in try results with segfault -> continue and break in finally with return in try results with segfault
messages: + msg349579
stage: patch review -> (no value)
2019-08-13 16:09:46serhiy.storchakasetkeywords: + 3.8regression, - patch

messages: + msg349578
2019-08-13 15:24:08ethan.furmansetnosy: + ethan.furman
messages: + msg349570
2019-08-13 06:52:51BTaskayasetmessages: + msg349528
2019-08-13 06:51:42BTaskayasetpull_requests: + pull_request14968
2019-08-13 01:22:41ppperrysetnosy: + ppperry
messages: + msg349513
2019-08-12 18:26:40BTaskayasetmessages: + msg349483
2019-08-12 18:18:36serhiy.storchakasetpull_requests: + pull_request14953
2019-08-12 12:35:11vstinnersetmessages: + msg349462
2019-08-12 12:25:51pablogsalsetnosy: + pablogsal
2019-08-12 12:01:26BTaskayasetmessages: + msg349460
2019-08-12 11:56:22serhiy.storchakasetmessages: + msg349458
2019-08-12 11:44:39vstinnersetnosy: + vstinner
2019-08-12 11:32:54serhiy.storchakasetpriority: normal -> release blocker
nosy: + lukasz.langa
2019-08-12 11:32:42serhiy.storchakasetassignee: serhiy.storchaka

messages: + msg349456
nosy: + Mark.Shannon
2019-08-12 10:34:52xtreaksetnosy: + serhiy.storchaka, xtreak
messages: + msg349451
2019-08-12 10:09:45BTaskayasetversions: + Python 3.8, Python 3.9
2019-08-12 09:57:51eric.smithsetnosy: + eric.smith
2019-08-12 09:57:35eric.smithsettype: crash
components: + Interpreter Core
2019-08-12 09:36:56BTaskayasetkeywords: + patch
stage: patch review
pull_requests: + pull_request14948
2019-08-12 09:33:33BTaskayacreate