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

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

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_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);
    }
}

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?

pythonGH-97592: Anticipate fut_callbacks being cleared by PyObject_Ri…

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.

@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?

pythonGH-97592: Anticipate fut_callbacks being cleared by PyObject_Ri…

@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.

GH-97592: Fix crash in C remove_done_callback due to evil code (#97660)

pythonGH-97592: Fix crash in C remove_done_callback due to evil code (p…

GH-97592: Fix crash in C remove_done_callback due to evil code (GH-97660

pythonGH-97592: Fix crash in C remove_done_callback due to evil code (p…

Navigation Menu

Repository navigation

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

Crash report

Error messages

Your environment

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Crash report

Error messages

Your environment

Activity

gvanrossum commented on Sep 27, 2022

CharlieZhao95 commented on Sep 28, 2022

gvanrossum commented on Sep 28, 2022

CharlieZhao95 commented on Sep 29, 2022

gvanrossum commented on Sep 29, 2022

gvanrossum commented on Sep 29, 2022

xiaxinmeng commented on Sep 30, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions