Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough #22670
Conversation
…m_self is kept alive long enough
|
And since this PR fixes a bug exposed in user code, add please a NEWS entry. |
…l, and add NEWS blurb
Not sure whether that mostly belongs in "C API" or "Core and Builtins", but I went with "C API" because I expected users are mostly affected when using the C API. If necessary, I can still move it. |
|
Thank you! Minor nitpick about the NEWS entry. |
Misc/NEWS.d/next/C API/2020-10-12-20-13-58.bpo-42015.X4H2_V.rst
Outdated
Show resolved
Hide resolved
|
Great! |
|
Thanks for the amazingly quick review, @serhiy-storchaka! Amazing to submit a PR like this :-) |
serhiy-storchaka
merged commit 04b8631
into
python:master
|
Thanks @YannickJadoul for the PR, and @serhiy-storchaka for merging it |
…m_self is kept alive long enough (pythonGH-22670) (cherry picked from commit 04b8631) Co-authored-by: Yannick Jadoul <[email protected]>
|
GH-22674 is a backport of this pull request to the 3.9 branch. |
|
I will be glad to work with you! |
…m_self is kept alive long enough (GH-22670) (cherry picked from commit 04b8631) Co-authored-by: Yannick Jadoul <[email protected]>
…m_self is kept alive long enough (pythonGH-22670)
…m_self is kept alive long enough (pythonGH-22670)
…m_self is kept alive long enough (pythonGH-22670)
In Python 3.9, the line
Py_XDECREF(PyCFunction_GET_CLASS(m));was added tometh_dealloc(inmethodobject.c). Unfortunately for pybind11, it's inserted exactly two lines too low, since it accesses thePyMethodDefand we store thePyMethodDefinstance in the capsule that's used asself-argument of thePyCFunction.Result: UB, since
Py_XDECREF(m->m_self);brings down the refcount of the capsule to 0 and (indirectly) frees thePyMethodDef, 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_XDECREF2 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.This PR reorders those lines.
If there's a different, recommended way of creating these
PyCFunctionObjects dynamically and cleaning up thePyMethodDef, 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.
https://bugs.python.org/issue42015