This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Negative Refcount in Python 3.8
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.8
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: Christian.Tismer, pablogsal, petr.viktorin
Priority: critical Keywords: patch

Created on 2019-12-10 15:37 by Christian.Tismer, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 17555 closed Christian.Tismer, 2019-12-10 15:38
Messages (12)
msg358198 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-10 15:37
By the new Py_TPFLAGS_METHOD_DESCRIPTOR flag, a new code path is
activated, and when extension types like PySide create a new
class, we observe negative refcounts.

The reason is that the code in typeobject.c fkt. type_mro_modified
calls lookup_maybe_method which returns a _borrowed_ reference.
This happens in the "if (custom) {" branch.

Removing all Py_XDECREF calls from the function fixes that.
msg358201 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-12-10 16:21
From the PR:

Christian, can you open the PR against master instead? We will backport the change to 3.8 after is merged.
msg358203 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-10 16:54
No, this appears to be impossible.

The function "type_mro_modified" exists as well, but there is no
"if (custom) {" branch at all!

On 10.12.19 17:21, Pablo Galindo Salgado wrote:
> 
> Pablo Galindo Salgado <pablogsal@gmail.com> added the comment:
> 
>>From the PR:
> 
> Christian, can you open the PR against master instead? We will backport the change to 3.8 after is merged.
> 
> ----------
> nosy: +pablogsal
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue39016>
> _______________________________________
>
msg358205 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-12-10 16:57
> No, this appears to be impossible.

Oh, I see. Apologies then for the misunderstunding.
msg358207 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-10 17:07
On 10.12.19 17:57, Pablo Galindo Salgado wrote:
> 
> Pablo Galindo Salgado <pablogsal@gmail.com> added the comment:
> 
>> No, this appears to be impossible.
> 
> Oh, I see. Apologies then for the misunderstunding.

Well, but I think that is weird, too!
Why should that custom clause be in 3.8 but not in master?
msg358228 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-10 22:13
On 10.12.19 17:57, Pablo Galindo Salgado wrote:
> 
> Pablo Galindo Salgado <pablogsal@gmail.com> added the comment:
> 
>> No, this appears to be impossible.
> 
> Oh, I see. Apologies then for the misunderstunding.

No problem! You could as well have been right.
I tried to move the patch to master and realized only then
that there was nothing similar. Interesting btw., I should
investigate when this divergence was introduced.

Best -- Chris
msg358260 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-12-11 13:13
lookup_maybe_method should not return a borrowed reference. It increfs its return value.
msg358268 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-11 15:48
On 11.12.19 14:13, Petr Viktorin wrote:
> 
> Petr Viktorin <encukou@gmail.com> added the comment:
> 
> lookup_maybe_method should not return a borrowed reference. It increfs its return value.

At second sight, this seems to be true. No idea why I
was so convinced. Need to sleep and look again..
msg358271 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-12-11 16:37
The goal now should be to find a reasonably small reproducer.

I'm trying to compile PySide to see what it does, but it's a big project and I'm a bit lost.
Christian, you said in an e-mail that you have a workaround -- unsetting the Py_TPFLAGS_METHOD_DESCRIPTOR. Could you point me to the commit with that workaround, so I could try exploring the code in context?
msg358274 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-11 17:00
On 11.12.19 17:37, Petr Viktorin wrote:
> 
> Petr Viktorin <encukou@gmail.com> added the comment:
> 
> The goal now should be to find a reasonably small reproducer.
> 
> I'm trying to compile PySide to see what it does, but it's a big project and I'm a bit lost.
> Christian, you said in an e-mail that you have a workaround -- unsetting the Py_TPFLAGS_METHOD_DESCRIPTOR. Could you point me to the commit with that workaround, so I could try exploring the code in context?

Yes, it went into gerrit here:

https://codereview.qt-project.org/c/pyside/pyside-setup/+/282705

The problem is that during PySide type creation the PyType_Ready
function looks into the mro() method, which uses this new
flag. When I temporary remove that, everything works.

Cheers -- Chris
msg358275 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-12-11 17:05
Thanks!

(note: unfortunately I'll probably not have time for CPython until next week, but further investigation is on my list.)
msg358284 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-12-11 22:44
Sorry, I believe I was wrong and lookup_maybe_method does not return a borrowed reference. _PyType_Lookup does and I was confused.
Closing that for now.
History
Date User Action Args
2022-04-11 14:59:24adminsetgithub: 83197
2019-12-11 22:46:06Christian.Tismersetstatus: open -> closed
resolution: not a bug
2019-12-11 22:45:03Christian.Tismersetstage: patch review -> resolved
2019-12-11 22:44:24Christian.Tismersetmessages: + msg358284
2019-12-11 17:05:49petr.viktorinsetmessages: + msg358275
2019-12-11 17:00:43Christian.Tismersetmessages: + msg358274
2019-12-11 16:37:27petr.viktorinsetmessages: + msg358271
2019-12-11 15:48:54Christian.Tismersetmessages: + msg358268
2019-12-11 13:13:40petr.viktorinsetnosy: + petr.viktorin
messages: + msg358260
2019-12-10 22:13:15Christian.Tismersetmessages: + msg358228
2019-12-10 17:07:32Christian.Tismersetmessages: + msg358207
2019-12-10 16:57:16pablogsalsetmessages: + msg358205
2019-12-10 16:54:54Christian.Tismersetmessages: + msg358203
2019-12-10 16:21:43pablogsalsetnosy: + pablogsal
messages: + msg358201
2019-12-10 15:38:18Christian.Tismersetkeywords: + patch
stage: patch review
pull_requests: + pull_request17029
2019-12-10 15:37:03Christian.Tismercreate