Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
isidentical opened this issue Aug 12, 2019 · 20 comments
Closed
Assignees
Labels
3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@isidentical
Copy link
Sponsor Member

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
    Copy link
    Sponsor Member Author

    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.

    @ericvsmith ericvsmith added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Aug 12, 2019
    @isidentical isidentical added 3.8 only security fixes 3.9 only security fixes labels Aug 12, 2019
    @tirkarthi
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Sponsor Member Author

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

    @vstinner
    Copy link
    Member

    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
    Copy link
    Sponsor Member Author

    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
    Copy link
    Mannequin

    pppery mannequin commented Aug 13, 2019

    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
    Copy link
    Sponsor Member Author

    serhiy can you review the solution i found?

    @ethanfurman
    Copy link
    Member

    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

    @serhiy-storchaka
    Copy link
    Member

    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.

    @serhiy-storchaka
    Copy link
    Member

    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

    @serhiy-storchaka serhiy-storchaka changed the title continue in finally with return in try results with segfault continue and break in finally with return in try results with segfault Aug 13, 2019
    @serhiy-storchaka
    Copy link
    Member

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 23, 2019

    This is marked as a release blocker. The last beta is scheduled for Monday. Please decide how to proceed ASAP.

    @pablogsal
    Copy link
    Member

    I think PR 15320 should be merge as soon as possible at least to fix the segfault and stop the release blocker. Anything else can wait IMHO

    @serhiy-storchaka
    Copy link
    Member

    New changeset ef61c52 by Serhiy Storchaka in branch 'master':
    bpo-37830: Fix compilation of break and continue in finally. (GH-15320)
    ef61c52

    @serhiy-storchaka
    Copy link
    Member

    New changeset ed146b5 by Serhiy Storchaka in branch '3.8':
    [3.8] bpo-37830: Fix compilation of break and continue in finally. (GH-15320) (GH-15456)
    ed146b5

    @serhiy-storchaka
    Copy link
    Member

    For forbidding 'break', 'continue' and 'return' in the 'finally' clause please open a topic on Python-Dev.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    9 participants