From 9f64043c687ee35e124e3856bdcf5e92083a2028 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 11:11:07 +0200 Subject: [PATCH 1/7] fix OOB in `future_schedule_callbacks` --- Lib/test/test_asyncio/test_futures.py | 32 +++++++++++++++++++++++++++ Modules/_asynciomodule.c | 25 +++++---------------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index c566b28adb2408..705460460a327b 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -929,6 +929,38 @@ def __eq__(self, other): 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/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 0a769c46b87ac8..08cbaed284db00 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -406,15 +406,11 @@ 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 */ - int ret = call_soon(state, - fut->fut_loop, fut->fut_callback0, - (PyObject *)fut, fut->fut_context0); + int ret = call_soon(state, fut->fut_loop, fut->fut_callback0, + (PyObject *)fut, fut->fut_context0); Py_CLEAR(fut->fut_callback0); Py_CLEAR(fut->fut_context0); @@ -429,19 +425,10 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) callbacks from the 'fut_callbacks' list. */ } - if (fut->fut_callbacks == NULL) { - /* No more callbacks, return. */ - 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++) { + // Beware: An evil call_soon could change fut->fut_callbacks. + for (Py_ssize_t i = 0; + fut->fut_callbacks != NULL && i < PyList_GET_SIZE(fut->fut_callbacks); + i++) { PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i); PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0); PyObject *ctx = PyTuple_GET_ITEM(cb_tup, 1); From aa628417d27e0a11a2aa96884f6f7a1d3495aa0b 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 11:13:29 +0200 Subject: [PATCH 2/7] blurb --- .../next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst 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 00000000000000..56b3760de969b0 --- /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.Future.call_soon` +mutates the length of the internal callbacks list. Patch by Bénédikt Tran. From d21016278344fff3e40e01cf73c30ab6145b4ff3 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 11:32:13 +0200 Subject: [PATCH 3/7] fixup rst? --- .../next/Library/2024-10-25-11-13-24.gh-issue-125969.YvbrTr.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 56b3760de969b0..dc99adff7416c5 100644 --- 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 @@ -1,2 +1,2 @@ -Fix an out-of-bounds crash when an evil :meth:`asyncio.Future.call_soon` +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. From 0dec70de4c6f5e2b466c3c6683cc6a62564899c1 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 12:18:33 +0200 Subject: [PATCH 4/7] fixup --- Lib/test/test_asyncio/test_futures.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index 705460460a327b..73c3ebeddc929d 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -962,6 +962,7 @@ def evil_call_soon(*args, **kwargs): # 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') class CFutureDoneCallbackTests(BaseFutureDoneCallbackTests, From db960525bc2a8f0e6fbac3baa7ad4ff1c462f4d0 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 14:11:13 +0200 Subject: [PATCH 5/7] revert cosmetic change --- Modules/_asynciomodule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 08cbaed284db00..2dd833d267b6ed 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -409,8 +409,9 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) if (fut->fut_callback0 != NULL) { /* There's a 1st callback */ - int ret = call_soon(state, fut->fut_loop, fut->fut_callback0, - (PyObject *)fut, fut->fut_context0); + int ret = call_soon(state, + fut->fut_loop, fut->fut_callback0, + (PyObject *)fut, fut->fut_context0); Py_CLEAR(fut->fut_callback0); Py_CLEAR(fut->fut_context0); From f332de9380a9a381bc2ddf50a8c0828ec2407850 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 14:32:00 +0200 Subject: [PATCH 6/7] Copy the callback list before consuming callbacks. --- Modules/_asynciomodule.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 2dd833d267b6ed..9b6fb0509c694f 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -426,23 +426,30 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) callbacks from the 'fut_callbacks' list. */ } + if (fut->fut_callbacks == NULL) { + /* No more callbacks, return. */ + return 0; + } + // Beware: An evil call_soon could change fut->fut_callbacks. - for (Py_ssize_t i = 0; - fut->fut_callbacks != NULL && i < PyList_GET_SIZE(fut->fut_callbacks); - i++) { - PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i); + // The idea is to store the callbacks list before clearing it + // on the future object. In particular, external code is not + // able to mutate the list during the iteration. + PyObject *callbacks = Py_NewRef(fut->fut_callbacks); + Py_ssize_t n = PyList_GET_SIZE(callbacks); + Py_CLEAR(fut->fut_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; } From 4197ed4162fcce39b398d245f71a2cf3de24feb9 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 17:00:17 +0200 Subject: [PATCH 7/7] Transfer ownership instead of incref/decref --- Modules/_asynciomodule.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 18dcb714add5ed..91ba5cea880a06 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -432,12 +432,12 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut) } // Beware: An evil call_soon could change fut->fut_callbacks. - // The idea is to store the callbacks list before clearing it - // on the future object. In particular, external code is not - // able to mutate the list during the iteration. - PyObject *callbacks = Py_NewRef(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); - Py_CLEAR(fut->fut_callbacks); for (Py_ssize_t i = 0; i < n; i++) { assert(PyList_GET_SIZE(callbacks) == n); PyObject *cb_tup = PyList_GET_ITEM(callbacks, i);