classification
Title: tp_descr_get(self, obj, type) is called without owning a reference to "self"
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Claudio.Freire, devurandom, erik.bray, fbissey, jdemeyer, miss-islington, pitrou, scoder, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-11-27 19:56 by jdemeyer, last changed 2018-11-20 18:45 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
descr_ref.patch jdemeyer, 2015-11-27 19:56 Patch
descr_ref-2.patch vstinner, 2017-01-03 14:29 review
Pull Requests
URL Status Linked Edit
PR 6118 merged python-dev, 2018-03-14 20:10
PR 9084 merged jdemeyer, 2018-09-06 18:38
PR 9087 merged miss-islington, 2018-09-07 07:37
PR 9088 merged miss-islington, 2018-09-07 07:37
PR 9089 closed miss-islington, 2018-09-07 07:37
PR 9091 merged vstinner, 2018-09-07 07:56
PR 10234 merged serhiy.storchaka, 2018-11-20 17:40
Messages (25)
msg255480 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2015-11-27 19:56
`type_getattro()` calls `tp_descr_get(self, obj, type)` without actually owning a reference to "self". In very rare cases, this can cause a segmentation fault if "self" is deleted by the descriptor.

Downstream: [http://trac.sagemath.org/ticket/19633]
msg255565 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2015-11-29 08:23
This is a known general issue which is documented in Lib/test/crashers/borrowed_ref_1 inside the 2.7 branch.  In trunk, I see that this file has been deleted, although the issue has not been solved in general.  Only the particular crash in the file has been solved.  (Somebody with motivation should restore the crashers that have been solved superficially only.  But finding actual crashing code takes more effort than I'm willing to put into this rather pointless process, so CPython still gets the occasional bug report like this one instead.)
msg255566 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2015-11-29 08:56
Thanks for the pointer. My patch does fix the crash in Lib/test/crashers/borrowed_ref_2.py on Python 2.7.10.
msg256352 - (view) Author: François Bissey (fbissey) Date: 2015-12-13 23:28
Will Jeroen's patch make it into 2.7.12 or are you expecting more stuff before committing a change?
msg274202 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2016-09-02 05:09
I haven't seen any crashes in the wild here, but this is still the case in the latest code base. The change doesn't seem invasive, so I don't see why it shouldn't get implemented.
msg284140 - (view) Author: Claudio Freire (Claudio.Freire) Date: 2016-12-27 23:05
I cannot be 100% sure, but we have ample evidence suggesting we're experiencing this same crash in production.

We have a big system that mixes Cython and pure-python coroutines, and in one version we started seeing segfaults that strongly hint at this root cause.

Adding pure-python indirections (that keep the arguments alive, I'd wager), fixes those segfaults.

I cannot share the codebase (and in any case it's too big, and the crash is too difficult to reproduce in isolation, without real traffic), but I'd add my +1 on applying this fix.

We're currently testing to try and reproduce the segfaults on 2.7.13, after that I'll try jdemeyer's patch and report the results.
msg284570 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-03 14:29
descr_ref-2.patch: patch rebased on the 2.7 branch.
msg284571 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-03 14:31
Claudio: "We're currently testing to try and reproduce the segfaults on 2.7.13, after that I'll try jdemeyer's patch and report the results."

Cool! Keep us in touch ;-)
msg284573 - (view) Author: Claudio Freire (Claudio.Freire) Date: 2017-01-03 14:36
The crash (the one we're experiencing) still happens with 2.7.13. But at this point it's not clear whether it's a Python bug or a Cython bug, as jdemeyer's patch doesn't fix it. We're having a hard time getting accurate backtraces to actually debug this thing, as it only happens on production servers.
msg284574 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-03 14:38
> The crash (the one we're experiencing) still happens with 2.7.13. But at this point it's not clear whether it's a Python bug or a Cython bug, as jdemeyer's patch doesn't fix it. We're having a hard time getting accurate backtraces to actually debug this thing, as it only happens on production servers.

I suggest you to try faulthandler to try to get a traceback ;-)
msg284575 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2017-01-03 14:42
<shameless plug> If you are on POSIX, you could also use cysignals to get a traceback (simply import cysignals, which will install a handler for fatal signals like SIGSEGV).
msg284576 - (view) Author: Claudio Freire (Claudio.Freire) Date: 2017-01-03 14:44
Nice ideas, will give them a try
msg324722 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 07:37
New changeset 8f735485acf2e35a75d2fa019feb8f905598c4e5 by Victor Stinner (jdemeyer) in branch 'master':
bpo-25750: fix refcounts in type_getattro() (GH-6118)
https://github.com/python/cpython/commit/8f735485acf2e35a75d2fa019feb8f905598c4e5
msg324723 - (view) Author: miss-islington (miss-islington) Date: 2018-09-07 07:50
New changeset f862f3abaed59b83763707ae529f0fe487961ba9 by Miss Islington (bot) in branch '3.7':
bpo-25750: fix refcounts in type_getattro() (GH-6118)
https://github.com/python/cpython/commit/f862f3abaed59b83763707ae529f0fe487961ba9
msg324724 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 07:57
New changeset 3ee07432f2e607cc6e7e6ea2d3695b672ceb1cea by Victor Stinner (Miss Islington (bot)) in branch '3.6':
bpo-25750: fix refcounts in type_getattro() (GH-6118) (GH-9088)
https://github.com/python/cpython/commit/3ee07432f2e607cc6e7e6ea2d3695b672ceb1cea
msg324728 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 08:15
New changeset bf2bd8f8a1d88de60c114de957f50fe2433e3937 by Victor Stinner in branch '2.7':
bpo-25750: fix refcounts in type_getattro() (GH-6118) (GH-9091)
https://github.com/python/cpython/commit/bf2bd8f8a1d88de60c114de957f50fe2433e3937
msg324729 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 08:16
The bug has been fixed in 2.7, 3.6, 3.7 and master branches. Thanks Jeroen Demeyer for your tenacity and hard work :-)

Let's see what we do with the unit test: PR 9084.
msg328065 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-19 21:50
New changeset 5a30620e68ebb911eef4d583de3776d782148637 by Victor Stinner (jdemeyer) in branch 'master':
bpo-25750: Add test on bad descriptor __get__() (GH-9084)
https://github.com/python/cpython/commit/5a30620e68ebb911eef4d583de3776d782148637
msg328176 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-20 19:23
This change introduced a compiler warning.

/home/serhiy/py/cpython/Modules/_testcapimodule.c:5042:17: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     {"bad_get", bad_get, METH_FASTCALL},
                 ^~~~~~~
/home/serhiy/py/cpython/Modules/_testcapimodule.c:5042:17: note: (near initialization for ‘TestMethods[173].ml_meth’)
msg328539 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 11:24
Comment on the commit:
https://github.com/python/cpython/commit/5a30620e68ebb911eef4d583de3776d782148637#commitcomment-31057082

"bad_get should be explicitly cast to PyCFunction here, or else the compiler will balk; see https://bugs.python.org/msg328176 "
msg328717 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-28 17:53
The cast warning is not specific to this issue, it's a more general issue that will be address in bpo-33012. I close again  the bug.
msg328898 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-30 11:46
bpo-33012 is about more strong warnings in gcc 8. This issue introduced a warning in gcc 7.
msg328899 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-30 11:50
Is it necessary to use METH_FASTCALL?
msg329286 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2018-11-05 08:28
> Is it necessary to use METH_FASTCALL?

In Python 3, the bug only occurs with METH_FASTCALL. The issue is a reference counting bug and the temporary tuple used for a METH_VARARGS method avoids the bug.
msg330144 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-20 18:45
New changeset b1dede3ee3498100b95265f7fdb0ea2bef9a2ba2 by Serhiy Storchaka in branch 'master':
bpo-25750: Fix a compiler warning introduced in GH-9084. (GH-10234)
https://github.com/python/cpython/commit/b1dede3ee3498100b95265f7fdb0ea2bef9a2ba2
History
Date User Action Args
2018-11-20 18:45:43serhiy.storchakasetmessages: + msg330144
2018-11-20 17:40:23serhiy.storchakasetpull_requests: + pull_request9857
2018-11-05 08:28:53jdemeyersetmessages: + msg329286
2018-10-30 11:50:11serhiy.storchakasetmessages: + msg328899
2018-10-30 11:46:47serhiy.storchakasetmessages: + msg328898
2018-10-28 17:53:12vstinnersetstatus: open -> closed

messages: + msg328717
2018-10-26 11:24:54vstinnersetmessages: + msg328539
2018-10-20 19:24:24serhiy.storchakasetstatus: closed -> open
2018-10-20 19:23:21serhiy.storchakasetmessages: + msg328176
2018-10-19 21:50:11vstinnersetmessages: + msg328065
2018-09-20 10:04:36jdemeyersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-09-07 08:16:51vstinnersetmessages: + msg324729
2018-09-07 08:15:42vstinnersetmessages: + msg324728
2018-09-07 07:58:28arigosetnosy: - arigo
2018-09-07 07:57:48vstinnersetmessages: + msg324724
2018-09-07 07:56:06vstinnersetpull_requests: + pull_request8549
2018-09-07 07:50:44miss-islingtonsetnosy: + miss-islington
messages: + msg324723
2018-09-07 07:37:36miss-islingtonsetpull_requests: + pull_request8548
2018-09-07 07:37:28miss-islingtonsetpull_requests: + pull_request8547
2018-09-07 07:37:19miss-islingtonsetpull_requests: + pull_request8546
2018-09-07 07:37:05vstinnersetmessages: + msg324722
2018-09-06 18:38:14jdemeyersetpull_requests: + pull_request8542
2018-03-14 20:10:30python-devsetpull_requests: + pull_request5881
2017-01-03 14:44:09Claudio.Freiresetmessages: + msg284576
2017-01-03 14:42:34jdemeyersetmessages: + msg284575
2017-01-03 14:38:16vstinnersetmessages: + msg284574
2017-01-03 14:36:42Claudio.Freiresetmessages: + msg284573
2017-01-03 14:31:06vstinnersetmessages: + msg284571
2017-01-03 14:30:00vstinnersetfiles: + descr_ref-2.patch

messages: + msg284570
2017-01-03 14:27:40vstinnersetnosy: + vstinner
2016-12-27 23:05:39Claudio.Freiresetnosy: + Claudio.Freire
messages: + msg284140
2016-11-29 20:52:34serhiy.storchakasetassignee: serhiy.storchaka
2016-09-07 10:00:54erik.braysetnosy: + erik.bray

stage: patch review
2016-09-02 05:09:39scodersetnosy: + pitrou, scoder, serhiy.storchaka

messages: + msg274202
versions: + Python 3.5, Python 3.6
2016-08-09 08:25:34devurandomsetnosy: + devurandom
2015-12-13 23:28:05fbisseysetnosy: + fbissey
messages: + msg256352
2015-11-29 08:56:41jdemeyersetmessages: + msg255566
2015-11-29 08:23:22arigosetnosy: + arigo
messages: + msg255565
2015-11-28 19:09:49jdemeyersettype: crash
2015-11-27 19:56:53jdemeyercreate