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

Resuming a 'yield from' stack is broken if a signal arrives in the middle #74225

Closed
njsmith opened this issue Apr 11, 2017 · 8 comments
Closed
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@njsmith
Copy link
Contributor

njsmith commented Apr 11, 2017

BPO 30039
Nosy @vstinner, @njsmith, @1st1
PRs
  • bpo-30039: Don't run signal handlers while resuming a yield from stack #1081
  • [3.6] bpo-30039: Don't run signal handlers while resuming a yield fro… #1640
  • 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/1st1'
    closed_at = <Date 2017-06-09.21:07:32.657>
    created_at = <Date 2017-04-11.09:33:27.080>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = "Resuming a 'yield from' stack is broken if a signal arrives in the middle"
    updated_at = <Date 2017-06-10.08:52:13.435>
    user = 'https://github.com/njsmith'

    bugs.python.org fields:

    activity = <Date 2017-06-10.08:52:13.435>
    actor = 'vstinner'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2017-06-09.21:07:32.657>
    closer = 'yselivanov'
    components = ['Interpreter Core']
    creation = <Date 2017-04-11.09:33:27.080>
    creator = 'njs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30039
    keywords = []
    message_count = 8.0
    messages = ['291469', '293887', '293902', '293903', '295578', '295579', '295580', '295620']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'njs', 'yselivanov']
    pr_nums = ['1081', '1640']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30039'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Apr 11, 2017

    If we have a chain of generators/coroutines that are 'yield from'ing each other, then resuming the stack works like:

    • call send() on the outermost generator
    • this enters _PyEval_EvalFrameDefault, which re-executes the YIELD_FROM opcode
    • which calls send() on the next generator
    • which enters _PyEval_EvalFrameDefault, which re-executes the YIELD_FROM opcode
    • ...etc.

    However, every time we enter _PyEval_EvalFrameDefault, the first thing we do is to check for pending signals, and if there are any then we run the signal handler. And if it raises an exception, then we immediately propagate that exception *instead* of starting to execute bytecode. This means that e.g. a SIGINT at the wrong moment can "break the chain" – it can be raised in the middle of our yield from chain, with the bottom part of the stack abandoned for the garbage collector.

    The fix is pretty simple: there's already a special case in _PyEval_EvalFrameEx where it skips running signal handlers if the next opcode is SETUP_FINALLY. (I don't see how this accomplishes anything useful, but that's another story.) If we extend this check to also skip running signal handlers when the next opcode is YIELD_FROM, then that closes the hole – now the exception can only be raised at the innermost stack frame.

    This shouldn't have any performance implications, because the opcode check happens inside the "slow path" after we've already determined that there's a pending signal or something similar for us to process; the vast majority of the time this isn't true.

    I'll post a PR in a few minutes that has a test case that demonstrates the problem and fails on current master, plus the fix.

    @njsmith njsmith added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 11, 2017
    @1st1
    Copy link
    Member

    1st1 commented May 17, 2017

    New changeset ab4413a by Yury Selivanov (Nathaniel J. Smith) in branch 'master':
    bpo-30039: Don't run signal handlers while resuming a yield from stack (bpo-1081)
    ab4413a

    @vstinner
    Copy link
    Member

    The change should be backported to 3.5 and 3.6, right? The change seems very short and safe. IMHO it's ok to backport.

    @1st1
    Copy link
    Member

    1st1 commented May 17, 2017

    Yes, I'll do the backport.

    @1st1
    Copy link
    Member

    1st1 commented Jun 9, 2017

    New changeset e89f95b by Yury Selivanov in branch '3.6':
    [3.6] bpo-30039: Don't run signal handlers while resuming a yield from stack (GH-1081) (bpo-1640)
    e89f95b

    @1st1 1st1 closed this as completed Jun 9, 2017
    @1st1 1st1 added the type-bug An unexpected behavior, bug, or error label Jun 9, 2017
    @vstinner
    Copy link
    Member

    vstinner commented Jun 9, 2017

    Why not backporting the fix to 3.5 as well?

    @1st1
    Copy link
    Member

    1st1 commented Jun 9, 2017

    I don't think we need to. Isn't 3.5 is in security/important bug fix mode? I don't view this change as an important one (it's just a nice thing to have).

    @vstinner
    Copy link
    Member

    Yury Selivanov added the comment:

    I don't think we need to. Isn't 3.5 is in security/important bug fix mode? I don't view this change as an important one (it's just a nice thing to have).

    Not yet, it still accept bug fixes:
    https://docs.python.org/devguide/#status-of-python-branches

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants