Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

[methane]

BPO	31095
Nosy	@vstinner, @larryhastings, @methane, @berkerpeksag, @serhiy-storchaka, @thehesiod
PRs	bpo-31095: fix potential crash during GC. #2974 [3.6] bpo-31095: fix potential crash during GC #3195 [3.5] bpo-31095: fix potential crash during GC #3196 [2.7] bpo-31095: Fix potential crash during GC #3197
Files	lru_cache_segv.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-10-28.22:47:25.467>
created_at = <Date 2017-08-01.05:49:49.875>
labels = ['extension-modules', 'interpreter-core', '3.7', 'type-crash']
title = 'Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC'
updated_at = <Date 2017-10-28.22:47:25.465>
user = 'https://github.com/methane'

bugs.python.org fields:

activity = <Date 2017-10-28.22:47:25.465>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2017-10-28.22:47:25.467>
closer = 'berker.peksag'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2017-08-01.05:49:49.875>
creator = 'methane'
dependencies = []
files = ['47055']
hgrepos = []
issue_num = 31095
keywords = []
message_count = 27.0
messages = ['299600', '299601', '299604', '299605', '299608', '299611', '299612', '299613', '299614', '299615', '300621', '300668', '300669', '300683', '300712', '300714', '300715', '300728', '300772', '300776', '300778', '301203', '301204', '301208', '303080', '303113', '305169']
nosy_count = 6.0
nosy_names = ['vstinner', 'larry', 'methane', 'berker.peksag', 'serhiy.storchaka', 'thehesiod']
pr_nums = ['2974', '3195', '3196', '3197']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue31095'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[methane]

like #47215, most types with Py_TPFLAGS_HAVE_GC
should call PyObject_GC_UnTrack() at top of the tp_dealloc.

For example, I found lru_cache doesn't call it and I can produce
segmentation fault.

I'll check other types too.

[thehesiod]

should the base method which calls tp_dealloc do this? Maybe can kill all birds with one stone.

[methane]

# collection module

dequeiter_dealloc doesn't call Untrack(), but it's safe because it only frees deque
and deque_dealloc calls Untrack()

        dequeiter_dealloc(dequeiterobject *dio)
        {
            Py_XDECREF(dio->deque);

defdict_dealloc doesn't call Untrack(). And it can cause segfault:

        from collections import defaultdict
        import gc

        class Evil:
            def __del__(self):
                gc.collect()
            def __call__(self):
                return 42

        def main():
            d = defaultdict(Evil())

        for i in range(1000):
            print(i)
            main()

# _elementtree module

elementiter_dealloc() calls UnTrack(), but it seems too late?

        static void
        elementiter_dealloc(ElementIterObject *it)
        {
            Py_ssize_t i = it->parent_stack_used;
            it->parent_stack_used = 0;
            while (i--)
                Py_XDECREF(it->parent_stack[i].parent);
            PyMem_Free(it->parent_stack);

            Py_XDECREF(it->sought_tag);
            Py_XDECREF(it->root_element);

            PyObject_GC_UnTrack(it);
            PyObject_GC_Del(it);
        }

[methane]

dequeiter_dealloc doesn't call Untrack(), but it's safe because it only frees deque
and deque_dealloc calls Untrack()

It may be not true, while I don't have exploit yet.

[methane]

# _json module

scanner_dealloc()
encoder_dealloc()

# _struct module

unpackiter_dealloc

# _ssl module

context_dealloc()

# Objects/

setiter_dealloc
dictiter_dealloc
dictview_dealloc

# Parser/

ast_dealloc

[thehesiod]

I suggest any places that don't need the calls should have comments so that future reviewers know why.

[thehesiod]

actually another idea: could the PR for this also update https://docs.python.org/2/c-api/typeobj.html#c.PyTypeObject.tp_dealloc to mention about these macros and when they should be used? That, along with all the other locations correctly calling these macros, and having comments when they're not needed hopefully should prevent this from happening again.

[methane]

should the base method which calls tp_dealloc do this? Maybe can kill all birds with one stone.

It may be possible, but unclear.
Object finalization process is very complicated.

I agree that UnTrack object as soon as refcnt=0, and Track only when
resurrected seems clearer.
But changing such basic object system needs carefully designed by expert.

[methane]

Docs/extending/newtypes.rst and Docs/include/noddy3.c should be updated too.
But I'm not good English writer. I need help.

[thehesiod]

omg I just realized I need the default dict one too, great investigation work!

[vstinner]

"like #47215, most types with Py_TPFLAGS_HAVE_GC should call PyObject_GC_UnTrack() at top of the tp_dealloc."

Hum, I wasn't aware of that. Writing correctly code for the Python garbage collector is very complex :-/

Maybe it would help to have a short comment, maybe with a link to this issue, on each PyObject_GC_UnTrack().

At the first read, I saw the newly added PyObject_GC_UnTrack() calls as duplicate, and so useless. For example, PyObject_Del() already untracks the object, so it doesn't seem to be needed to explicitly call PyObject_GC_UnTrack() "just before".