gh-125969: fix OOB in future_schedule_callbacks due to an evil call_soon (#125970)

picnixz · asvetlov · web-flow · commit c5b99f5c2c53 · 2024-10-25T23:45:09.000+05:30
Co-authored-by: Andrew Svetlov &lt;andrew.svetlov@gmail.com&gt;
diff --git 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
@@ -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
@@ -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;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+Fix an out-of-bounds crash when an evil :meth:`asyncio.loop.call_soon`
	`2`	`+mutates the length of the internal callbacks list. Patch by Bénédikt Tran.`