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

methane · 2017-08-01T09:52:29Z

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

serhiy-storchaka

LGTM (besides two details).

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

serhiy-storchaka · 2017-08-03T15:10:34Z

Modules/_functoolsmodule.c

@@ -1073,6 +1073,7 @@ lru_cache_clear_list(lru_list_elem *link)
 static void
 lru_cache_dealloc(lru_cache_object *obj)
 {
+    _PyObject_GC_UNTRACK(obj);


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

serhiy-storchaka · 2017-08-03T15:12:30Z

Modules/_functoolsmodule.c

@@ -1073,6 +1073,7 @@ lru_cache_clear_list(lru_list_elem *link)
 static void
 lru_cache_dealloc(lru_cache_object *obj)
 {
+    _PyObject_GC_UNTRACK(obj);
    lru_list_elem *list = lru_cache_unlink_list(obj);


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

serhiy-storchaka · 2017-08-03T15:15:47Z

Python/Python-ast.c

@@ -520,6 +520,7 @@ typedef struct {
 static void
 ast_dealloc(AST_object *self)
 {
+    _PyObject_GC_UNTRACK(self);


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

benjaminp

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

vstinner

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

bedevere-bot · 2017-08-22T09:44:19Z

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.

tiran

SSL and elementtree changes LGTM

added required comment

(cherry picked from commit a6296d3)

bedevere-bot · 2017-08-24T06:03:09Z

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

miss-islington · 2017-09-13T17:22:18Z

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!

miss-islington · 2017-09-13T17:22:21Z

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

vstinner · 2017-09-13T22:19:25Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

methane commented Aug 1, 2017 •

edited by bedevere-bot

Loading

serhiy-storchaka left a comment

serhiy-storchaka Aug 3, 2017

serhiy-storchaka Aug 3, 2017

serhiy-storchaka Aug 3, 2017

benjaminp left a comment

vstinner left a comment

bedevere-bot commented Aug 22, 2017

tiran left a comment

bedevere-bot commented Aug 24, 2017

miss-islington commented Sep 13, 2017

miss-islington commented Sep 13, 2017

vstinner commented Sep 13, 2017

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

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

Conversation

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

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

benjaminp left a comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 22, 2017

tiran left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 24, 2017

miss-islington commented Sep 13, 2017

miss-islington commented Sep 13, 2017

vstinner commented Sep 13, 2017

methane commented Aug 1, 2017 •

edited by bedevere-bot

Loading