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

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

 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

 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

 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#2974Fixes#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 4e97f02Compare 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:masterAug 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#2974Fixes#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

Navigation Menu

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!

serhiy-storchaka left a comment

Uh oh!

serhiy-storchakaAug 3, 2017

Uh oh!

serhiy-storchakaAug 3, 2017

Uh oh!

serhiy-storchakaAug 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

Uh oh!

Uh oh!

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-storchakaAug 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaAug 3, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaAug 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

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