[issue42015] Order of decrementing reference counts in meth_dealloc

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Henry Schreiner]

Henry Schreiner  added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[miss-islington]

miss-islington  added the comment:


New changeset 8a12503b4532e33d590ecea7eb94cd0e6b0f1488 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)
https://github.com/python/cpython/commit/8a12503b4532e33d590ecea7eb94cd0e6b0f1488


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[miss-islington]

Change by miss-islington :


--
nosy: +miss-islington
nosy_count: 3.0 -> 4.0
pull_requests: +21648
pull_request: https://github.com/python/cpython/pull/22674

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset 04b8631d84a870dda456ef86039c1baf34d08500 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)
https://github.com/python/cpython/commit/04b8631d84a870dda456ef86039c1baf34d08500


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Thank you Yannick for your report and PR!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Yannick Jadoul]

Yannick Jadoul  added the comment:

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

Meanwhile, https://github.com/python/cpython/pull/22670 should solve our 
issues. I think Henry confirmed this locally?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Yannick Jadoul]

Change by Yannick Jadoul :


--
keywords: +patch
pull_requests: +21643
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/22670

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Do you mind to create a PR Yannick?

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Henry Schreiner]

Change by Henry Schreiner :


--
nosy: +Henry Schreiner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42015] Order of decrementing reference counts in meth_dealloc

[Yannick Jadoul]

New submission from Yannick Jadoul :

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 `PyCFunctionObject`s 
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: https://github.com/pybind/pybind11/issues/2558
- pybind11 PR: https://github.com/pybind/pybind11/pull/2576

--
components: C API, Interpreter Core
messages: 378500
nosy: YannickJadoul
priority: normal
severity: normal
status: open
title: Order of decrementing reference counts in meth_dealloc
type: crash
versions: Python 3.10, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com