Skip to content

/cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Aug 24, 2017
Merged

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

merged 4 commits into from Aug 24, 2017
+60 −13

Conversation

@methane
Copy link
Member

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

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

https://bugs.python.org/issue31095
@methane
bpo-31095: fix potential crash during GC.
Loading status checks…
cbce046 
Some tp_dealloc methods of GC types didn't call PyObject_GC_UnTrack().
@methane methane requested a review from tiran as a code owner Aug 1, 2017
@methane methane mentioned this pull request Aug 1, 2017
Merged
@methane methane requested a review from serhiy-storchaka Aug 1, 2017
@serhiy-storchaka
serhiy-storchaka approved these changes Aug 3, 2017
View changes
Copy link
Member

serhiy-storchaka left a comment

LGTM (besides two details).

But this touches delicate things, and I think this PR needs a review of yet one core developer.
Modules/_functoolsmodule.c Outdated
@@ -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);

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Aug 3, 2017

Member

_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.
Modules/_functoolsmodule.c Outdated
@@ -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);

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Aug 3, 2017

Member

Declaration after code is C99 syntax. The code will need rewriting for backporting to 2.7.
Python/Python-ast.c Outdated
@@ -520,6 +520,7 @@ typedef struct {
static void
ast_dealloc(AST_object *self)
{
_PyObject_GC_UNTRACK(self);

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Aug 3, 2017

Member

The same as for lru_cache. _ast.AST is subclassable.
@methane
Replace _PyObject_GC_UNTRACK with PyObject_GC_UnTrack
Loading status checks…
0d576ca
@methane
bpo-31095: Document PyObject_GC_UnTrack requirement
Loading status checks…
4e97f02
@methane methane force-pushed the methane:fix/31095 branch from b2132f7 to 4e97f02 Aug 3, 2017
@benjaminp
benjaminp approved these changes Aug 22, 2017
View changes
Copy link
Contributor

benjaminp left a comment

lgtm. This suggests that we need a systematic way to deal with this problem, though.
@vstinner
vstinner reviewed Aug 22, 2017
View changes
Copy link
Member

vstinner left a comment

I suggest to add a comment explaining why an explicit Untrack is required.
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Aug 22, 2017

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
tiran approved these changes Aug 22, 2017
View changes
Copy link
Member

tiran left a comment

SSL and elementtree changes LGTM
@methane
Add note comments
Loading status checks…
1b2e354
@methane methane dismissed vstinner’s stale review Aug 23, 2017

added required comment

@vstinner
vstinner approved these changes Aug 23, 2017
View changes
@methane methane merged commit a6296d3 into python:master Aug 24, 2017
4 checks passed
4 checks passed
bedevere/issue-number Issue number 31095 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@methane methane deleted the methane:fix/31095 branch Aug 24, 2017
methane added a commit to methane/cpython that referenced this pull request Aug 24, 2017
@methane
[3.6] bpo-31095: fix potential crash during GC (pythonGH-2974)
c4242b6 
(cherry picked from commit a6296d3)
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Aug 24, 2017

GH-3195 is a backport of this pull request to the 3.6 branch.
methane added a commit to methane/cpython that referenced this pull request Aug 24, 2017
@methane
bpo-31095: fix potential crash during GC (pythonGH-2974)
2c49cbb 
(cherry picked from commit a6296d3)
GadgetSteve added a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
@methane @GadgetSteve
bpo-31095: fix potential crash during GC (pythonGH-2974)
7ff9475
jamadden added a commit to zopefoundation/zope.security that referenced this pull request Sep 11, 2017
KIMURA Chikahiro @jamadden
call PyObject_GC_UnTrack() in tp_dealloc()
Verified
This commit was signed with a verified signature.
jamadden Jason Madden
GPG key ID: 349F84431A08B99E Learn about signing commits
Loading status checks…
a11c5e2 
see the following sites for details:

* https://bugs.python.org/issue31095
* python/cpython#2974

Fixes #35.
@jamadden jamadden mentioned this pull request Sep 11, 2017
Merged
stephan-hof pushed a commit to zopefoundation/Acquisition that referenced this pull request Sep 11, 2017
y-fujisaki2 chi-kimura@xr.jp.nec.com
call PyObject_GC_UnTrack() in tp_dealloc()
Loading status checks…
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
@stephan-hof
call PyObject_GC_UnTrack() in tp_dealloc()
Loading status checks…
19ea668 
    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
KIMURA Chikahiro @stephan-hof
call PyObject_GC_UnTrack() in tp_dealloc()
Loading status checks…
11c621e 
    see the following sites for details:
    * https://bugs.python.org/issue31095
    * python/cpython#2974
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Sep 13, 2017

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

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Sep 13, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented Sep 13, 2017

The commit 4cde4bd is the backport to Python 2.7.
y-fujisaki2 added a commit to y-fujisaki2/zope.interface that referenced this pull request Sep 19, 2017
@y-fujisaki2
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 y-fujisaki2 mentioned this pull request Sep 19, 2017
Merged
y-fujisaki2 added a commit to y-fujisaki2/zope.container that referenced this pull request Sep 21, 2017
@y-fujisaki2
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
@y-fujisaki2
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
@y-fujisaki2
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
@y-fujisaki2
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 added a commit that referenced this pull request Sep 26, 2017
@methane @larryhastings
bpo-31095: fix potential crash during GC (GH-2974) (#3196)
Loading status checks…
0fcc033 
(cherry picked from commit a6296d3)
hannosch added a commit to zopefoundation/Acquisition that referenced this pull request Dec 1, 2017
KIMURA Chikahiro @hannosch
call PyObject_GC_UnTrack() in tp_dealloc()
Verified
This commit was signed with a verified signature.
hannosch Hanno Schlichting
GPG key ID: A5C3A8301A902174 Learn about signing commits
Loading status checks…
95a045d 
    see the following sites for details:
    * https://bugs.python.org/issue31095
    * python/cpython#2974
hannosch added a commit to zopefoundation/Acquisition that referenced this pull request Dec 1, 2017
KIMURA Chikahiro @hannosch
call PyObject_GC_UnTrack() in tp_dealloc()
ad02cb2 
    see the following sites for details:
    * https://bugs.python.org/issue31095
    * python/cpython#2974
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@benjaminp benjaminp

@tiran tiran

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
CLA signed type-bugfix
Projects
None yet
Milestone
No milestone
9 participants
@methane @bedevere-bot @miss-islington @vstinner @benjaminp @tiran @serhiy-storchaka @Mariatta @the-knights-who-say-ni
You can’t perform that action at this time.