bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough by YannickJadoul · Pull Request #22670 · python/cpython · GitHub

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.
This PR reorders those lines.

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

https://bugs.python.org/issue42015

…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

And since this PR fixes a bug exposed in user code, add please a NEWS entry.

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.

Great!

Thanks for the amazingly quick review, @serhiy-storchaka! Amazing to submit a PR like this :-)

Thanks @YannickJadoul for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

…m_self is kept alive long enough (pythonGH-22670) (cherry picked from commit 04b8631) Co-authored-by: Yannick Jadoul <yannick.jadoul@belgacom.net>

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 <yannick.jadoul@belgacom.net>

…m_self is kept alive long enough (pythonGH-22670)

YannickJadoul:reorder-meth_dealloc-decref

…m_self is kept alive long enough (pythonGH-22670)

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure …

50bf715

…m_self is kept alive long enough

the-knights-who-say-ni added the CLA signed label Oct 12, 2020

bedevere-bot added the awaiting review label Oct 12, 2020

serhiy-storchaka reviewed Oct 12, 2020

View reviewed changes

Comment thread Objects/methodobject.c

serhiy-storchaka added needs backport to 3.9 type-bug An unexpected behavior, bug, or error labels Oct 12, 2020

Add comment on derefering order, change order in meth_traverse as wel…

1e69eff

…l, and add NEWS blurb

serhiy-storchaka reviewed Oct 12, 2020

View reviewed changes

Comment thread Misc/NEWS.d/next/C API/2020-10-12-20-13-58.bpo-42015.X4H2_V.rst Outdated

Rephrase NEWS fragment to be more user-focussed

c508a4f

serhiy-storchaka approved these changes Oct 12, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Oct 12, 2020

serhiy-storchaka merged commit 04b8631 into python:master Oct 12, 2020

bedevere-bot removed the awaiting merge label Oct 12, 2020

bedevere-bot removed the needs backport to 3.9 label Oct 12, 2020

YannickJadoul deleted the reorder-meth_dealloc-decref branch October 12, 2020 21:10

YannickJadoul mentioned this pull request Oct 12, 2020

fix: valgrind-detected after-freeing access of PyMethodDef (macOS Python 3.9.0 segfaults) pybind/pybind11#2576

Merged

mbargull mentioned this pull request Oct 14, 2020

Add patch for https://bugs.python.org/issue42015 (which affects pybind11) conda-forge/python-feedstock#402

Merged

5 tasks

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure …

0b6c5b5

…m_self is kept alive long enough (pythonGH-22670)

reinecke mentioned this pull request Oct 30, 2020

Python 3.9 Support AcademySoftwareFoundation/OpenTimelineIO#828

Closed

rwgk mentioned this pull request Nov 2, 2020

Use weakref to clean up captured function object in def_buffer pybind/pybind11#2634

Merged

ax3l mentioned this pull request Nov 24, 2020

Python: 3.6 - 3.9 openPMD/openPMD-api#828

Merged

This was referenced Dec 3, 2020

Updates for Python 3.9.0 pathfinder-for-autonomous-navigation/lin#18

Merged

Updates for Python 3.9.0 pathfinder-for-autonomous-navigation/psim#279

Merged

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure …

21448b3

…m_self is kept alive long enough (pythonGH-22670)

ssheorey mentioned this pull request May 6, 2021

PyPi has open3d 0.12 only for Python 3.8 and earlier, not for 3.9 isl-org/Open3D#3065

Closed

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough by YannickJadoul · Pull Request #22670 · python/cpython · GitHub

Uh oh!

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough - #22670

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough#22670
serhiy-storchaka merged 3 commits into
python:masterfrom
YannickJadoul:reorder-meth_dealloc-decref

YannickJadoul commented Oct 12, 2020 •
edited by bedevere-bot
Loading

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Oct 12, 2020

Uh oh!

YannickJadoul commented Oct 12, 2020

Uh oh!

serhiy-storchaka left a comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Uh oh!

YannickJadoul commented Oct 12, 2020

Uh oh!

miss-islington commented Oct 12, 2020

Uh oh!

bedevere-bot commented Oct 12, 2020

Uh oh!

serhiy-storchaka commented Oct 12, 2020

Uh oh!

danielleoking7474 commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough by YannickJadoul · Pull Request #22670 · python/cpython · GitHub

Uh oh!

bpo-42015: Reorder dereferencing calls in meth_dealloc, to make sure m_self is kept alive long enough - #22670

Conversation

YannickJadoul commented Oct 12, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Oct 12, 2020

Uh oh!

YannickJadoul commented Oct 12, 2020

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

YannickJadoul commented Oct 12, 2020

Uh oh!

miss-islington commented Oct 12, 2020

Uh oh!

bedevere-bot commented Oct 12, 2020

Uh oh!

serhiy-storchaka commented Oct 12, 2020

Uh oh!

danielleoking7474 commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

YannickJadoul commented Oct 12, 2020 •
edited by bedevere-bot
Loading