Order of decrementing reference counts in meth_dealloc

[YannickJadoul]

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]

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.

• Related issue: https://bugs.python.org/issue41237
• pybind11 issue: [bug] Python 3.9 macOS segfault pybind/pybind11#2558
• pybind11 PR: fix: valgrind-detected after-freeing access of PyMethodDef (macOS Python 3.9.0 segfaults) pybind/pybind11#2576

[serhiy-storchaka]

Do you mind to create a PR Yannick?

[YannickJadoul]

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]

Thank you Yannick for your report and PR!

[serhiy-storchaka]

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