From c5b99f5c2c5347d66b9da362773969c531fb6c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Fri, 25 Oct 2024 20:15:09 +0200 Subject: [PATCH] gh-125969: fix OOB in `future_schedule_callbacks` due to an evil `call_soon` (#125970) Co-authored-by: Andrew Svetlov --- Lib/test/test_asyncio/test_futures.py | 33 +++++++++++++++++++ ...-10-25-11-13-24.gh-issue-125969.YvbrTr.rst | 2 ++ Modules/_asynciomodule.c | 29 +++++++--------- 3 files changed, 47 insertions(+), 17 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index b3517407e53..4e3efa7ad6a 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -947,6 +947,39 @@ class BaseFutureDoneCallbackTests(): fut.remove_done_callback(evil()) + def test_evil_call_soon_list_mutation(self): + called_on_fut_callback0 = False + + pad = lambda: ... + + def evil_call_soon(*args, **kwargs): + nonlocal called_on_fut_callback0 + if called_on_fut_callback0: + # Called when handling fut->fut_callbacks[0] + # and mutates the length fut->fut_callbacks. + fut.remove_done_callback(int) + fut.remove_done_callback(pad) + else: + called_on_fut_callback0 = True + + fake_event_loop = lambda: ... + fake_event_loop.call_soon = evil_call_soon + fake_event_loop.get_debug = lambda: False # suppress traceback + + with mock.patch.object(self, 'loop', fake_event_loop): + fut = self._new_future() + self.assertIs(fut.get_loop(), fake_event_loop) + + fut.add_done_callback(str) # sets fut->fut_callback0 + fut.add_done_callback(int) # sets fut->fut_callbacks[0] + fut.add_done_callback(pad) # sets fut->fut_callbacks[1] + fut.add_done_callback(pad) # sets fut->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, [])) + @unittest.skipUnless(hasattr(futures, '_CFuture'), 'requires the C _asyncio module') diff --git a/Misc/NEWS.d/next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst b/Misc/NEWS.d/next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst new file mode 100644 index 00000000000..dc99adff741 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst @@ -0,0 +1,2 @@ +Fix an out-of-bounds crash when an evil :meth:`asyncio.loop.call_soon` +mutates the length of the internal callbacks list. Patch by Bénédikt Tran. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index a7385b4c541..91ba5cea880 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -406,9 +406,6 @@ future_ensure_alive(FutureObj *fut) static int future_schedule_callbacks(asyncio_state *state, FutureObj *fut) { - Py_ssize_t len; - Py_ssize_t i; - if (fut->fut_callback0 != NULL) { /* There's a 1st callback */ @@ -434,27 +431,25 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) return 0; } - len = PyList_GET_SIZE(fut->fut_callbacks); - if (len == 0) { - /* The list of callbacks was empty; clear it and return. */ - Py_CLEAR(fut->fut_callbacks); - return 0; - } - - for (i = 0; i < len; i++) { - PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i); + // Beware: An evil call_soon could change fut->fut_callbacks. + // The idea is to transfer the ownership of the callbacks list + // so that external code is not able to mutate the list during + // the iteration. + PyObject *callbacks = fut->fut_callbacks; + fut->fut_callbacks = NULL; + Py_ssize_t n = PyList_GET_SIZE(callbacks); + for (Py_ssize_t i = 0; i < n; i++) { + assert(PyList_GET_SIZE(callbacks) == n); + PyObject *cb_tup = PyList_GET_ITEM(callbacks, i); PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0); PyObject *ctx = PyTuple_GET_ITEM(cb_tup, 1); if (call_soon(state, fut->fut_loop, cb, (PyObject *)fut, ctx)) { - /* If an error occurs in pure-Python implementation, - all callbacks are cleared. */ - Py_CLEAR(fut->fut_callbacks); + Py_DECREF(callbacks); return -1; } } - - Py_CLEAR(fut->fut_callbacks); + Py_DECREF(callbacks); return 0; }