From 13844094609cf8265a2eed023e33c7002f3f530d Mon Sep 17 00:00:00 2001 From: Jun Komoda <45822440+junkmd@users.noreply.github.com> Date: Sat, 26 Oct 2024 01:31:35 +0900 Subject: [PATCH 1/2] gh-125783: Add tests to prevent regressions with the combination of `ctypes` and metaclasses. (GH-125881) --- .../test_ctypes/test_c_simple_type_meta.py | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 Lib/test/test_ctypes/test_c_simple_type_meta.py diff --git a/Lib/test/test_ctypes/test_c_simple_type_meta.py b/Lib/test/test_ctypes/test_c_simple_type_meta.py new file mode 100644 index 00000000000000..fa5144a3ca01bb --- /dev/null +++ b/Lib/test/test_ctypes/test_c_simple_type_meta.py @@ -0,0 +1,87 @@ +import unittest +import ctypes +from ctypes import POINTER, c_void_p + +from ._support import PyCSimpleType + + +class PyCSimpleTypeAsMetaclassTest(unittest.TestCase): + def tearDown(self): + # to not leak references, we must clean _pointer_type_cache + ctypes._reset_cache() + + def test_creating_pointer_in_dunder_new_1(self): + # Test metaclass whose instances are C types; when the type is + # created it automatically creates a pointer type for itself. + # The pointer type is also an instance of the metaclass. + # Such an implementation is used in `IUnknown` of the `comtypes` + # project. See gh-124520. + + class ct_meta(type): + def __new__(cls, name, bases, namespace): + self = super().__new__(cls, name, bases, namespace) + + # Avoid recursion: don't set up a pointer to + # a pointer (to a pointer...) + if bases == (c_void_p,): + # When creating PtrBase itself, the name + # is not yet available + return self + if issubclass(self, PtrBase): + return self + + if bases == (object,): + ptr_bases = (self, PtrBase) + else: + ptr_bases = (self, POINTER(bases[0])) + p = p_meta(f"POINTER({self.__name__})", ptr_bases, {}) + ctypes._pointer_type_cache[self] = p + return self + + class p_meta(PyCSimpleType, ct_meta): + pass + + class PtrBase(c_void_p, metaclass=p_meta): + pass + + class CtBase(object, metaclass=ct_meta): + pass + + class Sub(CtBase): + pass + + class Sub2(Sub): + pass + + self.assertIsInstance(POINTER(Sub2), p_meta) + self.assertTrue(issubclass(POINTER(Sub2), Sub2)) + self.assertTrue(issubclass(POINTER(Sub2), POINTER(Sub))) + self.assertTrue(issubclass(POINTER(Sub), POINTER(CtBase))) + + def test_creating_pointer_in_dunder_new_2(self): + # A simpler variant of the above, used in `CoClass` of the `comtypes` + # project. + + class ct_meta(type): + def __new__(cls, name, bases, namespace): + self = super().__new__(cls, name, bases, namespace) + if isinstance(self, p_meta): + return self + p = p_meta(f"POINTER({self.__name__})", (self, c_void_p), {}) + ctypes._pointer_type_cache[self] = p + return self + + class p_meta(PyCSimpleType, ct_meta): + pass + + class Core(object): + pass + + class CtBase(Core, metaclass=ct_meta): + pass + + class Sub(CtBase): + pass + + self.assertIsInstance(POINTER(Sub), p_meta) + self.assertTrue(issubclass(POINTER(Sub), Sub)) 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 2/2] 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 b3517407e5394f..4e3efa7ad6abc0 100644 --- a/Lib/test/test_asyncio/test_futures.py +++ b/Lib/test/test_asyncio/test_futures.py @@ -947,6 +947,39 @@ 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/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..dc99adff7416c5 --- /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 a7385b4c541df9..91ba5cea880a06 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; }