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

Order of decrementing reference counts in meth_dealloc #86181

Closed
YannickJadoul mannequin opened this issue Oct 12, 2020 · 7 comments
Closed

Order of decrementing reference counts in meth_dealloc #86181

YannickJadoul mannequin opened this issue Oct 12, 2020 · 7 comments
Labels
3.9 only security fixes 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@YannickJadoul
Copy link
Mannequin

YannickJadoul mannequin commented Oct 12, 2020

BPO 42015
Nosy @serhiy-storchaka, @henryiii, @miss-islington, @YannickJadoul
PRs
  • bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough #22670
  • [3.9] bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough (GH-22670) #22674
    		•

    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-10-13.05:55:13.544>
created_at = <Date 2020-10-12.14:23:44.124>
labels = ['interpreter-core', 'expert-C-API', '3.10', '3.9', 'type-crash']
title = 'Order of decrementing reference counts in meth_dealloc'
updated_at = <Date 2020-10-13.05:55:13.543>
user = 'https://github.com/YannickJadoul'

    bugs.python.org fields:

    activity = <Date 2020-10-13.05:55:13.543>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2020-10-13.05:55:13.544>
closer = 'serhiy.storchaka'
components = ['Interpreter Core', 'C API']
creation = <Date 2020-10-12.14:23:44.124>
creator = 'YannickJadoul'
dependencies = []
files = []
hgrepos = []
issue_num = 42015
keywords = ['patch']
message_count = 7.0
messages = ['378500', '378502', '378506', '378511', '378525', '378528', '378529']
nosy_count = 4.0
nosy_names = ['serhiy.storchaka', 'Henry Schreiner', 'miss-islington', 'YannickJadoul']
pr_nums = ['22670', '22674']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue42015'
versions = ['Python 3.9', 'Python 3.10']
    @YannickJadoul
    Copy link
    Mannequin Author

    YannickJadoul mannequin commented Oct 12, 2020

    In Python 3.9, the line Py_XDECREF(PyCFunction_GET_CLASS(m)); was added to meth_dealloc (in methodobject.c). Unfortunately for pybind11, it's inserted exactly two lines too low, since it accesses the PyMethodDef and we store the PyMethodDef instance in the capsule that's used as self-argument of the PyCFunction.

    Result: UB, since Py_XDECREF(m->m_self); brings down the refcount of the capsule to 0 and (indirectly) frees the PyMethodDef, while its contents are now still accessed.

    From the pybind11 perspective, it would be optimal if this could be fixed in CPython itself, by moving up this one Py_XDECREF 2 lines. This would a) be more efficient than creating a workaround, and b) allow old, existing versions of pybind11 to work with Python 3.9 (well, 3.9.1, then, hopefully); the user base of pybind11 has grown quite a bit and now includes giants like scipy or some Google libraries.
    I will make a PR implementing this, soon.

    If there's a different, recommended way of creating these PyCFunctionObjects dynamically and cleaning up the PyMethodDef, we'd be interested as well, to make sure these kinds of breakages are avoided in the future.

    Apologies for only figuring out now how to debug this, using valgrind. Up until yesterday, we only saw some failures in CI on macOS, but it was hard to reproduce and debug locally.

    @YannickJadoul YannickJadoul mannequin added 3.9 only security fixes 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 12, 2020
    @serhiy-storchaka
    Copy link
    Member

    Do you mind to create a PR Yannick?

    @YannickJadoul
    Copy link
    Mannequin Author

    YannickJadoul mannequin commented Oct 12, 2020

    Yes, sorry for the delay; I got caught up in something else.

    Meanwhile, #22670 should solve our issues. I think Henry confirmed this locally?

    @serhiy-storchaka
    Copy link
    Member

    Thank you Yannick for your report and PR!

    @serhiy-storchaka
    Copy link
    Member

    New changeset 04b8631 by Yannick Jadoul in branch 'master':
    bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough (GH-22670)
    04b8631

    @miss-islington
    Copy link
    Contributor

    New changeset 8a12503 by Miss Skeleton (bot) in branch '3.9':
    bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough (GH-22670)
    8a12503

    @henryiii
    Copy link
    Mannequin

    henryiii mannequin commented Oct 12, 2020

    I tested before the patch, and I got 17 segfaults running a pybind11 module 20 times. After the patch, I ran about 50 times and had no segfaults!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @t-kalinowski t-kalinowski mentioned this issue Jul 15, 2022
    Closed
    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 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API 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
    2 participants
    @serhiy-storchaka @miss-islington