bpo-33930: Fix segfault with deep recursion when cleaning method objects #27678

pablogsal · 2021-08-09T13:30:31Z

https://bugs.python.org/issue33930

Co-authored by Augusto Goulart

Lib/test/test_exceptions.py

Objects/methodobject.c

…od objects

vstinner

LGTM. But I have a few minor comments.

Lib/test/test_exceptions.py

Objects/methodobject.c

…ng method objects

vstinner

LGTM.

vstinner · 2021-08-10T17:09:20Z

Objects/methodobject.c

@@ -160,7 +160,10 @@ PyCMethod_GetClass(PyObject *op)
 static void
 meth_dealloc(PyCFunctionObject *m)
 {
-    _PyObject_GC_UNTRACK(m);
+    // The Py_TRASHCAN mechanism requires that we be able to
+    // call PyObject_GC_UnTrack twice on an object.


I don't understand that, but thanks for the comment!

@tim-one added this comment in 803526b but the original change is by @nascheme, committed by @gvanrossum in ff413af with the following description:

This is Neil's fix for SF bug 535905 (Evil Trashcan and GC interaction). The fix makes it possible to call PyObject_GC_UnTrack() more than once on the same object, and then move the PyObject_GC_UnTrack() call to *before* the trashcan code is invoked.

The "SF bug" is also available on BPO: https://bugs.python.org/issue535905

If the comment is unclear, maybe we should try to improve it. I believe the main point is that, since the object uses the trashcan, the function version of untrack is used rather than the macro version. The macro is not safe to call twice because it assumes the object is a part of the GC double linked list. At least, that's my memory of it.

Why the trashcan calls untrack more than once is obscure and I don't recall why it does so. I would need to study the code. My guess would be because it (ab)uses the GC head pointers to store the object in a special trashcan linked list.

Maybe it would be more clear if you would say that the code before Py_TRASHCAN_BEGIN can be executed more than once. Calling the macro version of untrack twice is not safe while the function version is safe.

Misc/NEWS.d/next/Core and Builtins/2021-08-09-14-29-52.bpo-33930.--5LQ-.rst

…30.--5LQ-.rst Co-authored-by: Łukasz Langa <lukasz@langa.pl>

miss-islington · 2021-08-10T23:34:23Z

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2021-08-10T23:34:23Z

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington · 2021-08-10T23:34:23Z

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington · 2021-08-10T23:34:33Z

Sorry, @pablogsal, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker bfc2d5a5c4550ab3a2fadeb9459b4bd948ff61a2 3.8

miss-islington · 2021-08-10T23:34:34Z

Sorry @pablogsal, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker bfc2d5a5c4550ab3a2fadeb9459b4bd948ff61a2 3.9

bedevere-bot · 2021-08-10T23:34:42Z

GH-27719 is a backport of this pull request to the 3.10 branch.

…cts (pythonGH-27678) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

miss-islington · 2021-08-10T23:34:51Z

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-08-10T23:34:59Z

GH-27720 is a backport of this pull request to the 3.9 branch.

…cts (pythonGH-27678) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

pablogsal · 2021-08-10T23:35:19Z

@ambv do you want to backport this to 3.8?

…cts (GH-27678) (GH-27720) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

…cts (GH-27678) (GH-27719) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

…d objects (pythonGH-27678). (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

bedevere-bot · 2021-08-11T09:04:57Z

GH-27721 is a backport of this pull request to the 3.8 branch.

…d objects (GH-27678) (GH-27721) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

…d objects (pythonGH-27678) (pythonGH-27721) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

bpo-33930: Fix segfault with deep recursion when cleaning method objects

a444686

the-knights-who-say-ni added the CLA signed label Aug 9, 2021

bedevere-bot added the awaiting core review label Aug 9, 2021

serhiy-storchaka reviewed Aug 9, 2021

View reviewed changes

Lib/test/test_exceptions.py Show resolved Hide resolved

iritkatriel reviewed Aug 9, 2021

View reviewed changes

Objects/methodobject.c Outdated Show resolved Hide resolved

fixup! bpo-33930: Fix segfault with deep recursion when cleaning meth…

6bcd0a0

…od objects

pablogsal force-pushed the bpo-33930 branch from 82fe9e0 to 6bcd0a0 Compare August 9, 2021 14:01

vstinner approved these changes Aug 10, 2021

View reviewed changes

Lib/test/test_exceptions.py Show resolved Hide resolved

Lib/test/test_exceptions.py Outdated Show resolved Hide resolved

Objects/methodobject.c Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 10, 2021

fixup! fixup! bpo-33930: Fix segfault with deep recursion when cleani…

80ac4cf

…ng method objects

vstinner approved these changes Aug 10, 2021

View reviewed changes

ambv reviewed Aug 10, 2021

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2021-08-09-14-29-52.bpo-33930.--5LQ-.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Core and Builtins/2021-08-09-14-29-52.bpo-339…

738fecc

…30.--5LQ-.rst Co-authored-by: Łukasz Langa <lukasz@langa.pl>

pablogsal closed this Aug 10, 2021

pablogsal reopened this Aug 10, 2021

pablogsal mentioned this pull request Aug 10, 2021

bpo-44881: Integrate GC untrack into trashcan begin. #27718

Closed

pablogsal merged commit bfc2d5a into python:main Aug 10, 2021

pablogsal deleted the bpo-33930 branch August 10, 2021 23:34

bedevere-bot removed the awaiting merge label Aug 10, 2021

pablogsal added needs backport to 3.8 needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Aug 10, 2021

miss-islington assigned pablogsal Aug 10, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 10, 2021

pablogsal removed the needs backport to 3.9 only security fixes label Aug 10, 2021

pablogsal added the needs backport to 3.9 only security fixes label Aug 10, 2021

bedevere-bot removed the needs backport to 3.9 only security fixes label Aug 10, 2021

vstinner pushed a commit that referenced this pull request Aug 11, 2021

bpo-33930: Fix segfault with deep recursion when cleaning method obje…

15f0a45

…cts (GH-27678) (GH-27720) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

vstinner pushed a commit that referenced this pull request Aug 11, 2021

bpo-33930: Fix segfault with deep recursion when cleaning method obje…

d6d2d54

…cts (GH-27678) (GH-27719) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

ambv pushed a commit to ambv/cpython that referenced this pull request Aug 11, 2021

[3.8] bpo-33930: Fix segfault with deep recursion when cleaning metho…

0e0e2f7

…d objects (pythonGH-27678). (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

bedevere-bot removed the needs backport to 3.8 label Aug 11, 2021

ambv added a commit that referenced this pull request Aug 11, 2021

[3.8] bpo-33930: Fix segfault with deep recursion when cleaning metho…

09d814b

…d objects (GH-27678) (GH-27721) (cherry picked from commit bfc2d5a) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

Uh oh!

bpo-33930: Fix segfault with deep recursion when cleaning method objects #27678

bpo-33930: Fix segfault with deep recursion when cleaning method objects #27678

Uh oh!

Conversation

pablogsal commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

ambv Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nascheme Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

nascheme Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington commented Aug 10, 2021

Uh oh!

miss-islington commented Aug 10, 2021

Uh oh!

miss-islington commented Aug 10, 2021

Uh oh!

miss-islington commented Aug 10, 2021

Uh oh!

miss-islington commented Aug 10, 2021

Uh oh!

bedevere-bot commented Aug 10, 2021

Uh oh!

miss-islington commented Aug 10, 2021

Uh oh!

bedevere-bot commented Aug 10, 2021

Uh oh!

pablogsal commented Aug 10, 2021

Uh oh!

bedevere-bot commented Aug 11, 2021

Uh oh!

Uh oh!

pablogsal commented Aug 9, 2021 •

edited

Loading

ambv Aug 10, 2021 •

edited

Loading