gh-125984: fix use-after-free on `fut->fut_{callback,context}0` due to an evil `loop...

diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py
index 4e3efa7ad6abc039d63e86c3e74b822a0d398905..bac76b074981dfffdb56495d5ae654148ab5e8fa 100644 (file)
--- a/Lib/test/test_asyncio/test_futures.py
+++ b/Lib/test/test_asyncio/test_futures.py
@@ -32,6 +32,25 @@ def last_cb():
     pass
 
 
+class ReachableCode(Exception):
+    """Exception to raise to indicate that some code was reached.
+
+    Use this exception if using mocks is not a good alternative.
+    """
+
+
+class SimpleEvilEventLoop(asyncio.base_events.BaseEventLoop):
+    """Base class for UAF and other evil stuff requiring an evil event loop."""
+
+    def get_debug(self):  # to suppress tracebacks
+        return False
+
+    def __del__(self):
+        # Automatically close the evil event loop to avoid warnings.
+        if not self.is_closed() and not self.is_running():
+            self.close()
+
+
 class DuckFuture:
     # Class that does not inherit from Future but aims to be duck-type
     # compatible with it.
@@ -948,6 +967,7 @@ class BaseFutureDoneCallbackTests():
         fut.remove_done_callback(evil())
 
     def test_evil_call_soon_list_mutation(self):
+        # see: https://github.com/python/cpython/issues/125969
         called_on_fut_callback0 = False
 
         pad = lambda: ...
@@ -962,9 +982,8 @@ class BaseFutureDoneCallbackTests():
             else:
                 called_on_fut_callback0 = True
 
-        fake_event_loop = lambda: ...
+        fake_event_loop = SimpleEvilEventLoop()
         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()
@@ -980,6 +999,56 @@ class BaseFutureDoneCallbackTests():
             # returns an empty list but the C implementation returns None.
             self.assertIn(fut._callbacks, (None, []))
 
+    def test_use_after_free_on_fut_callback_0_with_evil__getattribute__(self):
+        # see: https://github.com/python/cpython/issues/125984
+
+        class EvilEventLoop(SimpleEvilEventLoop):
+            def call_soon(self, *args, **kwargs):
+                super().call_soon(*args, **kwargs)
+                raise ReachableCode
+
+            def __getattribute__(self, name):
+                nonlocal fut_callback_0
+                if name == 'call_soon':
+                    fut.remove_done_callback(fut_callback_0)
+                    del fut_callback_0
+                return object.__getattribute__(self, name)
+
+        evil_loop = EvilEventLoop()
+        with mock.patch.object(self, 'loop', evil_loop):
+            fut = self._new_future()
+            self.assertIs(fut.get_loop(), evil_loop)
+
+            fut_callback_0 = lambda: ...
+            fut.add_done_callback(fut_callback_0)
+            self.assertRaises(ReachableCode, fut.set_result, "boom")
+
+    def test_use_after_free_on_fut_context_0_with_evil__getattribute__(self):
+        # see: https://github.com/python/cpython/issues/125984
+
+        class EvilEventLoop(SimpleEvilEventLoop):
+            def call_soon(self, *args, **kwargs):
+                super().call_soon(*args, **kwargs)
+                raise ReachableCode
+
+            def __getattribute__(self, name):
+                if name == 'call_soon':
+                    # resets the future's event loop
+                    fut.__init__(loop=SimpleEvilEventLoop())
+                return object.__getattribute__(self, name)
+
+        evil_loop = EvilEventLoop()
+        with mock.patch.object(self, 'loop', evil_loop):
+            fut = self._new_future()
+            self.assertIs(fut.get_loop(), evil_loop)
+
+            fut_callback_0 = mock.Mock()
+            fut_context_0 = mock.Mock()
+            fut.add_done_callback(fut_callback_0, context=fut_context_0)
+            del fut_context_0
+            del fut_callback_0
+            self.assertRaises(ReachableCode, fut.set_result, "boom")
+
 
 @unittest.skipUnless(hasattr(futures, '_CFuture'),
                      'requires the C _asyncio module')

diff --git a/Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst b/Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst
new file mode 100644 (file)
index 0000000..7a1d7b5
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2024-10-26-12-50-48.gh-issue-125984.d4vp5_.rst
@@ -0,0 +1,3 @@
+Fix use-after-free crashes on :class:`asyncio.Future` objects for which the
+underlying event loop implements an evil :meth:`~object.__getattribute__`.
+Reported by Nico-Posada. Patch by Bénédikt Tran.

diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index 91ba5cea880a06495df4f91866f7bc8ed8f37e07..a4dab29ee9428d37e1d6c6d6913d2f84dceacaf3 100644 (file)
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -409,12 +409,19 @@ 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);
-
-        Py_CLEAR(fut->fut_callback0);
-        Py_CLEAR(fut->fut_context0);
+        // Beware: An evil call_soon could alter fut_callback0 or fut_context0.
+        // Since we are anyway clearing them after the call, whether call_soon
+        // succeeds or not, the idea is to transfer ownership so that external
+        // code is not able to alter them during the call.
+        PyObject *fut_callback0 = fut->fut_callback0;
+        fut->fut_callback0 = NULL;
+        PyObject *fut_context0 = fut->fut_context0;
+        fut->fut_context0 = NULL;
+
+        int ret = call_soon(state, fut->fut_loop, fut_callback0,
+                            (PyObject *)fut, fut_context0);
+        Py_CLEAR(fut_callback0);
+        Py_CLEAR(fut_context0);
         if (ret) {
             /* If an error occurs in pure-Python implementation,
                all callbacks are cleared. */