Repository navigation
Bugs in asyncio.Future.remove_done_callback() cause segfault. #97592
Description
Activity
Yeah, I can still repro this on 3.11. It doesn't crash when the pure-Python asyncio implementation is used (
sys.modules['_asyncio'] = None) so this appears to be crashing in_asyncio.Someone needs to look into the code there and see what is happening.
I simply debugged with the following code:
import asyncio async def crash(): fut = asyncio.Future() fut.add_done_callback(str) for _ in range(10): fut.add_done_callback(id) class Evil: def __eq__(self, other): fut.remove_done_callback(other) fut.remove_done_callback(Evil()) asyncio.create_task(set_after(fut, 1, '... world')) print('hello ...') print(await fut) if __name__ == "__main__": asyncio.run(crash())
The core dump seems to happen in the
_asyncio_Future_remove_done_callbackfunction of_asynciomodule.c.
At some point, theself->fut_callbacksis set to null, which causes crashing. In my tests it seems to be related toPy_CLEAR(self->fut_callbacks);static PyObject * _asyncio_Future_remove_done_callback(FutureObj *self, PyObject *fn) { ... // The `self->fut_callbacks` here is a nullptr, which causes the core dump for (i = 0; i < PyList_GET_SIZE(self->fut_callbacks); i++) { int ret; PyObject *item = PyList_GET_ITEM(self->fut_callbacks, i); Py_INCREF(item); ret = PyObject_RichCompareBool(PyTuple_GET_ITEM(item, 0), fn, Py_EQ); if (ret == 0) { if (j < len) { PyList_SET_ITEM(newlist, j, item); j++; continue; } ret = PyList_Append(newlist, item); } Py_DECREF(item); if (ret < 0) { goto fail; } } if (j == 0) { Py_CLEAR(self->fut_callbacks); Py_DECREF(newlist); return PyLong_FromSsize_t(len + cleared_callback0); } }
Thanks, that's good research! The cause is now clear: the
PyObject_RichCompareBoolcall invokes theevilclass's__eq__method, which calls the same function and ends up clearingfut_callbacks.The fix would seem to require some check for whether
fut_callbacksis NULL before doing anything further to it. This seems a little tricky, we need to check in theforloop header, but also in the code (that you snipped) below that assignsnewlistto a slice offut_callbacks.Do you want to create the PR to get full credit?
Thanks for your guidance! I notice that you have created a commit, I wonder if it would be better to add a test case that can trigger the crash :)
Yeah, I didn't get to that yet. It should be a simple case of converting your test case to a unit test.
FWIW looking for a good place to add that test I noticed that there was a very similar test case already, fixing #73149. @xiaxinmeng did you derive your demo from that code?
FWIW looking for a good place to add that test I noticed that there was a very similar test case already, fixing #73149. @xiaxinmeng did you derive your demo from that code?
Thanks for your efforts on fixing this bug! Yes, this case is derived from that code. The original bug has been fixed and the original test case cannot crash Python anymore. So we mutate the dependency relations in the original code and found this one crashing Python again.
Metadata
Metadata
Assignees
Labels
Projects
- Status
Crash report
The following example triggers a segfault on the latest stable Python3.8.14. I think there might be a bug in asyncio.Future.remove_done_callback().
Error messages
Segmentation fault (core dumped)
Your environment