From be7cac9df76ee13729799088257fe66f9868b119 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Tue, 22 Oct 2024 11:48:56 +0200
Subject: [PATCH 1/8] mitigate side-effects in futures scheduling

---
 Lib/test/test_asyncio/test_futures.py | 50 +++++++++++++++++++
 Modules/_asynciomodule.c              | 70 +++++++++++++++++++++------
 2 files changed, 105 insertions(+), 15 deletions(-)

diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py
index c566b28adb2408..cbd963e0e7b5a5 100644
--- a/Lib/test/test_asyncio/test_futures.py
+++ b/Lib/test/test_asyncio/test_futures.py
@@ -929,6 +929,47 @@ def __eq__(self, other):
 
         fut.remove_done_callback(evil())
 
+    def test_schedule_callbacks_list_mutation_3(self):
+        raise NotImplemented
+
+    def _test_schedule_callbacks_list_mutation_3(self, exc_type, exc_text=None):
+        # see https://github.com/python/cpython/issues/125789 for details
+        fut = self._new_future()
+
+        class evil:
+            def __eq__(self, other):
+                global mem
+                mem = other
+                return False
+
+        cb_pad = lambda: ...
+        fut.add_done_callback(cb_pad)  # sets fut->fut_callback0
+        fut.add_done_callback(evil())  # sets first item in fut->fut_callbacks
+        # Consume fut->fut_callback0 callback but checks the remaining callbacks,
+        # thereby invoking evil.__eq__().
+        fut.remove_done_callback(cb_pad)
+        self.assertIs(mem, cb_pad)
+
+        fake = (
+            (0).to_bytes(8, 'little') +
+            id(bytearray).to_bytes(8, 'little') +
+            (2 ** 63 - 1).to_bytes(8, 'little') +
+            (0).to_bytes(24, 'little')
+        )
+
+        i2f = lambda num: 5e-324 * num
+        fut._callbacks[0] = complex(0, i2f(id(fake) + bytes.__basicsize__ - 1))
+
+        # We want to call once again evil.__eq__() to set 'mem' to our
+        # malicious bytearray. However, since we manually modified the
+        # callbacks list, we will not be able to by-pass the checks.
+        if exc_text is None:
+            self.assertRaises(exc_type, fut.remove_done_callback, evil())
+        else:
+            self.assertRaisesRegex(exc_type, exc_text, fut.remove_done_callback, evil())
+
+        self.assertIs(mem, cb_pad)
+
 
 @unittest.skipUnless(hasattr(futures, '_CFuture'),
                      'requires the C _asyncio module')
@@ -938,6 +979,9 @@ class CFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
     def _new_future(self):
         return futures._CFuture(loop=self.loop)
 
+    def test_schedule_callbacks_list_mutation_3(self):
+        super()._test_schedule_callbacks_list_mutation_3(RuntimeError, 'corrupted')
+
 
 @unittest.skipUnless(hasattr(futures, '_CFuture'),
                      'requires the C _asyncio module')
@@ -949,6 +993,9 @@ class CSubFuture(futures._CFuture):
             pass
         return CSubFuture(loop=self.loop)
 
+    def test_schedule_callbacks_list_mutation_3(self):
+        super()._test_schedule_callbacks_list_mutation_3(RuntimeError, 'corrupted')
+
 
 class PyFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
                                 test_utils.TestCase):
@@ -956,6 +1003,9 @@ class PyFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
     def _new_future(self):
         return futures._PyFuture(loop=self.loop)
 
+    def test_schedule_callbacks_list_mutation_3(self):
+        super()._test_schedule_callbacks_list_mutation_3(TypeError)
+
 
 class BaseFutureInheritanceTests:
 
diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index 0a769c46b87ac8..e0c1e4fb7596f6 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -434,6 +434,11 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
         return 0;
     }
 
+    if (!PyList_Check(fut->fut_callbacks)) {
+        PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        return -1;
+    }
+
     len = PyList_GET_SIZE(fut->fut_callbacks);
     if (len == 0) {
         /* The list of callbacks was empty; clear it and return. */
@@ -441,8 +446,21 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
         return 0;
     }
 
-    for (i = 0; i < len; i++) {
+    // Beware: 'call_soon' below may change fut_callbacks or its items
+    // (see https://github.com/python/cpython/issues/125789 for details).
+    for (i = 0; fut->fut_callbacks != NULL; i++) {
+        if (!PyList_Check(fut->fut_callbacks)) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+            return -1;
+        }
+        if (i >= PyList_GET_SIZE(fut->fut_callbacks)) {
+            break; // done
+        }
         PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
+        if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 2) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+            return -1;
+        }
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
         PyObject *ctx = PyTuple_GET_ITEM(cb_tup, 1);
 
@@ -1033,6 +1051,11 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         return PyLong_FromSsize_t(cleared_callback0);
     }
 
+    if (!PyList_Check(self->fut_callbacks)) {
+        PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        return NULL;
+    }
+
     len = PyList_GET_SIZE(self->fut_callbacks);
     if (len == 0) {
         Py_CLEAR(self->fut_callbacks);
@@ -1041,8 +1064,15 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
 
     if (len == 1) {
         PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, 0);
-        int cmp = PyObject_RichCompareBool(
-            PyTuple_GET_ITEM(cb_tup, 0), fn, Py_EQ);
+        // Beware: PyObject_RichCompareBool below may change fut_callbacks or
+        // its items (see https://github.com/python/cpython/issues/97592 and
+        // https://github.com/python/cpython/issues/125789 for details).
+        if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+            return NULL;
+        }
+        PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
+        int cmp = PyObject_RichCompareBool(cb, fn, Py_EQ);
         if (cmp == -1) {
             return NULL;
         }
@@ -1060,24 +1090,34 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         return NULL;
     }
 
-    // Beware: PyObject_RichCompareBool below may change fut_callbacks.
-    // See GH-97592.
-    for (i = 0;
-         self->fut_callbacks != NULL && i < PyList_GET_SIZE(self->fut_callbacks);
-         i++) {
-        int ret;
-        PyObject *item = PyList_GET_ITEM(self->fut_callbacks, i);
-        Py_INCREF(item);
-        ret = PyObject_RichCompareBool(PyTuple_GET_ITEM(item, 0), fn, Py_EQ);
+    // Beware: PyObject_RichCompareBool below may change fut_callbacks or
+    // its items (see https://github.com/python/cpython/issues/97592 and
+    // https://github.com/python/cpython/issues/125789 for details).
+    for (i = 0; self->fut_callbacks != NULL; i++) {
+        if (!PyList_Check(self->fut_callbacks)) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+            goto fail;
+        }
+        if (i >= PyList_GET_SIZE(self->fut_callbacks)) {
+            break; // done
+        }
+        PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i);
+        if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+            goto fail;
+        }
+        Py_INCREF(cb_tup);
+        PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
+        int ret = PyObject_RichCompareBool(cb, fn, Py_EQ);
         if (ret == 0) {
             if (j < len) {
-                PyList_SET_ITEM(newlist, j, item);
+                PyList_SET_ITEM(newlist, j, cb_tup);
                 j++;
                 continue;
             }
-            ret = PyList_Append(newlist, item);
+            ret = PyList_Append(newlist, cb_tup);
         }
-        Py_DECREF(item);
+        Py_DECREF(cb_tup);
         if (ret < 0) {
             goto fail;
         }

From ca61908e8c7836185c91e34a3b8620a691582ec7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Tue, 22 Oct 2024 12:47:50 +0200
Subject: [PATCH 2/8] blurb

---
 .../Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst     | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst

diff --git a/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst b/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst
new file mode 100644
index 00000000000000..71c07594c587ce
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst
@@ -0,0 +1,3 @@
+Mitigate interpreter's crashes and state corruption due to side-effects in
+:meth:`asyncio.Future.remove_done_callback` or others callback scheduling
+methods. Patch by Bénédikt Tran.

From 640c7999a30f23bb4a72cec4d2d45532af1f29ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Tue, 22 Oct 2024 15:49:02 +0200
Subject: [PATCH 3/8] fix UAF when removing a callback

---
 Modules/_asynciomodule.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index e0c1e4fb7596f6..2f991a4d1698ed 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -1071,8 +1071,10 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
             PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
             return NULL;
         }
+        Py_INCREF(cb_tup);
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
         int cmp = PyObject_RichCompareBool(cb, fn, Py_EQ);
+        Py_DECREF(cb_tup);
         if (cmp == -1) {
             return NULL;
         }

From a05cde237e43b99a47cad807b22cd6aeba020d6b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 23 Oct 2024 14:10:45 +0200
Subject: [PATCH 4/8] Address Guido's and Yury's reviews.

---
 ...-10-22-12-47-46.gh-issue-125789.Hk885p.rst |  4 +-
 Modules/_asynciomodule.c                      | 40 ++++++++++---------
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst b/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst
index 71c07594c587ce..077b7f0dba10fb 100644
--- a/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst
+++ b/Misc/NEWS.d/next/Library/2024-10-22-12-47-46.gh-issue-125789.Hk885p.rst
@@ -1,3 +1,3 @@
-Mitigate interpreter's crashes and state corruption due to side-effects in
-:meth:`asyncio.Future.remove_done_callback` or others callback scheduling
+Mitigate interpreter crashes and state corruption due to side-effects in
+:meth:`asyncio.Future.remove_done_callback` or other callback scheduling
 methods. Patch by Bénédikt Tran.
diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index 2f991a4d1698ed..93e85ea99f603f 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -434,8 +434,8 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
         return 0;
     }
 
-    if (!PyList_Check(fut->fut_callbacks)) {
-        PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+    if (!PyList_CheckExact(fut->fut_callbacks)) {
+        PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
         return -1;
     }
 
@@ -446,19 +446,19 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
         return 0;
     }
 
-    // Beware: 'call_soon' below may change fut_callbacks or its items
+    // Beware: An evil 'call_soon' could change fut_callbacks or its items
     // (see https://github.com/python/cpython/issues/125789 for details).
     for (i = 0; fut->fut_callbacks != NULL; i++) {
-        if (!PyList_Check(fut->fut_callbacks)) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        if (!PyList_CheckExact(fut->fut_callbacks)) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
             return -1;
         }
         if (i >= PyList_GET_SIZE(fut->fut_callbacks)) {
             break; // done
         }
         PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
-        if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 2) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 2) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             return -1;
         }
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
@@ -1035,6 +1035,8 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
     ENSURE_FUTURE_ALIVE(state, self)
 
     if (self->fut_callback0 != NULL) {
+        // Beware: An evil PyObject_RichCompareBool could change fut_callback0
+        // (see https://github.com/python/cpython/issues/125789 for details).
         int cmp = PyObject_RichCompareBool(self->fut_callback0, fn, Py_EQ);
         if (cmp == -1) {
             return NULL;
@@ -1051,8 +1053,8 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         return PyLong_FromSsize_t(cleared_callback0);
     }
 
-    if (!PyList_Check(self->fut_callbacks)) {
-        PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+    if (!PyList_CheckExact(self->fut_callbacks)) {
+        PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
         return NULL;
     }
 
@@ -1064,11 +1066,11 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
 
     if (len == 1) {
         PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, 0);
-        // Beware: PyObject_RichCompareBool below may change fut_callbacks or
-        // its items (see https://github.com/python/cpython/issues/97592 and
+        // Beware: An evil PyObject_RichCompareBool could change fut_callbacks
+        // or its items (see https://github.com/python/cpython/issues/97592 or
         // https://github.com/python/cpython/issues/125789 for details).
-        if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             return NULL;
         }
         Py_INCREF(cb_tup);
@@ -1092,20 +1094,20 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         return NULL;
     }
 
-    // Beware: PyObject_RichCompareBool below may change fut_callbacks or
-    // its items (see https://github.com/python/cpython/issues/97592 and
+    // Beware: An evil PyObject_RichCompareBool could change fut_callbacks
+    // or its items (see https://github.com/python/cpython/issues/97592 or
     // https://github.com/python/cpython/issues/125789 for details).
     for (i = 0; self->fut_callbacks != NULL; i++) {
-        if (!PyList_Check(self->fut_callbacks)) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        if (!PyList_CheckExact(self->fut_callbacks)) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
             goto fail;
         }
         if (i >= PyList_GET_SIZE(self->fut_callbacks)) {
             break; // done
         }
         PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i);
-        if (!PyTuple_Check(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted future state");
+        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             goto fail;
         }
         Py_INCREF(cb_tup);

From 22b21cc9aace297a076fd3b53315782a0703ed13 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 23 Oct 2024 14:11:19 +0200
Subject: [PATCH 5/8] Fix UAF (address Nico Posada's review)

---
 Lib/test/test_asyncio/test_futures.py | 39 +++++++++++++++++++++++++++
 Modules/_asynciomodule.c              | 10 ++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py
index cbd963e0e7b5a5..3aec83f9b6d7a1 100644
--- a/Lib/test/test_asyncio/test_futures.py
+++ b/Lib/test/test_asyncio/test_futures.py
@@ -970,6 +970,45 @@ def __eq__(self, other):
 
         self.assertIs(mem, cb_pad)
 
+    # Use-After-Free sanity checks.
+    # Credits to Nico-Posada for the PoCs.
+    # See https://github.com/python/cpython/pull/125833.
+
+    def test_use_after_free_fixed_1(self):
+        fut = self._new_future()
+
+        class setup:
+            def __eq__(self, other):
+                return False
+
+        # evil MUST be a subclass of setup for __eq__ to get called first
+        class evil(setup):
+            def __eq__(self, value):
+                fut._callbacks.clear()
+                return NotImplemented
+
+        cb_pad = lambda: ...
+        fut.add_done_callback(cb_pad)  # sets fut->fut_callback0
+        fut.add_done_callback(setup())  # sets fut->fut_callbacks[0]
+        # removes callback from fut->fut_callback0 setting it to NULL
+        fut.remove_done_callback(cb_pad)
+        fut.remove_done_callback(evil())
+
+    def test_use_after_free_fixed_2(self):
+        fut = self._new_future()
+
+        class cb_pad:
+            def __eq__(self, other):
+                return True
+
+        class evil(cb_pad):
+            def __eq__(self, other):
+                fut.remove_done_callback(None)
+                return NotImplemented
+
+        fut.add_done_callback(cb_pad())
+        fut.remove_done_callback(evil())
+
 
 @unittest.skipUnless(hasattr(futures, '_CFuture'),
                      'requires the C _asyncio module')
diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index 93e85ea99f603f..db2f0a30d15082 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -1036,8 +1036,12 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
 
     if (self->fut_callback0 != NULL) {
         // Beware: An evil PyObject_RichCompareBool could change fut_callback0
-        // (see https://github.com/python/cpython/issues/125789 for details).
-        int cmp = PyObject_RichCompareBool(self->fut_callback0, fn, Py_EQ);
+        // (see https://github.com/python/cpython/issues/125789 for details)
+        // In addition, the reference to self->fut_callback0 may be cleared,
+        // so we need to temporarily hold it explicitly.
+        PyObject *fut_callback0 = Py_NewRef(self->fut_callback0);
+        int cmp = PyObject_RichCompareBool(fut_callback0, fn, Py_EQ);
+        Py_DECREF(fut_callback0);
         if (cmp == -1) {
             return NULL;
         }
@@ -1076,7 +1080,7 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         Py_INCREF(cb_tup);
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
         int cmp = PyObject_RichCompareBool(cb, fn, Py_EQ);
-        Py_DECREF(cb_tup);
+        Py_XDECREF(cb_tup);
         if (cmp == -1) {
             return NULL;
         }

From f7b67309e88614d34e8cdafe591bfb6bddc8c661 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Wed, 23 Oct 2024 15:15:31 +0200
Subject: [PATCH 6/8] Enforce conditions on the internal tuple

---
 Modules/_asynciomodule.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index db2f0a30d15082..c680fe485fd779 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -457,7 +457,7 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
             break; // done
         }
         PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
-        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 2) {
+        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
             PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             return -1;
         }
@@ -1073,7 +1073,7 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         // Beware: An evil PyObject_RichCompareBool could change fut_callbacks
         // or its items (see https://github.com/python/cpython/issues/97592 or
         // https://github.com/python/cpython/issues/125789 for details).
-        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
+        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
             PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             return NULL;
         }
@@ -1110,7 +1110,7 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
             break; // done
         }
         PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i);
-        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) < 1) {
+        if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
             PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             goto fail;
         }

From f72c3934583530b4c8e772295d617bb286847fca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 24 Oct 2024 10:31:07 +0200
Subject: [PATCH 7/8] Address reviews and improve coverage.

---
 Lib/asyncio/futures.py                |   3 +-
 Lib/test/test_asyncio/test_futures.py | 135 +++++++++++++++++++-------
 Modules/_asynciomodule.c              |  36 ++-----
 3 files changed, 111 insertions(+), 63 deletions(-)

diff --git a/Lib/asyncio/futures.py b/Lib/asyncio/futures.py
index c95fce035cd548..2422520c97bb98 100644
--- a/Lib/asyncio/futures.py
+++ b/Lib/asyncio/futures.py
@@ -234,10 +234,11 @@ def remove_done_callback(self, fn):
 
         Returns the number of callbacks removed.
         """
+        count = len(self._callbacks)
         filtered_callbacks = [(f, ctx)
                               for (f, ctx) in self._callbacks
                               if f != fn]
-        removed_count = len(self._callbacks) - len(filtered_callbacks)
+        removed_count = count - len(filtered_callbacks)
         if removed_count:
             self._callbacks[:] = filtered_callbacks
         return removed_count
diff --git a/Lib/test/test_asyncio/test_futures.py b/Lib/test/test_asyncio/test_futures.py
index 3aec83f9b6d7a1..8cf3087e5b8213 100644
--- a/Lib/test/test_asyncio/test_futures.py
+++ b/Lib/test/test_asyncio/test_futures.py
@@ -929,13 +929,14 @@ def __eq__(self, other):
 
         fut.remove_done_callback(evil())
 
+    # Sanity checks for callback tuples corruption and Use-After-Free.
+    # Special thanks to Nico-Posada for the original PoCs and ideas.
+    # See https://github.com/python/cpython/issues/125789.
+
     def test_schedule_callbacks_list_mutation_3(self):
-        raise NotImplemented
+        raise NotImplementedError
 
     def _test_schedule_callbacks_list_mutation_3(self, exc_type, exc_text=None):
-        # see https://github.com/python/cpython/issues/125789 for details
-        fut = self._new_future()
-
         class evil:
             def __eq__(self, other):
                 global mem
@@ -943,36 +944,77 @@ def __eq__(self, other):
                 return False
 
         cb_pad = lambda: ...
+
+        fut = self._new_future()
         fut.add_done_callback(cb_pad)  # sets fut->fut_callback0
-        fut.add_done_callback(evil())  # sets first item in fut->fut_callbacks
-        # Consume fut->fut_callback0 callback but checks the remaining callbacks,
-        # thereby invoking evil.__eq__().
-        fut.remove_done_callback(cb_pad)
+        fut.add_done_callback(evil())  # sets fut->fut_callbacks[0]
+        # Consume fut->fut_callback0 callback and set it to NULL before
+        # checking fut->fut_callbacks, thereby invoking evil.__eq__().
+        removed = fut.remove_done_callback(cb_pad)
+        self.assertEqual(removed, 1)
         self.assertIs(mem, cb_pad)
-
-        fake = (
-            (0).to_bytes(8, 'little') +
-            id(bytearray).to_bytes(8, 'little') +
-            (2 ** 63 - 1).to_bytes(8, 'little') +
-            (0).to_bytes(24, 'little')
-        )
-
-        i2f = lambda num: 5e-324 * num
-        fut._callbacks[0] = complex(0, i2f(id(fake) + bytes.__basicsize__ - 1))
-
-        # We want to call once again evil.__eq__() to set 'mem' to our
-        # malicious bytearray. However, since we manually modified the
-        # callbacks list, we will not be able to by-pass the checks.
+        # Set the malicious callback tuple and trigger a type check on it
+        # by re-invoking evil.__eq__() through remove_done_callback().
+        fut._callbacks[0] = complex(0, 0)
         if exc_text is None:
             self.assertRaises(exc_type, fut.remove_done_callback, evil())
         else:
-            self.assertRaisesRegex(exc_type, exc_text, fut.remove_done_callback, evil())
-
+            self.assertRaisesRegex(exc_type, exc_text,
+                                   fut.remove_done_callback, evil())
         self.assertIs(mem, cb_pad)
 
-    # Use-After-Free sanity checks.
-    # Credits to Nico-Posada for the PoCs.
-    # See https://github.com/python/cpython/pull/125833.
+    def _evil_call_soon_event_loop(self, evil_call_soon):
+        fake_event_loop = lambda: ...
+        fake_event_loop.call_soon = evil_call_soon
+        fake_event_loop.get_debug = lambda: False  # suppress traceback
+        return fake_event_loop
+
+    def test_evil_call_soon_list_mutation_1(self):
+        def evil_call_soon(*args, **kwargs):
+            fut._callbacks.clear()
+
+        loop = self._evil_call_soon_event_loop(evil_call_soon)
+        with mock.patch.object(self, 'loop', loop):
+            fut = self._new_future()
+            self.assertIs(fut.get_loop(), loop)
+
+            cb_pad = lambda: ...
+            fut.add_done_callback(cb_pad)
+            fut.add_done_callback(None)
+            fut.add_done_callback(None)
+
+            removed = fut.remove_done_callback(cb_pad)
+            self.assertEqual(removed, 1)
+            self.assertEqual(len(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, []))
+
+    def test_evil_call_soon_list_mutation_2(self):
+        raise NotImplementedError
+
+    def _test_evil_call_soon_list_mutation_2(self, exc_type, exc_text=None):
+        def evil_call_soon(*args, **kwargs):
+            fut._callbacks[1] = complex(0, 0)
+
+        loop = self._evil_call_soon_event_loop(evil_call_soon)
+        with mock.patch.object(self, 'loop', loop):
+            fut = self._new_future()
+            self.assertIs(fut.get_loop(), loop)
+
+            cb_pad = lambda: ...
+            fut.add_done_callback(cb_pad)
+            fut.add_done_callback(None)
+            fut.add_done_callback(None)
+            removed = fut.remove_done_callback(cb_pad)
+            self.assertEqual(removed, 1)
+            self.assertEqual(len(fut._callbacks), 2)
+            # The evil 'call_soon' is executed by calling set_result().
+            if exc_text is None:
+                self.assertRaises(exc_type, fut.set_result, "boom")
+            else:
+                self.assertRaisesRegex(exc_type, exc_text, fut.set_result, "boom")
 
     def test_use_after_free_fixed_1(self):
         fut = self._new_future()
@@ -990,11 +1032,17 @@ def __eq__(self, value):
         cb_pad = lambda: ...
         fut.add_done_callback(cb_pad)  # sets fut->fut_callback0
         fut.add_done_callback(setup())  # sets fut->fut_callbacks[0]
-        # removes callback from fut->fut_callback0 setting it to NULL
-        fut.remove_done_callback(cb_pad)
-        fut.remove_done_callback(evil())
+        removed = fut.remove_done_callback(cb_pad)
+        self.assertEqual(removed, 1)
+
+        # This triggers evil.__eq__(), thereby clearing fut->fut_callbacks
+        # but we will still hold a reference to fut->fut_callbacks[0] until
+        # it is no more needed.
+        removed = fut.remove_done_callback(evil())
+        self.assertEqual(removed, 0)
 
     def test_use_after_free_fixed_2(self):
+        asserter = self
         fut = self._new_future()
 
         class cb_pad:
@@ -1003,11 +1051,13 @@ def __eq__(self, other):
 
         class evil(cb_pad):
             def __eq__(self, other):
-                fut.remove_done_callback(None)
+                removed = fut.remove_done_callback(None)
+                asserter.assertEqual(removed, 1)
                 return NotImplemented
 
         fut.add_done_callback(cb_pad())
-        fut.remove_done_callback(evil())
+        removed = fut.remove_done_callback(evil())
+        self.assertEqual(removed, 1)
 
 
 @unittest.skipUnless(hasattr(futures, '_CFuture'),
@@ -1019,7 +1069,12 @@ def _new_future(self):
         return futures._CFuture(loop=self.loop)
 
     def test_schedule_callbacks_list_mutation_3(self):
-        super()._test_schedule_callbacks_list_mutation_3(RuntimeError, 'corrupted')
+        errmsg = 'corrupted callback tuple'
+        super()._test_schedule_callbacks_list_mutation_3(RuntimeError, errmsg)
+
+    def test_evil_call_soon_list_mutation_2(self):
+        errmsg = 'corrupted callback tuple'
+        super()._test_evil_call_soon_list_mutation_2(RuntimeError, errmsg)
 
 
 @unittest.skipUnless(hasattr(futures, '_CFuture'),
@@ -1033,7 +1088,12 @@ class CSubFuture(futures._CFuture):
         return CSubFuture(loop=self.loop)
 
     def test_schedule_callbacks_list_mutation_3(self):
-        super()._test_schedule_callbacks_list_mutation_3(RuntimeError, 'corrupted')
+        errmsg = 'corrupted callback tuple'
+        super()._test_schedule_callbacks_list_mutation_3(RuntimeError, errmsg)
+
+    def test_evil_call_soon_list_mutation_2(self):
+        errmsg = 'corrupted callback tuple'
+        super()._test_evil_call_soon_list_mutation_2(RuntimeError, errmsg)
 
 
 class PyFutureDoneCallbackTests(BaseFutureDoneCallbackTests,
@@ -1045,6 +1105,13 @@ def _new_future(self):
     def test_schedule_callbacks_list_mutation_3(self):
         super()._test_schedule_callbacks_list_mutation_3(TypeError)
 
+    def test_evil_call_soon_list_mutation_2(self):
+        # For this test, the Python implementation raises an IndexError
+        # because the attribute fut._callbacks is set to an empty list
+        # *before* invoking the callbacks, while the C implementation
+        # does not make a temporary copy of the list of callbacks.
+        super()._test_evil_call_soon_list_mutation_2(IndexError)
+
 
 class BaseFutureInheritanceTests:
 
diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index c680fe485fd779..404ea096cdcd59 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -434,11 +434,6 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
         return 0;
     }
 
-    if (!PyList_CheckExact(fut->fut_callbacks)) {
-        PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
-        return -1;
-    }
-
     len = PyList_GET_SIZE(fut->fut_callbacks);
     if (len == 0) {
         /* The list of callbacks was empty; clear it and return. */
@@ -448,17 +443,12 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
 
     // Beware: An evil 'call_soon' could change fut_callbacks or its items
     // (see https://github.com/python/cpython/issues/125789 for details).
-    for (i = 0; fut->fut_callbacks != NULL; i++) {
-        if (!PyList_CheckExact(fut->fut_callbacks)) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
-            return -1;
-        }
-        if (i >= PyList_GET_SIZE(fut->fut_callbacks)) {
-            break; // done
-        }
+    for (i = 0;
+         fut->fut_callbacks != NULL && i < PyList_GET_SIZE(fut->fut_callbacks);
+         i++) {
         PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
         if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple?");
             return -1;
         }
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
@@ -1057,11 +1047,6 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         return PyLong_FromSsize_t(cleared_callback0);
     }
 
-    if (!PyList_CheckExact(self->fut_callbacks)) {
-        PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
-        return NULL;
-    }
-
     len = PyList_GET_SIZE(self->fut_callbacks);
     if (len == 0) {
         Py_CLEAR(self->fut_callbacks);
@@ -1080,7 +1065,7 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
         Py_INCREF(cb_tup);
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);
         int cmp = PyObject_RichCompareBool(cb, fn, Py_EQ);
-        Py_XDECREF(cb_tup);
+        Py_DECREF(cb_tup);
         if (cmp == -1) {
             return NULL;
         }
@@ -1101,14 +1086,9 @@ _asyncio_Future_remove_done_callback_impl(FutureObj *self, PyTypeObject *cls,
     // Beware: An evil PyObject_RichCompareBool could change fut_callbacks
     // or its items (see https://github.com/python/cpython/issues/97592 or
     // https://github.com/python/cpython/issues/125789 for details).
-    for (i = 0; self->fut_callbacks != NULL; i++) {
-        if (!PyList_CheckExact(self->fut_callbacks)) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted callbacks list");
-            goto fail;
-        }
-        if (i >= PyList_GET_SIZE(self->fut_callbacks)) {
-            break; // done
-        }
+    for (i = 0;
+         self->fut_callbacks != NULL && i < PyList_GET_SIZE(self->fut_callbacks);
+         i++) {
         PyObject *cb_tup = PyList_GET_ITEM(self->fut_callbacks, i);
         if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
             PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");

From 378fe095eba672639a1247059598d8468abc71ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?=
 <10796600+picnixz@users.noreply.github.com>
Date: Thu, 24 Oct 2024 11:02:45 +0200
Subject: [PATCH 8/8] fix typo

---
 Modules/_asynciomodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c
index 404ea096cdcd59..d76f8dd80fee76 100644
--- a/Modules/_asynciomodule.c
+++ b/Modules/_asynciomodule.c
@@ -448,7 +448,7 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
          i++) {
         PyObject *cb_tup = PyList_GET_ITEM(fut->fut_callbacks, i);
         if (!PyTuple_CheckExact(cb_tup) || PyTuple_GET_SIZE(cb_tup) != 2) {
-            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple?");
+            PyErr_SetString(PyExc_RuntimeError, "corrupted callback tuple");
             return -1;
         }
         PyObject *cb = PyTuple_GET_ITEM(cb_tup, 0);