Bugs in asyncio.Future.remove_done_callback() cause segfault.

[xiaxinmeng]

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

[gvanrossum]

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]

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]

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]

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]

Yeah, I didn't get to that yet. It should be a simple case of converting your test case to a unit test.

[gvanrossum]

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?

[xiaxinmeng]

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.