Merge pull request #511 from kumaraditya303/ft-fix

Fix free-threading crash from untraversed deferred references in suspended greenlets

Author: jamadden

.github/workflows/tests.yml

@@ -130,7 +130,8 @@ jobs:
         # standalone in a unit test doesn't produce the error.
         #
         # So this is a temporary workaround.
-        PYTHON_TLBC: "0"
+        # This has been fixed for 3.15+
+        PYTHON_TLBC: ${{ matrix.python-version == '3.14t' && '0' || '1' }}
       run: |
         sphinx-build -b doctest -d docs/_build/doctrees2 docs docs/_build/doctest2
     - name: Lint

docs/greenlet_gc.rst

@@ -164,53 +164,3 @@ frames and cycles can be freed:
     Collecting garbage
     (Running finalizer)
     (Running finalizer)
-
-A Cycle Of Greenlets Is A Leak
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-What if we introduce a cycle among the greenlets themselves while also
-leaving a greenlet suspended? Here, the frames of the ``inner``
-greenlet refer to the ``outer`` (as the ``inner`` greenlet itself
-does), and both the frames of the ``outer``, as well as the ``outer``
-greenlet itself, refer to the ``inner``:
-
-.. doctest::
-    :pyversion: >= 3.5
-
-    >>> def inner():
-    ...      cycle1 = Cycle()
-    ...      cycle2 = Cycle()
-    ...      cycle1.cycle = cycle2
-    ...      cycle2.cycle = cycle1
-    ...      parent = getcurrent().parent
-    ...      parent.switch()
-    >>> def outer():
-    ...     glet = greenlet(inner)
-    ...     getcurrent().child_greenlet = glet
-    ...     glet.switch()
-    ...     collect_it()
-
-This time, even letting the outer and inner greenlets die doesn't find
-the cycle hidden in the inner greenlet's frame:
-
-.. doctest::
-    :pyversion: >= 3.5
-
-    >>> outer_glet = greenlet(outer)
-    >>> outer_glet.switch()
-    Collecting garbage
-    >>> outer_glet.dead
-    True
-    >>> collect_it()
-    Collecting garbage
-
-Even explicitly deleting the outer greenlet doesn't find and clear the
-cycle; we have created a legitimate memory leak, not just of the
-greenlet objects, but also the objects in any suspended frames:
-
-.. doctest::
-    :pyversion: >= 3.5
-
-    >>> del outer_glet
-    >>> collect_it()
-    Collecting garbage

src/greenlet/TGreenlet.hpp

@@ -42,6 +42,14 @@ using greenlet::refs::BorrowedGreenlet;
 #endif
 #endif
 
+
+#if GREENLET_PY315
+#include "internal/pycore_gc.h"
+#if !defined(_MSC_VER) && !defined(__MINGW64__)
+#include "internal/pycore_stackref.h"
+#endif
+#endif
+
 // XXX: TODO: Work to remove all virtual functions
 // for speed of calling and size of objects (no vtable).
 // One pattern is the Curiously Recurring Template

src/greenlet/TPythonState.cpp

@@ -348,21 +348,38 @@ void PythonState::set_initial_state(const PyThreadState* const tstate) noexcept
 #endif
 }
 // TODO: Better state management about when we own the top frame.
-int PythonState::tp_traverse(visitproc visit, void* arg, bool own_top_frame) noexcept
+int PythonState::tp_traverse(visitproc visit, void* arg, bool visit_top_frame) noexcept
 {
     Py_VISIT(this->_context.borrow());
-    if (own_top_frame) {
+    if (visit_top_frame) {
         Py_VISIT(this->_top_frame.borrow());
     }
-#if GREENLET_PY314
-    // TODO: Should we be visiting the c_stack_refs objects?
-    // CPython uses a specific macro to do that which takes into
-    // account boxing and null values and then calls
-    // ``_PyGC_VisitStackRef``, but we don't have access to that, and
-    // we can't duplicate it ourself (because it compares
-    // ``visitproc`` to another function we can't access).
-    // The naive way of looping over c_stack_refs->ref and visiting
-    // those crashes the process (at least with GIL disabled).
+#if GREENLET_PY315
+    // Visit the references held by our suspended frames.
+    // This is important specially on free-threading where the
+    // the suspended frames may contain deferred references to
+    // objects, and if they are not traversed then the interpreter
+    // can free objects early causing a use-after-free crash
+    // at runtime exit.
+    if (this->_top_frame) {
+        for (_PyInterpreterFrame* iframe = this->_top_frame->f_frame;
+             iframe != nullptr; iframe = iframe->previous) {
+            // Skip generator/coroutine frames; their object's traverse
+            // already visits them (gen_traverse), so we'd double-count.
+            // expose_frames leaves them in the ->previous chain.
+            if (iframe->owner != FRAME_OWNED_BY_THREAD) {
+                continue;
+            }
+            Py_VISIT(iframe->frame_obj);
+            Py_VISIT(iframe->f_locals);
+            _Py_VISIT_STACKREF(iframe->f_funcobj);
+            _Py_VISIT_STACKREF(iframe->f_executable);
+            int frame_result = _PyGC_VisitFrameStack(iframe, visit, arg);
+            if (frame_result) {
+                return frame_result;
+            }
+        }
+    }
 #endif
     // Note that we DO NOT visit ``delete_later``. Even if it's
     // non-null and we technically own a reference to it, its

src/greenlet/greenlet_msvc_compat.hpp

@@ -50,12 +50,34 @@ _PyCode_GetTLBCArray(PyCodeObject *co)
 #else
 #define Py_TAG_BITS     ((uintptr_t)1)
 #define Py_TAG_DEFERRED (1)
+#define Py_INT_TAG      3
 #endif
 
 
 static const _PyStackRef PyStackRef_NULL = { .bits = Py_TAG_DEFERRED};
 #define PyStackRef_IsNull(stackref) ((stackref).bits == PyStackRef_NULL.bits)
 
+static inline bool
+PyStackRef_IsTaggedInt(_PyStackRef i)
+{
+    return (i.bits & Py_INT_TAG) == Py_INT_TAG;
+}
+
+static inline bool
+PyStackRef_IsNullOrInt(_PyStackRef stackref)
+{
+    return PyStackRef_IsNull(stackref) || PyStackRef_IsTaggedInt(stackref);
+}
+
+#define _Py_VISIT_STACKREF(ref)                                         \
+    do {                                                                \
+        if (!PyStackRef_IsNullOrInt(ref)) {                             \
+            int vret = _PyGC_VisitStackRef(&(ref), visit, arg);         \
+            if (vret)                                                   \
+                return vret;                                            \
+        }                                                               \
+    } while (0)
+
 static inline PyObject *
 PyStackRef_AsPyObjectBorrow(_PyStackRef stackref)
 {
@@ -64,7 +86,7 @@ PyStackRef_AsPyObjectBorrow(_PyStackRef stackref)
 }
 
 static inline PyCodeObject *_PyFrame_GetCode(_PyInterpreterFrame *f) {
-    assert(!PyStackRef_IsNull(f->f_executable));
+    assert(!PyStackRef_IsNullOrInt(f->f_executable));
     PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);
     assert(PyCode_Check(executable));
     return (PyCodeObject *)executable;

src/greenlet/tests/test_gc.py

@@ -1,7 +1,7 @@
 import gc
 
 import weakref
-
+import sys
 import greenlet
 
 
@@ -16,7 +16,6 @@ def test_dead_circular_ref(self):
         o = weakref.ref(greenlet.greenlet(greenlet.getcurrent).switch())
         gc.collect()
         if o() is not None:
-            import sys
             print("O IS NOT NONE.", sys.getrefcount(o()))
         self.assertIsNone(o())
         self.assertFalse(gc.garbage, gc.garbage)
@@ -84,3 +83,55 @@ def greenlet_body():
         del g
         greenlet.getcurrent()
         gc.collect()
+
+    def test_crashing_deferred_object(self):
+        if sys.version_info < (3, 15):
+            self.skipTest("Test is 3.15+ only")
+        import doctest
+        def with_doctest():
+            """
+            >>> import gc
+            >>> from greenlet import getcurrent, greenlet, GreenletExit
+            >>> def outer():
+            ...     gc.collect()
+            >>> outer_glet = greenlet(outer)
+            >>> outer_glet.switch()
+            """
+        doctest.run_docstring_examples(with_doctest, dict())
+
+    def test_cycle_in_suspended_frame(self):
+        if sys.version_info < (3, 15):
+            self.skipTest("Test is 3.15+ only")
+        import doctest
+        def with_doctest():
+            """
+            >>> import gc
+            >>> from greenlet import getcurrent, greenlet
+            >>> class Cycle:
+            ...     def __del__(self):
+            ...         print("(Running finalizer)")
+            >>> def collect_it():
+            ...     print("Collecting garbage")
+            ...     gc.collect()
+            >>> def inner():
+            ...     cycle1 = Cycle()
+            ...     cycle2 = Cycle()
+            ...     cycle1.cycle = cycle2
+            ...     cycle2.cycle = cycle1
+            ...     getcurrent().parent.switch()
+            >>> def outer():
+            ...     glet = greenlet(inner)
+            ...     glet.switch()
+            ...     collect_it()
+
+            >>> outer_glet = greenlet(outer)
+            >>> outer_glet.switch()
+            Collecting garbage
+            >>> outer_glet.dead
+            True
+            >>> collect_it()
+            Collecting garbage
+            (Running finalizer)
+            (Running finalizer)
+            """
+        doctest.run_docstring_examples(with_doctest, dict())