tp_descr_get(self, obj, type) is called without owning a reference to "self" #69936

jdemeyer · 2015-11-27T19:56:53Z

BPO	25750
Nosy	@pitrou, @scoder, @vstinner, @embray, @serhiy-storchaka, @jdemeyer, @miss-islington
PRs	bpo-25750: fix refcounts in type_getattro() #6118 bpo-25750: add testcase on bad descriptor __get__() #9084 [3.7] bpo-25750: fix refcounts in type_getattro() (GH-6118) #9087 [3.6] bpo-25750: fix refcounts in type_getattro() (GH-6118) #9088 [2.7] bpo-25750: fix refcounts in type_getattro() (GH-6118) #9089 [2.7] bpo-25750: fix refcounts in type_getattro() (GH-6118) #9091 bpo-25750: Fix a compiler warning introduced in #9084. #10234
Files	descr_ref.patch: Patch descr_ref-2.patch

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2018-10-28.17:53:12.898>
created_at = <Date 2015-11-27.19:56:53.263>
labels = ['interpreter-core', 'type-crash']
title = 'tp_descr_get(self, obj, type) is called without owning a reference to "self"'
updated_at = <Date 2018-11-20.18:45:43.539>
user = 'https://github.com/jdemeyer'

bugs.python.org fields:

activity = <Date 2018-11-20.18:45:43.539>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-10-28.17:53:12.898>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2015-11-27.19:56:53.263>
creator = 'jdemeyer'
dependencies = []
files = ['41173', '46125']
hgrepos = []
issue_num = 25750
keywords = ['patch']
message_count = 25.0
messages = ['255480', '255565', '255566', '256352', '274202', '284140', '284570', '284571', '284573', '284574', '284575', '284576', '324722', '324723', '324724', '324728', '324729', '328065', '328176', '328539', '328717', '328898', '328899', '329286', '330144']
nosy_count = 10.0
nosy_names = ['pitrou', 'scoder', 'vstinner', 'devurandom', 'fbissey', 'erik.bray', 'serhiy.storchaka', 'jdemeyer', 'Claudio.Freire', 'miss-islington']
pr_nums = ['6118', '9084', '9087', '9088', '9089', '9091', '10234']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue25750'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

jdemeyer · 2015-11-27T19:56:53Z

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]

arigo · 2015-11-29T08:23:22Z

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.)

jdemeyer · 2015-11-29T08:56:41Z

Thanks for the pointer. My patch does fix the crash in Lib/test/crashers/borrowed_ref_2.py on Python 2.7.10.

fbissey · 2015-12-13T23:28:05Z

Will Jeroen's patch make it into 2.7.12 or are you expecting more stuff before committing a change?

scoder · 2016-09-02T05:09:40Z

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.

ClaudioFreire · 2016-12-27T23:05:39Z

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.

vstinner · 2017-01-03T14:30:00Z

descr_ref-2.patch: patch rebased on the 2.7 branch.

vstinner · 2017-01-03T14:31:06Z

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 ;-)

ClaudioFreire · 2017-01-03T14:36:42Z

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.

vstinner · 2017-01-03T14:38:17Z

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 ;-)

jdemeyer · 2017-01-03T14:42:35Z

<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).

ClaudioFreire · 2017-01-03T14:44:10Z

Nice ideas, will give them a try

vstinner · 2018-09-07T07:37:05Z

New changeset 8f73548 by Victor Stinner (jdemeyer) in branch 'master':
bpo-25750: fix refcounts in type_getattro() (GH-6118)
8f73548

miss-islington · 2018-09-07T07:50:44Z

New changeset f862f3a by Miss Islington (bot) in branch '3.7':
bpo-25750: fix refcounts in type_getattro() (GH-6118)
f862f3a

vstinner · 2018-09-07T07:57:48Z

New changeset 3ee0743 by Victor Stinner (Miss Islington (bot)) in branch '3.6':
bpo-25750: fix refcounts in type_getattro() (GH-6118) (GH-9088)
3ee0743

vstinner · 2018-09-07T08:15:43Z

New changeset bf2bd8f by Victor Stinner in branch '2.7':
bpo-25750: fix refcounts in type_getattro() (GH-6118) (GH-9091)
bf2bd8f

vstinner · 2018-09-07T08:16:52Z

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.

vstinner · 2018-10-19T21:50:11Z

New changeset 5a30620 by Victor Stinner (jdemeyer) in branch 'master':
bpo-25750: Add test on bad descriptor __get__() (GH-9084)
5a30620

serhiy-storchaka · 2018-10-20T19:23:21Z

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’)

vstinner · 2018-10-26T11:24:55Z

Comment on the commit:
5a30620#commitcomment-31057082

"bad_get should be explicitly cast to PyCFunction here, or else the compiler will balk; see https://bugs.python.org/msg328176 "

vstinner · 2018-10-28T17:53:13Z

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.

serhiy-storchaka · 2018-10-30T11:46:48Z

bpo-33012 is about more strong warnings in gcc 8. This issue introduced a warning in gcc 7.

serhiy-storchaka · 2018-10-30T11:50:11Z

Is it necessary to use METH_FASTCALL?

jdemeyer · 2018-11-05T08:28:53Z

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.

serhiy-storchaka · 2018-11-20T18:45:43Z

New changeset b1dede3 by Serhiy Storchaka in branch 'master':
bpo-25750: Fix a compiler warning introduced in #53330. (GH-10234)
b1dede3

jdemeyer added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Nov 27, 2015

serhiy-storchaka self-assigned this Nov 29, 2016

jdemeyer closed this as completed Sep 20, 2018

serhiy-storchaka reopened this Oct 20, 2018

vstinner closed this as completed Oct 28, 2018

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tp_descr_get(self, obj, type) is called without owning a reference to "self" #69936

tp_descr_get(self, obj, type) is called without owning a reference to "self" #69936

jdemeyer commented Nov 27, 2015

jdemeyer commented Nov 27, 2015

arigo mannequin commented Nov 29, 2015

jdemeyer commented Nov 29, 2015

fbissey mannequin commented Dec 13, 2015

scoder commented Sep 2, 2016

ClaudioFreire mannequin commented Dec 27, 2016

vstinner commented Jan 3, 2017

vstinner commented Jan 3, 2017

ClaudioFreire mannequin commented Jan 3, 2017

vstinner commented Jan 3, 2017

jdemeyer commented Jan 3, 2017

ClaudioFreire mannequin commented Jan 3, 2017

vstinner commented Sep 7, 2018

miss-islington commented Sep 7, 2018

vstinner commented Sep 7, 2018

vstinner commented Sep 7, 2018

vstinner commented Sep 7, 2018

vstinner commented Oct 19, 2018

serhiy-storchaka commented Oct 20, 2018

vstinner commented Oct 26, 2018

vstinner commented Oct 28, 2018

serhiy-storchaka commented Oct 30, 2018

serhiy-storchaka commented Oct 30, 2018

jdemeyer commented Nov 5, 2018

serhiy-storchaka commented Nov 20, 2018

tp_descr_get(self, obj, type) is called without owning a reference to "self" #69936

tp_descr_get(self, obj, type) is called without owning a reference to "self" #69936

Comments

jdemeyer commented Nov 27, 2015

jdemeyer commented Nov 27, 2015

arigo mannequin commented Nov 29, 2015

jdemeyer commented Nov 29, 2015

fbissey mannequin commented Dec 13, 2015

scoder commented Sep 2, 2016

ClaudioFreire mannequin commented Dec 27, 2016

vstinner commented Jan 3, 2017

vstinner commented Jan 3, 2017

ClaudioFreire mannequin commented Jan 3, 2017

vstinner commented Jan 3, 2017

jdemeyer commented Jan 3, 2017

ClaudioFreire mannequin commented Jan 3, 2017

vstinner commented Sep 7, 2018

miss-islington commented Sep 7, 2018

vstinner commented Sep 7, 2018

vstinner commented Sep 7, 2018

vstinner commented Sep 7, 2018

vstinner commented Oct 19, 2018

serhiy-storchaka commented Oct 20, 2018

vstinner commented Oct 26, 2018

vstinner commented Oct 28, 2018

serhiy-storchaka commented Oct 30, 2018

serhiy-storchaka commented Oct 30, 2018

jdemeyer commented Nov 5, 2018

serhiy-storchaka commented Nov 20, 2018