From 63f1957cedd85d80067bfe2cb917b75cf06bd080 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Thu, 24 Oct 2024 13:22:04 +0000 Subject: [PATCH 1/2] fix callbacks to copy it always --- Lib/test/test_asyncio/test_futures.py | 16 ++++++ ...-10-24-14-08-10.gh-issue-125789.eaiAMw.rst | 1 + Modules/_asynciomodule.c | 53 +++++++++---------- 3 files changed, 42 insertions(+), 28 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-24-14-08-10.gh-issue-125789.eaiAMw.rst diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index c566b28adb2408..af31fd5f2c6fe6 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -697,6 +697,22 @@ def test_future_del_segfault(self): with self.assertRaises(AttributeError): del fut._log_traceback + def test_callbacks_copy(self): + # See https://github.com/python/cpython/issues/125789 + # In C implementation, the `_callbacks` attribute + # always returns a new list to avoid mutations of internal state + + fut = self._new_future(loop=self.loop) + f1 = lambda _: 1 + f2 = lambda _: 2 + fut.add_done_callback(f1) + fut.add_done_callback(f2) + callbacks = fut._callbacks + self.assertIsNot(callbacks, fut._callbacks) + fut.remove_done_callback(f1) + callbacks = fut._callbacks + self.assertIsNot(callbacks, fut._callbacks) + @unittest.skipUnless(hasattr(futures, '_CFuture'), 'requires the C _asyncio module') diff --git a/Misc/NEWS.d/next/Library/2024-10-24-14-08-10.gh-issue-125789.eaiAMw.rst b/Misc/NEWS.d/next/Library/2024-10-24-14-08-10.gh-issue-125789.eaiAMw.rst new file mode 100644 index 00000000000000..964a006bb47b7b --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-24-14-08-10.gh-issue-125789.eaiAMw.rst @@ -0,0 +1 @@ +Fix possible crash when mutating list of callbacks returned by :attr:`!asyncio.Future._callbacks`. It now always returns a new copy in C implementation :mod:`!_asyncio`. Patch by Kumar Aditya. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 0a769c46b87ac8..3c358aee8b1752 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1265,52 +1265,49 @@ static PyObject * FutureObj_get_callbacks(FutureObj *fut, void *Py_UNUSED(ignored)) { asyncio_state *state = get_asyncio_state_by_def((PyObject *)fut); - Py_ssize_t i; - ENSURE_FUTURE_ALIVE(state, fut) - if (fut->fut_callback0 == NULL) { - if (fut->fut_callbacks == NULL) { - Py_RETURN_NONE; - } - - return Py_NewRef(fut->fut_callbacks); + Py_ssize_t len = 0; + if (fut->fut_callback0 != NULL) { + len++; } - - Py_ssize_t len = 1; if (fut->fut_callbacks != NULL) { len += PyList_GET_SIZE(fut->fut_callbacks); } - - PyObject *new_list = PyList_New(len); - if (new_list == NULL) { - return NULL; + if (len == 0) { + Py_RETURN_NONE; } - PyObject *tup0 = PyTuple_New(2); - if (tup0 == NULL) { - Py_DECREF(new_list); + PyObject *callbacks = PyList_New(len); + if (callbacks == NULL) { return NULL; } - Py_INCREF(fut->fut_callback0); - PyTuple_SET_ITEM(tup0, 0, fut->fut_callback0); - assert(fut->fut_context0 != NULL); - Py_INCREF(fut->fut_context0); - PyTuple_SET_ITEM(tup0, 1, (PyObject *)fut->fut_context0); - - PyList_SET_ITEM(new_list, 0, tup0); + Py_ssize_t i = 0; + if (fut->fut_callback0 != NULL) { + PyObject *tup0 = PyTuple_New(2); + if (tup0 == NULL) { + Py_DECREF(callbacks); + return NULL; + } + PyTuple_SET_ITEM(tup0, 0, Py_NewRef(fut->fut_callback0)); + assert(fut->fut_context0 != NULL); + PyTuple_SET_ITEM(tup0, 1, Py_NewRef(fut->fut_context0)); + PyList_SET_ITEM(callbacks, i, tup0); + i++; + } if (fut->fut_callbacks != NULL) { - for (i = 0; i < PyList_GET_SIZE(fut->fut_callbacks); i++) { - PyObject *cb = PyList_GET_ITEM(fut->fut_callbacks, i); + for (int j = 0; j < PyList_GET_SIZE(fut->fut_callbacks); j++) { + PyObject *cb = PyList_GET_ITEM(fut->fut_callbacks, j); Py_INCREF(cb); - PyList_SET_ITEM(new_list, i + 1, cb); + PyList_SET_ITEM(callbacks, i, cb); + i++; } } - return new_list; + return callbacks; } static PyObject * From 299ba2677630ba033c0f57f2d3e5fa8d5a2ed735 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Fri, 25 Oct 2024 17:48:48 +0530 Subject: [PATCH 2/2] address review --- Lib/test/test_asyncio/test_futures.py | 2 ++ Modules/_asynciomodule.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py index af31fd5f2c6fe6..b3517407e5394f 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -712,6 +712,8 @@ def test_callbacks_copy(self): fut.remove_done_callback(f1) callbacks = fut._callbacks self.assertIsNot(callbacks, fut._callbacks) + fut.remove_done_callback(f2) + self.assertIsNone(fut._callbacks) @unittest.skipUnless(hasattr(futures, '_CFuture'), diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 3c358aee8b1752..a7385b4c541df9 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -1299,7 +1299,7 @@ FutureObj_get_callbacks(FutureObj *fut, void *Py_UNUSED(ignored)) } if (fut->fut_callbacks != NULL) { - for (int j = 0; j < PyList_GET_SIZE(fut->fut_callbacks); j++) { + for (Py_ssize_t j = 0; j < PyList_GET_SIZE(fut->fut_callbacks); j++) { PyObject *cb = PyList_GET_ITEM(fut->fut_callbacks, j); Py_INCREF(cb); PyList_SET_ITEM(callbacks, i, cb);