Skip to content

bpo-31061: fix crash in asyncio speedup module #2966

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Aug 2, 2017
Merged

bpo-31061: fix crash in asyncio speedup module #2966

Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
336a6ce
attempt to fix http://bugs.python.org/issue31061
thehesiod Aug 1, 2017
4fd34a6
fix typo
thehesiod Aug 1, 2017
97ccf31
fix capitalization
thehesiod Aug 1, 2017
a67847e
switch to private version
thehesiod Aug 1, 2017
e126945
undo :)
thehesiod Aug 1, 2017
de3ee01
add missing untrack
thehesiod Aug 1, 2017
6a02050
revert change
thehesiod Aug 1, 2017
503806f
add news item
thehesiod Aug 1, 2017
f900058
simplify patch based on review
thehesiod Aug 1, 2017
483586b
remove
thehesiod Aug 1, 2017
b4589ce
add blurb
thehesiod Aug 1, 2017
fa2b42b
remove space
thehesiod Aug 1, 2017
3ca521d
revert
thehesiod Aug 1, 2017
b0fb8e7
new task test case
thehesiod Aug 1, 2017
f32e949
fix unittest
thehesiod Aug 1, 2017
7bc0c2a
set the loop
thehesiod Aug 1, 2017
55a2eb5
fix off by one paren error :)
thehesiod Aug 1, 2017
3033b15
fix ordering
thehesiod Aug 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,14 +962,18 @@ FutureObj_dealloc(PyObject *self)
{
FutureObj *fut = (FutureObj *)self;

PyObject_GC_UnTrack(self);

if (Future_CheckExact(fut)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please move PyObject_GC_UnTrack after this if block, and remove Track and Untrack in the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but that would mean this (34fae03) is wrong too then no?

because it first does an UnTrack: 34fae03#diff-c3cf251f16d5a03a9e7d4639f2d6f998L1113

With the same Track/Untrack pattern here:
34fae03#diff-c3cf251f16d5a03a9e7d4639f2d6f998R1129

Copy link
Member

Choose a reason for hiding this comment

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

current code:

UnTrack()
Track()
PyObject_CallFinalizerFromDealloc()
UnTrack()

I proposed:

PyObject_CallFinalizerFromDealloc()
UnTrack()

34fae03 does other things between UnTrack() and Track()
But this code does only Future_CheckExact() and it is safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, I'm claiming ignorance since I don't know what these macros achieve :) I'd personally feel safer if we had a test case, do you know how to create a testcase to cause this to crash? Mine takes a day to reproduce in a production environment :(

Copy link
Member

Choose a reason for hiding this comment

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

CPython's circular reference GC uses doubly linked list to track object which can be member of circular reference.
PyObject_GC_UnTrack() removes object from the list, and PyObject_GC_Track() insert the object to the list.

Despite very undetarministic multithreading, I can cause SEGV quickly with this code:

import asyncio
import gc
gc.set_debug(gc.DEBUG_STATS)

class Evil:
    def __del__(self):
        gc.collect()

def main():
    f = asyncio.Future()
    f.set_result(Evil())

for i in range(100):
    main()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice! I'll add testcases and add your suggestions, thanks so much for all the help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, added FutureObj testcase, have a good test case for the TaskObj? Not quite sure how to trigger it.

/* When fut is subclass of Future, finalizer is called from
* subtype_dealloc.
*/
PyObject_GC_Track(self);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, you're right. it's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just following what was done in https://bugs.python.org/issue26617. I think it's worth someone grepping through the whole codebase, I saw several other places that maybe needed this. It seems like every dealloc should have a PyObject_GC_UnTrack at the top?

Copy link
Member

@methane methane Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe, noddy4 example should do it at first :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw how do we get this into 3.6.3 as well?

Copy link
Member

Choose a reason for hiding this comment

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

It seems like every dealloc should have a PyObject_GC_UnTrack at the top?

When the type has Py_TPFLAGS_HAVE_GC.
lru_cache_dealloc in _functoolsmodule seems need it.

Copy link
Member

Choose a reason for hiding this comment

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

I filed an issue for that. http://bugs.python.org/issue31095

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, should I remove the LRU change I added here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please. asyncio speedup module is introduced from Python 3.6.
It make easy to backport.

if (PyObject_CallFinalizerFromDealloc(self) < 0) {
// resurrected.
return;
}
PyObject_GC_UnTrack(self);
}

if (fut->fut_weakreflist != NULL) {
Expand Down Expand Up @@ -1836,14 +1840,18 @@ TaskObj_dealloc(PyObject *self)
{
TaskObj *task = (TaskObj *)self;

PyObject_GC_UnTrack(self);

if (Task_CheckExact(self)) {
/* When fut is subclass of Task, finalizer is called from
* subtype_dealloc.
*/
PyObject_GC_Track(self);
if (PyObject_CallFinalizerFromDealloc(self) < 0) {
// resurrected.
return;
}
PyObject_GC_UnTrack(self);
}

if (task->task_weakreflist != NULL) {
Expand Down