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

Possible fatal errors due to _PyEval_SetAsyncGen{Finalizer,Firstiter}() #82591

Closed
ZackerySpytz mannequin opened this issue Oct 8, 2019 · 15 comments
Closed

Possible fatal errors due to _PyEval_SetAsyncGen{Finalizer,Firstiter}() #82591

ZackerySpytz mannequin opened this issue Oct 8, 2019 · 15 comments
Labels
3.8 only security fixes 3.9 only security fixes deferred-blocker easy interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@ZackerySpytz
Copy link
Mannequin

ZackerySpytz mannequin commented Oct 8, 2019

BPO 38410
Nosy @ambv, @serhiy-storchaka, @zooba, @ZackerySpytz
PRs
  • bpo-38410: Properly handle PySys_Audit() failures #16657
  • [3.8] bpo-38410: Properly handle PySys_Audit() failures #18658
  • 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 = <Date 2020-03-26.12:12:21.384>
    created_at = <Date 2019-10-08.14:06:14.978>
    labels = ['interpreter-core', 'easy', 'deferred-blocker', '3.8', '3.9', 'type-crash']
    title = 'Possible fatal errors due to _PyEval_SetAsyncGen{Finalizer,Firstiter}()'
    updated_at = <Date 2020-03-26.12:12:21.383>
    user = 'https://github.com/ZackerySpytz'

    bugs.python.org fields:

    activity = <Date 2020-03-26.12:12:21.383>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-03-26.12:12:21.384>
    closer = 'steve.dower'
    components = ['Interpreter Core']
    creation = <Date 2019-10-08.14:06:14.978>
    creator = 'ZackerySpytz'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38410
    keywords = ['patch', 'newcomer friendly']
    message_count = 15.0
    messages = ['354210', '354220', '354602', '354637', '354641', '354868', '354910', '358102', '362638', '362652', '362659', '363135', '365057', '365062', '365063']
    nosy_count = 4.0
    nosy_names = ['lukasz.langa', 'serhiy.storchaka', 'steve.dower', 'ZackerySpytz']
    pr_nums = ['16657', '18658']
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue38410'
    versions = ['Python 3.8', 'Python 3.9']

    @ZackerySpytz
    Copy link
    Mannequin Author

    ZackerySpytz mannequin commented Oct 8, 2019

    _PyEval_SetAsyncGenFinalizer() and _PyEval_SetAsyncGenFirstiter() don't include proper error handling for their PySys_Audit() calls. This could lead to leaked exceptions and fatal errors.

    @ZackerySpytz ZackerySpytz mannequin added 3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 8, 2019
    @zooba
    Copy link
    Member

    zooba commented Oct 8, 2019

    You're right, they need either your patch or PyErr_WriteUnraisable(NULL) before returning.

    Łukasz - this needs a fix in 3.8, but we don't have to necessarily change the (internal, but exposed) ABI. For 3.9, we'll fix it properly, but for 3.8 I'll let you make the call whether we can also add a return value to these functions.

    -PyAPI_FUNC(void) _PyEval_SetAsyncGenFirstiter(PyObject *);
    +PyAPI_FUNC(int) _PyEval_SetAsyncGenFirstiter(PyObject *);
     PyAPI_FUNC(PyObject *) _PyEval_GetAsyncGenFirstiter(void);
    -PyAPI_FUNC(void) _PyEval_SetAsyncGenFinalizer(PyObject *);
    +PyAPI_FUNC(int) _PyEval_SetAsyncGenFinalizer(PyObject *);

    @ambv
    Copy link
    Contributor

    ambv commented Oct 13, 2019

    Unfortunately at this point we will have to leave the ABI as is. We are in fact promising to lock it by Beta 3 so quite a long time ago.

    @ambv ambv removed the 3.8 only security fixes label Oct 13, 2019
    @zooba
    Copy link
    Member

    zooba commented Oct 14, 2019

    In that case, the fix for 3.8 should be to call PyErr_WriteUnraisable(NULL) when an error occurs. This will report it through the new unraisable hook, and also prevent a fatal error if the exception escapes.

    @zooba zooba added the 3.8 only security fixes label Oct 14, 2019
    @zooba
    Copy link
    Member

    zooba commented Oct 14, 2019

    But I'm okay to defer it - this is still going to be obscure functionality for the time being, and it's possible to avoid the issue.

    @serhiy-storchaka
    Copy link
    Member

    Why this ABI is exported at all? These functions are only used in the sys module, why they are defined in ceval.c instead of be static functions in sysmodule.c?

    @zooba
    Copy link
    Member

    zooba commented Oct 18, 2019

    Why this ABI is exported at all?

    Great question for (probably) Yury, but it is exported and so we're stuck with it for 3.8 at least :(

    @ambv
    Copy link
    Contributor

    ambv commented Dec 9, 2019

    Note: this is going to miss Python 3.8.1 as I'm releasing 3.8.1rc1 right now. Please address this before 3.8.2 (due in February).

    @ambv
    Copy link
    Contributor

    ambv commented Feb 25, 2020

    Sadly, this missed the train for 3.8.2. Steve, what's the status of the open PR for this?

    @zooba
    Copy link
    Member

    zooba commented Feb 25, 2020

    I thought we weren't going to take the fix for 3.8 anyway? (Or did we make a smaller one and commit it directly, apparently without linking it to the issue correctly?)

    The status is the PR needs conflicts resolved, and people need to stop adding to my infinite list of GitHub notifications and ping me on here instead :)

    @serhiy-storchaka
    Copy link
    Member

    PR 18658 is a backport to 3.8 which preserves binary compatibility.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 87a4cd5 by Serhiy Storchaka in branch '3.8':
    bpo-38410: Properly handle PySys_Audit() failures (GH-18658)
    87a4cd5

    @ZackerySpytz
    Copy link
    Mannequin Author

    ZackerySpytz mannequin commented Mar 26, 2020

    I had pinged Steve Dower on the PR, but I didn't realize they weren't notified. The merge conflicts have been fixed.

    @zooba
    Copy link
    Member

    zooba commented Mar 26, 2020

    Sorry for missing the pings! My GitHub notifications are a bit of a black hole. I'll merge it now.

    @zooba
    Copy link
    Member

    zooba commented Mar 26, 2020

    New changeset 79ceccd by Zackery Spytz in branch 'master':
    bpo-38410: Properly handle PySys_Audit() failures (GH-16657)
    79ceccd

    @zooba zooba closed this as completed Mar 26, 2020
    @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 deferred-blocker easy interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants