bpo-31095: fix potential crash during GC. by methane · Pull Request #2974 · python/cpython · GitHub

Some tp_dealloc methods of GC types didn't call PyObject_GC_UnTrack().

https://bugs.python.org/issue31095

Some tp_dealloc methods of GC types didn't call PyObject_GC_UnTrack().

LGTM (besides two details).

But this touches delicate things, and I think this PR needs a review of yet one core developer.

_PyObject_GC_UNTRACK() shouldn't be used in types that can be subclassed because it can't be called repeatedly. Otherwise the subclass's destructor would need to call _PyObject_GC_TRACK() before calling parent's destructor:

static void
lru_cache_subclass_dealloc(lru_cache_subclass_object *obj)
{
    _PyObject_GC_UNTRACK(obj);
    Py_XDECREF(obj->foo);
    Py_XDECREF(obj->bar);
    _PyObject_GC_TRACK(obj);
    lru_cache_dealloc((lru_cache_object *)obj);
}

This looks too fragile.

Declaration after code is C99 syntax. The code will need rewriting for backporting to 2.7.

The same as for lru_cache. _ast.AST is subclassable.

lgtm. This suggests that we need a systematic way to deal with this problem, though.

I suggest to add a comment explaining why an explicit Untrack is required.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

SSL and elementtree changes LGTM

added required comment

(cherry picked from commit a6296d3)

GH-3195 is a backport of this pull request to the 3.6 branch.

(cherry picked from commit a6296d3)

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974 Fixes #35.

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

Thanks @methane for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

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

The commit 4cde4bd is the backport to Python 2.7.

call PyObject_GC_UnTrack() in tp_dealloc() see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

(cherry picked from commit a6296d3)

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

bpo-31095: fix potential crash during GC.

cbce046

Some tp_dealloc methods of GC types didn't call PyObject_GC_UnTrack().

methane added CLA not signed topic-asyncio type-bug An unexpected behavior, bug, or error labels Aug 1, 2017

methane requested a review from tiran as a code owner August 1, 2017 09:52

methane mentioned this pull request Aug 1, 2017

bpo-31061: fix crash in asyncio speedup module #2966

Merged

methane requested a review from serhiy-storchaka August 1, 2017 09:59

methane added needs backport to 3.5 and removed topic-asyncio labels Aug 1, 2017

serhiy-storchaka removed the needs backport to 3.5 label Aug 3, 2017

serhiy-storchaka approved these changes Aug 3, 2017

View reviewed changes

Replace _PyObject_GC_UNTRACK with PyObject_GC_UnTrack

0d576ca

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Aug 3, 2017

bpo-31095: Document PyObject_GC_UnTrack requirement

4e97f02

methane force-pushed the fix/31095 branch from b2132f7 to 4e97f02 Compare August 3, 2017 17:35

benjaminp approved these changes Aug 22, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Aug 22, 2017

vstinner previously requested changes Aug 22, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Aug 22, 2017

tiran approved these changes Aug 22, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Aug 22, 2017

Add note comments

1b2e354

vstinner approved these changes Aug 23, 2017

View reviewed changes

methane merged commit a6296d3 into python:master Aug 24, 2017

methane deleted the fix/31095 branch August 24, 2017 05:55

methane added a commit to methane/cpython that referenced this pull request Aug 24, 2017

[3.6] bpo-31095: fix potential crash during GC (pythonGH-2974 )

c4242b6

(cherry picked from commit a6296d3)

bedevere-bot removed the needs backport to 3.6 label Aug 24, 2017

methane added a commit to methane/cpython that referenced this pull request Aug 24, 2017

bpo-31095: fix potential crash during GC (pythonGH-2974 )

2c49cbb

(cherry picked from commit a6296d3)

Mariatta removed the awaiting merge label Aug 29, 2017

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

bpo-31095: fix potential crash during GC (pythonGH-2974 )

7ff9475

jamadden pushed a commit to zopefoundation/zope.security that referenced this pull request Sep 11, 2017

call PyObject_GC_UnTrack() in tp_dealloc()

a11c5e2

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974 Fixes #35.

jamadden mentioned this pull request Sep 11, 2017

call PyObject_GC_UnTrack() in tp_dealloc() zopefoundation/zope.security#37

Merged

stephan-hof pushed a commit to zopefoundation/Acquisition that referenced this pull request Sep 11, 2017

call PyObject_GC_UnTrack() in tp_dealloc()

61bae8d

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

stephan-hof added a commit to zopefoundation/Acquisition that referenced this pull request Sep 11, 2017

call PyObject_GC_UnTrack() in tp_dealloc()

19ea668

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

stephan-hof pushed a commit to zopefoundation/Acquisition that referenced this pull request Sep 11, 2017

call PyObject_GC_UnTrack() in tp_dealloc()

11c621e

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

Mariatta added needs backport to 2.7 and removed needs backport to 2.7 labels Sep 13, 2017

vstinner removed the needs backport to 2.7 label Sep 13, 2017

y-fujisaki2 added a commit to y-fujisaki2/zope.interface that referenced this pull request Sep 19, 2017

Fix process of reference count during GC

6ba212b

call PyObject_GC_UnTrack() in tp_dealloc() see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

y-fujisaki2 mentioned this pull request Sep 19, 2017

Fix process of reference count during GC zopefoundation/zope.interface#103

Merged

y-fujisaki2 added a commit to y-fujisaki2/zope.container that referenced this pull request Sep 21, 2017

Fix process of reference count during GC

40aa0a9

call PyObject_GC_UnTrack() in tp_dealloc() see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

y-fujisaki2 added a commit to y-fujisaki2/zope.i18nmessageid that referenced this pull request Sep 21, 2017

Fix process of reference count during GC

0faafd2

call PyObject_GC_UnTrack() in tp_dealloc() see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

y-fujisaki2 added a commit to y-fujisaki2/zope.i18nmessageid that referenced this pull request Sep 26, 2017

Fix process of reference count during GC

83c5b8f

call PyObject_GC_UnTrack() in tp_dealloc() see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

y-fujisaki2 added a commit to y-fujisaki2/zope.container that referenced this pull request Sep 26, 2017

Fix process of reference count during GC

5978d8b

call PyObject_GC_UnTrack() in tp_dealloc() see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

larryhastings pushed a commit that referenced this pull request Sep 26, 2017

bpo-31095 : fix potential crash during GC (GH-2974 ) (#3196 )

0fcc033

(cherry picked from commit a6296d3)

hannosch pushed a commit to zopefoundation/Acquisition that referenced this pull request Dec 1, 2017

call PyObject_GC_UnTrack() in tp_dealloc()

95a045d

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

hannosch pushed a commit to zopefoundation/Acquisition that referenced this pull request Dec 1, 2017

call PyObject_GC_UnTrack() in tp_dealloc()

ad02cb2

see the following sites for details: * https://bugs.python.org/issue31095 * python/cpython#2974

neonene mentioned this pull request Mar 12, 2024

gh-114314: ctypes: remove stgdict and switch to heap types #116458

Merged

bpo-31095: fix potential crash during GC. by methane · Pull Request #2974 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-31095: fix potential crash during GC. - #2974

bpo-31095: fix potential crash during GC.#2974
methane merged 4 commits into
python:masterfrom
methane:fix/31095

methane commented Aug 1, 2017 •
edited by bedevere-bot
Loading

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Uh oh!

serhiy-storchaka Aug 3, 2017

Uh oh!

serhiy-storchaka Aug 3, 2017

Uh oh!

serhiy-storchaka Aug 3, 2017

Uh oh!

benjaminp left a comment

Uh oh!

vstinner left a comment

Uh oh!

bedevere-bot commented Aug 22, 2017

Uh oh!

tiran left a comment

Uh oh!

bedevere-bot commented Aug 24, 2017

Uh oh!

miss-islington commented Sep 13, 2017

Uh oh!

miss-islington commented Sep 13, 2017

Uh oh!

vstinner commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

               lru_cache_dealloc(lru_cache_object *obj)
               {
+                  _PyObject_GC_UNTRACK(obj);
                   lru_list_elem *list = lru_cache_unlink_list(obj);

               static void
               ast_dealloc(AST_object *self)
               {
+                  _PyObject_GC_UNTRACK(self);

bpo-31095: fix potential crash during GC. by methane · Pull Request #2974 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-31095: fix potential crash during GC. - #2974

bpo-31095: fix potential crash during GC.#2974methane merged 4 commits intopython:masterpython/cpython:masterfrom methane:fix/31095methane/cpython:fix/31095Copy head branch name to clipboard

Conversation

methane commented Aug 1, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 22, 2017

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 24, 2017

Uh oh!

miss-islington commented Sep 13, 2017

Uh oh!

miss-islington commented Sep 13, 2017

Uh oh!

vstinner commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bpo-31095: fix potential crash during GC.#2974
methane merged 4 commits into
python:masterfrom
methane:fix/31095

methane commented Aug 1, 2017 •
edited by bedevere-bot
Loading