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

Jump to bottom

crash if asyncio is used before and after re-initialization if using python embedded in an application #89425

Closed
benjamin-sch mannequin opened this issue Sep 22, 2021 · 9 comments
Closed

crash if asyncio is used before and after re-initialization if using python embedded in an application #89425

benjamin-sch mannequin opened this issue Sep 22, 2021 · 9 comments
Labels
3.9 only security fixes topic-asyncio type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@benjamin-sch
Copy link
Mannequin

benjamin-sch mannequin commented Sep 22, 2021

BPO 45262
Nosy @vstinner, @asvetlov, @1st1, @miss-islington, @HiassofT
PRs
  • bpo-45262: Prevent use-after-free in asyncio #28796
  • [3.9] bpo-45262, asyncio: Fix cache of the running loop holder (GH-28796) #28815
  • [3.10] bpo-45262, asyncio: Fix cache of the running loop holder (GH-28796) #28816
    		•

    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 = None
closed_at = None
created_at = <Date 2021-09-22.08:32:26.699>
labels = ['3.9', 'type-crash', 'expert-asyncio']
title = 'crash if asyncio is used before and after re-initialization if using python embedded in an application'
updated_at = <Date 2021-10-08.08:55:45.466>
user = 'https://bugs.python.org/benjamin-sch'

    bugs.python.org fields:

    activity = <Date 2021-10-08.08:55:45.466>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2021-09-22.08:32:26.699>
creator = 'benjamin-sch'
dependencies = []
files = []
hgrepos = []
issue_num = 45262
keywords = ['patch']
message_count = 8.0
messages = ['402412', '402469', '402905', '403407', '403445', '403448', '403462', '403463']
nosy_count = 6.0
nosy_names = ['vstinner', 'asvetlov', 'yselivanov', 'miss-islington', 'benjamin-sch', 'HiassofT']
pr_nums = ['28796', '28815', '28816']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue45262'
versions = ['Python 3.9']
    @benjamin-sch
    Copy link
    Mannequin Author

    benjamin-sch mannequin commented Sep 22, 2021

    We have embedded Python in our application and we deinitialize/initialize the interpreter at some point of time. If a simple script with a thread that sleeps with asyncio.sleep is loaded before and after the re-initialization, then we get the following assertion in the second run of the python module:

    "Assertion failed: Py_IS_TYPE(rl, &PyRunningLoopHolder_Type), file D:\a\1\s\Modules_asynciomodule.c, line 261"

    Example to reproduce this crash: https://github.com/benjamin-sch/asyncio_crash_in_second_run

    @benjamin-sch benjamin-sch mannequin added 3.9 only security fixes topic-asyncio type-crash A hard crash of the interpreter, possibly with a core dump labels Sep 22, 2021
    @asvetlov
    Copy link
    Contributor

    I guess the fix requires switching C Extension types from static to heap for _asyncio module.
    It is possible for sure but requires a non-trivial amount of work.

    We need a champion for the issue.

    @vstinner
    Copy link
    Member

    "Assertion failed: Py_IS_TYPE(rl, &PyRunningLoopHolder_Type), file D:\a\1\s\Modules_asynciomodule.c, line 261"

    The _asyncio extension uses static types and doesn't implement the multiphase initialization API. It doesn't work well with your "deinitialize/initialize the interpreter" use case.

    The _asyncio extension should use heap types and the multiphase init API.

    @HiassofT
    Copy link
    Mannequin

    HiassofT mannequin commented Oct 7, 2021

    We were hitting the same issue in kodi, which uses embedded sub-interpreters to run python addons, after one of the addons was switched to asyncio.

    The immediate cause of the assertion failure is a use-after-free issue from the running loop holder cache:

    When the running loop holder is deallocated (which happens eg on interpreter shutdown) cached_running_holder holds a dangling pointer.

    A subsequent call to get_running_loop() may then pick that up and boom.

    While probably not a full fix for this issue I think it would be good to fix the use-after-free problem - there could be other code paths that lead to this situation. I've created a github pull request for that

    @vstinner
    Copy link
    Member

    vstinner commented Oct 7, 2021

    New changeset 392a898 by Matthias Reichl in branch 'main':
    bpo-45262, asyncio: Fix cache of the running loop holder (GH-28796)
    392a898

    @miss-islington
    Copy link
    Contributor

    New changeset 87f0156 by Miss Islington (bot) in branch '3.9':
    bpo-45262, asyncio: Fix cache of the running loop holder (GH-28796)
    87f0156

    @vstinner
    Copy link
    Member

    vstinner commented Oct 8, 2021

    Is Matthias Reichl's fix enough to use asyncio in a subinterpreter?

    @vstinner
    Copy link
    Member

    vstinner commented Oct 8, 2021

    New changeset 6846d67 by Miss Islington (bot) in branch '3.10':
    bpo-45262, asyncio: Fix cache of the running loop holder (GH-28796) (GH-28816)
    6846d67

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @kumaraditya303
    Copy link
    Contributor

    The original issue was fixed, hence closing if there are more issue you can create new issue for it.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes topic-asyncio type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @asvetlov @miss-islington @kumaraditya303