diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py index c95fce035cd548..2422520c97bb98 100644 --- a/Lib/asyncio/futures.py +++ b/Lib/asyncio/futures.py @@ -234,10 +234,11 @@ def remove_done_callback(self, fn): Returns the number of callbacks removed. """ + count = len(self._callbacks) filtered_callbacks = [(f, ctx) for (f, ctx) in self._callbacks if f != fn] - removed_count = len(self._callbacks) - len(filtered_callbacks) + removed_count = count - len(filtered_callbacks) if removed_count: self._callbacks[:] = filtered_callbacks return removed_count diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index c566b28adb2408..8cf3087e5b8213 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -929,6 +929,136 @@ def __eq__(self, other): fut.remove_done_callback(evil()) + # Sanity checks for callback tuples corruption and Use-After-Free. + # Special thanks to Nico-Posada for the original PoCs and ideas. + # See https://github.com/python/cpython/issues/125789. + + def test_schedule_callbacks_list_mutation_3(self): + raise NotImplementedError + + def _test_schedule_callbacks_list_mutation_3(self, exc_type, exc_text=None): + class evil: + def __eq__(self, other): + global mem + mem = other + return False + + cb_pad = lambda: ... + + fut = self._new_future() + fut.add_done_callback(cb_pad) # sets fut->fut_callback0 + fut.add_done_callback(evil()) # sets fut->fut_callbacks[0] + # Consume fut->fut_callback0 callback and set it to NULL before + # checking fut->fut_callbacks, thereby invoking evil.__eq__(). + removed = fut.remove_done_callback(cb_pad) + self.assertEqual(removed, 1) + self.assertIs(mem, cb_pad) + # Set the malicious callback tuple and trigger a type check on it + # by re-invoking evil.__eq__() through remove_done_callback(). + fut._callbacks[0] = complex(0, 0) + if exc_text is None: + self.assertRaises(exc_type, fut.remove_done_callback, evil()) + else: + self.assertRaisesRegex(exc_type, exc_text, + fut.remove_done_callback, evil()) + self.assertIs(mem, cb_pad) + + def _evil_call_soon_event_loop(self, evil_call_soon): + fake_event_loop = lambda: ... + fake_event_loop.call_soon = evil_call_soon + fake_event_loop.get_debug = lambda: False # suppress traceback + return fake_event_loop + + def test_evil_call_soon_list_mutation_1(self): + def evil_call_soon(*args, **kwargs): + fut._callbacks.clear() + + loop = self._evil_call_soon_event_loop(evil_call_soon) + with mock.patch.object(self, 'loop', loop): + fut = self._new_future() + self.assertIs(fut.get_loop(), loop) + + cb_pad = lambda: ... + fut.add_done_callback(cb_pad) + fut.add_done_callback(None) + fut.add_done_callback(None) + + removed = fut.remove_done_callback(cb_pad) + self.assertEqual(removed, 1) + self.assertEqual(len(fut._callbacks), 2) + fut.set_result("boom") + # When there are no more callbacks, the Python implementation + # returns an empty list but the C implementation returns None. + self.assertIn(fut._callbacks, (None, [])) + + def test_evil_call_soon_list_mutation_2(self): + raise NotImplementedError + + def _test_evil_call_soon_list_mutation_2(self, exc_type, exc_text=None): + def evil_call_soon(*args, **kwargs): + fut._callbacks[1] = complex(0, 0) + + loop = self._evil_call_soon_event_loop(evil_call_soon) + with mock.patch.object(self, 'loop', loop): + fut = self._new_future() + self.assertIs(fut.get_loop(), loop) + + cb_pad = lambda: ... + fut.add_done_callback(cb_pad) + fut.add_done_callback(None) + fut.add_done_callback(None) + removed = fut.remove_done_callback(cb_pad) + self.assertEqual(removed, 1) + self.assertEqual(len(fut._callbacks), 2) + # The evil 'call_soon' is executed by calling set_result(). + if exc_text is None: + self.assertRaises(exc_type, fut.set_result, "boom") + else: + self.assertRaisesRegex(exc_type, exc_text, fut.set_result, "boom") + + def test_use_after_free_fixed_1(self): + fut = self._new_future() + + class setup: + def __eq__(self, other): + return False + + # evil MUST be a subclass of setup for __eq__ to get called first + class evil(setup): + def __eq__(self, value): + fut._callbacks.clear() + return NotImplemented + + cb_pad = lambda: ... + fut.add_done_callback(cb_pad) # sets fut->fut_callback0 + fut.add_done_callback(setup()) # sets fut->fut_callbacks[0] + removed = fut.remove_done_callback(cb_pad) + self.assertEqual(removed, 1) + + # This triggers evil.__eq__(), thereby clearing fut->fut_callbacks + # but we will still hold a reference to fut->fut_callbacks[0] until + # it is no more needed. + removed = fut.remove_done_callback(evil()) + self.assertEqual(removed, 0) + + def test_use_after_free_fixed_2(self): + asserter = self + fut = self._new_future() + + class cb_pad: + def __eq__(self, other): + return True + + class evil(cb_pad): + def __eq__(self, other): + removed = fut.remove_done_callback(None) + asserter.assertEqual(removed, 1) + return NotImplemented + + fut.add_done_callback(cb_pad()) + removed = fut.remove_done_callback(evil()) + self.assertEqual(removed, 1) + @unittest.skipUnless(hasattr(futures, '_CFuture'), 'requires the C _asyncio module') @@ -938,6 +1068,14 @@ class CFutureDoneCallbackTests(BaseFutureDoneCallbackTests, def _new_future(self): return futures._CFuture(loop=self.loop) + def test_schedule_callbacks_list_mutation_3(self): + errmsg = 'corrupted callback tuple' + super()._test_schedule_callbacks_list_mutation_3(RuntimeError, errmsg) + + def test_evil_call_soon_list_mutation_2(self): + errmsg = 'corrupted callback tuple' + super()._test_evil_call_soon_list_mutation_2(RuntimeError, errmsg) + @unittest.skipUnless(hasattr(futures, '_CFuture'), 'requires the C _asyncio module') @@ -949,6 +1087,14 @@ class CSubFuture(futures._CFuture): pass return CSubFuture(loop=self.loop) + def test_schedule_callbacks_list_mutation_3(self): + errmsg = 'corrupted callback tuple' + super()._test_schedule_callbacks_list_mutation_3(RuntimeError, errmsg) + + def test_evil_call_soon_list_mutation_2(self): + errmsg = 'corrupted callback tuple' + super()._test_evil_call_soon_list_mutation_2(RuntimeError, errmsg) + class PyFutureDoneCallbackTests(BaseFutureDoneCallbackTests, test_utils.TestCase): @@ -956,6 +1102,16 @@ class PyFutureDoneCallbackTests(BaseFutureDoneCallbackTests, def _new_future(self): return futures._PyFuture(loop=self.loop) + def test_schedule_callbacks_list_mutation_3(self): + super()._test_schedule_callbacks_list_mutation_3(TypeError) + + def test_evil_call_soon_list_mutation_2(self): + # For this test, the Python implementation raises an IndexError + # because the attribute fut._callbacks is set to an empty list + # *before* invoking the callbacks, while the C implementation + # does not make a temporary copy of the list of callbacks. + super()._test_evil_call_soon_list_mutation_2(IndexError) + class BaseFutureInheritanceTests: diff --git a/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst b/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst new file mode 100644 index 00000000000000..077b7f0dba10fb --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst @@ -0,0 +1,3 @@ +Mitigate interpreter crashes and state corruption due to side-effects in +:meth:`asyncio.Future.remove_done_callback` or other callback scheduling +methods. Patch by Bénédikt Tran. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 0a769c46b87ac8..d76f8dd80fee76 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -441,8 +441,16 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) return 0; } - for (i = 0; i < len; i++) { + // Beware: An evil 'call_soon' could change fut_callbacks or its items + // (see https://github.com/python/cpython/issues/125789 for details). + for (i = 0; + fut->fut_callbacks != NULL && i < PyList_GET_SIZE(fut->fut_callbacks); + i++) { PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i); + if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) { + PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple"); + return -1; + } PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0); PyObject *ctx = PyTuple_GET_ITEM(cb_tup, 1); @@ -1017,7 +1025,13 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls, ENSURE_FUTURE_ALIVE(state, self) if (self->fut_callback0 != NULL) { - int cmp = PyObject_RichCompareBool(self->fut_callback0, fn, Py_EQ); + // Beware: An evil PyObject_RichCompareBool could change fut_callback0 + // (see https://github.com/python/cpython/issues/125789 for details) + // In addition, the reference to self->fut_callback0 may be cleared, + // so we need to temporarily hold it explicitly. + PyObject *fut_callback0 = Py_NewRef(self->fut_callback0); + int cmp = PyObject_RichCompareBool(fut_callback0, fn, Py_EQ); + Py_DECREF(fut_callback0); if (cmp == -1) { return NULL; } @@ -1041,8 +1055,17 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls, if (len == 1) { PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, 0); - int cmp = PyObject_RichCompareBool( - PyTuple_GET_ITEM(cb_tup, 0), fn, Py_EQ); + // Beware: An evil PyObject_RichCompareBool could change fut_callbacks + // or its items (see https://github.com/python/cpython/issues/97592 or + // https://github.com/python/cpython/issues/125789 for details). + if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) { + PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple"); + return NULL; + } + Py_INCREF(cb_tup); + PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0); + int cmp = PyObject_RichCompareBool(cb, fn, Py_EQ); + Py_DECREF(cb_tup); if (cmp == -1) { return NULL; } @@ -1060,24 +1083,29 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls, return NULL; } - // Beware: PyObject_RichCompareBool below may change fut_callbacks. - // See GH-97592. + // Beware: An evil PyObject_RichCompareBool could change fut_callbacks + // or its items (see https://github.com/python/cpython/issues/97592 or + // https://github.com/python/cpython/issues/125789 for details). for (i = 0; self->fut_callbacks != NULL && 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); + PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i); + if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) { + PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple"); + goto fail; + } + Py_INCREF(cb_tup); + PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0); + int ret = PyObject_RichCompareBool(cb, fn, Py_EQ); if (ret == 0) { if (j < len) { - PyList_SET_ITEM(newlist, j, item); + PyList_SET_ITEM(newlist, j, cb_tup); j++; continue; } - ret = PyList_Append(newlist, item); + ret = PyList_Append(newlist, cb_tup); } - Py_DECREF(item); + Py_DECREF(cb_tup); if (ret < 0) { goto fail; }