GeezerVison is showing github.com — Bugs in asyncio.Future.remove_done_callback() cause segfault. · Issue #97592 · python/cpython
Bugs in asyncio.Future.remove_done_callback() cause segfault. #97592Description
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().
import asyncio
fut = asyncio.Future()
fut.add_done_callback(str)
for str in range(63):
fut.add_done_callback(id)
class evil:
def __eq__(self, other):
fut.remove_done_callback(other)
fut.remove_done_callback(evil())
Error messages
Segmentation fault (core dumped)
Your environment
- CPython versions tested on: Python 3.8.14, Python 3.9.0, main branch 68c46ae
- Operating system and architecture: [GCC 7.5.0] on linux
Activitygvanrossum commented on Sep 27, 2022 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. CharlieZhao95 commented on Sep 28, 2022 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_callback function of _asynciomodule.c.
At some point, the self->fut_callbacks is set to null, which causes crashing. In my tests it seems to be related to Py_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);
}
}gvanrossum commented on Sep 28, 2022 Thanks, that's good research! The cause is now clear: the PyObject_RichCompareBool call invokes the evil class's __eq__ method, which calls the same function and ends up clearing fut_callbacks.
The fix would seem to require some check for whether fut_callbacks is NULL before doing anything further to it. This seems a little tricky, we need to check in the for loop header, but also in the code (that you snipped) below that assigns newlist to a slice of fut_callbacks.
Do you want to create the PR to get full credit? CharlieZhao95 commented on Sep 29, 2022 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 :) gvanrossum commented on Sep 29, 2022 Yeah, I didn't get to that yet. It should be a simple case of converting your test case to a unit test. gvanrossum commented on Sep 29, 2022 xiaxinmeng commented on Sep 30, 2022
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.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|