Skip to content
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

Jump to bottom

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

Conversation

methane
Copy link
Member

@methane 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.
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 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);
Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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);
Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

benjaminp
benjaminp approved these changes Aug 22, 2017
View changes
Copy link
Contributor

@benjaminp benjaminp left a comment

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

vstinner
vstinner previously requested changes Aug 22, 2017
View changes
Copy link
Member

@vstinner vstinner left a comment

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

@bedevere-bot
Copy link

@bedevere-bot 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 tiran left a comment

SSL and elementtree changes LGTM

@methane methane dismissed vstinner’s stale review Aug 23, 2017

added required comment

@methane methane merged commit a6296d3 into python:master Aug 24, 2017
4 checks passed
@methane methane deleted the fix/31095 branch Aug 24, 2017
methane added a commit to methane/cpython that referenced this issue Aug 24, 2017
@methane
[3.6] bpo-31095: fix potential crash during GC (pythonGH-2974)
c4242b6 
(cherry picked from commit a6296d3)
@bedevere-bot
Copy link

@bedevere-bot 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 issue Aug 24, 2017
@methane
bpo-31095: fix potential crash during GC (pythonGH-2974)
2c49cbb 
(cherry picked from commit a6296d3)
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this issue Sep 10, 2017
@methane @GadgetSteve
bpo-31095: fix potential crash during GC (pythonGH-2974)
7ff9475
jamadden pushed a commit to zopefoundation/zope.security that referenced this issue Sep 11, 2017
@jamadden
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 jamadden mentioned this pull request Sep 11, 2017
Merged
stephan-hof pushed a commit to zopefoundation/Acquisition that referenced this issue 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 issue Sep 11, 2017
@stephan-hof
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 issue Sep 11, 2017
@stephan-hof
call PyObject_GC_UnTrack() in tp_dealloc()
11c621e 
    see the following sites for details:
    * https://bugs.python.org/issue31095
    * python/cpython#2974
@miss-islington
Copy link
Contributor

@miss-islington 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
Copy link
Contributor

@miss-islington 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
Copy link
Member

@vstinner 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 issue 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 issue 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 issue 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 issue 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 issue 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 pushed a commit that referenced this issue Sep 26, 2017
@methane @larryhastings
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 issue Dec 1, 2017
@hannosch
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 issue Dec 1, 2017
@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-bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@methane @bedevere-bot @miss-islington @vstinner @benjaminp @tiran @serhiy-storchaka @Mariatta @the-knights-who-say-ni