fix refleaks from suspended frames

.github/workflows/tests.yml

@@ -21,6 +21,7 @@ jobs:
   test:
     runs-on: ${{ matrix.os }}
     strategy:
+      fail-fast: false
       matrix:
         python-version:
           - "3.10"
@@ -187,6 +188,7 @@ jobs:
     runs-on: ubuntu-latest
     # We use a regular Python matrix entry to share as much code as possible.
     strategy:
+      fail-fast: false
       matrix:
         python-version:
           - "3.14"

src/greenlet/PyGreenlet.cpp

@@ -808,7 +808,7 @@ PyTypeObject PyGreenlet_Type = {
     .tp_alloc=PyType_GenericAlloc,                  /* tp_alloc */
     .tp_new=(newfunc)green_new,                          /* tp_new */
     .tp_free=PyObject_GC_Del,                   /* tp_free */
-#ifndef Py_GIL_DISABLED
+#if !GREENLET_PY315 && !(GREENLET_PY314 && defined(Py_GIL_DISABLED))
 /*
   We may have been handling this wrong all along.
 

src/greenlet/TGreenlet.cpp

@@ -607,6 +607,10 @@ Greenlet::tp_clear()
     bool own_top_frame = this->was_running_in_dead_thread();
     this->exception_state.tp_clear();
     this->python_state.tp_clear(own_top_frame);
+    if (own_top_frame) {
+        // Throw away any saved stack state since the owned frame is cleared.
+        this->stack_state.set_inactive();
+    }
     return 0;
 }
 

src/greenlet/TPythonState.cpp

@@ -397,7 +397,36 @@ void PythonState::tp_clear(bool own_top_frame) noexcept
     // we got dealloc'd without being finished. We may or may not be
     // in the same thread.
     if (own_top_frame) {
+#if GREENLET_PY315
+        // Release the references held by our suspended frames.
+        // this->top_frame gets implicitly cleared by the Py_CLEAR(iframe->frame_obj)
+        // of the first complete frame, so in the end we relinquish ownership of it.
+        if (this->_top_frame) {
+            for (_PyInterpreterFrame* iframe = this->_top_frame->f_frame;
+                 iframe != nullptr; iframe = iframe->previous) {
+                if (iframe->owner != FRAME_OWNED_BY_THREAD) {
+                    continue;
+                }
+                // Clear the references held by this frame's evaluation stack.
+                _PyStackRef* locals = iframe->localsplus;
+                _PyStackRef* sp = iframe->stackpointer;
+                if (sp) {
+                    while (sp > locals) {
+                        sp--;
+                        PyStackRef_CLEAR(*sp);
+                    }
+                    iframe->stackpointer = locals;
+                }
+                Py_CLEAR(iframe->f_locals);
+                Py_CLEAR(iframe->frame_obj);
+                PyStackRef_CLEAR(iframe->f_funcobj);
+                PyStackRef_CLEAR(iframe->f_executable);
+            }
+        }
+        this->_top_frame.relinquish_ownership();
+#else
         this->_top_frame.CLEAR();
+#endif
     }
 }
 

src/greenlet/TUserGreenlet.cpp

@@ -422,11 +422,13 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run)
     args <<= this->args();
     assert(!this->args());
 
-    // XXX: We could clear this much earlier, right?
-    // Or would that introduce the possibility of running Python
-    // code when we don't want to?
-    // CAUTION: This may run arbitrary Python code.
     this->_run_callable.CLEAR();
+    // stash the run callable in this->_run_callable to ensure that GC will be
+    // able to find the object later.
+    // This is needed for the case of a permanently suspended greenlet
+    // so that the run callable is not leaked.
+    this->_run_callable.steal(run);
+    run = nullptr;
 
 
     // The first switch we need to manually call the trace
@@ -467,7 +469,7 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run)
             // CAUTION: Just invoking this, before the function even
             // runs, may cause memory allocations, which may trigger
             // GC, which may run arbitrary Python code.
-            result = OwnedObject::consuming(PyObject_Call(run, args.args().borrow(), args.kwargs().borrow()));
+            result = OwnedObject::consuming(PyObject_Call(this->_run_callable.borrow(), args.args().borrow(), args.kwargs().borrow()));
         }
         catch (...) {
             // Unhandled C++ exception!
@@ -517,7 +519,8 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run)
     }
     // These lines may run arbitrary code
     args.CLEAR();
-    Py_CLEAR(run);
+    assert(run == nullptr);
+    this->_run_callable.CLEAR();
 
     if (!result
         && mod_globs->PyExc_GreenletExit.PyExceptionMatches()

src/greenlet/greenlet_msvc_compat.hpp

@@ -85,6 +85,27 @@ PyStackRef_AsPyObjectBorrow(_PyStackRef stackref)
     return cleared;
 }
 
+#define Py_TAG_REFCNT 1
+#define BITS_TO_PTR(ref) ((PyObject *)((ref).bits))
+
+#define PyStackRef_RefcountOnObject(ref) (((ref).bits & Py_TAG_REFCNT) == 0)
+
+#define PyStackRef_CLOSE(REF)                            \
+    do {                                                 \
+        _PyStackRef _close_tmp = (REF);                  \
+        if (PyStackRef_RefcountOnObject(_close_tmp)) {   \
+            Py_DECREF(BITS_TO_PTR(_close_tmp));          \
+        }                                                \
+    } while (0)
+
+#define PyStackRef_CLEAR(REF)                            \
+    do {                                                 \
+        _PyStackRef* _clear_ptr = &(REF);                \
+        _PyStackRef _clear_old = (*_clear_ptr);          \
+        *_clear_ptr = PyStackRef_NULL;                   \
+        PyStackRef_CLOSE(_clear_old);                    \
+    } while (0)
+
 static inline PyCodeObject *_PyFrame_GetCode(_PyInterpreterFrame *f) {
     assert(!PyStackRef_IsNullOrInt(f->f_executable));
     PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);

src/greenlet/tests/leakcheck.py

@@ -86,6 +86,21 @@ class and specify variants of behaviour (such as pool sizes).
     func.ignore_leakcheck = True
     return func
 
+def ignores_leakcheck_if(condition, message):
+    """
+    Return a decorator that marks the function to be ignored during
+    leakchecks (see `ignores_leakcheck`) when *condition* is true.
+
+    *message* describes why the leakcheck is ignored. When *condition*
+    is false, the function is returned unchanged.
+    """
+    def decorator(func):
+        if condition:
+            func = ignores_leakcheck(func)
+            func.ignore_leakcheck_reason = message
+        return func
+    return decorator
+
 def fails_leakcheck(func):
     """
     Mark that the function is known to leak.
@@ -95,6 +110,14 @@ def fails_leakcheck(func):
         func = unittest.skip("Skipping known failures")(func)
     return func
 
+def fails_leakcheck_on_py314_or_less(func):
+    """
+    Mark the function as known to leak (fails refcount leakchecks) on Python 3.14 or less.
+    """
+    if sys.version_info[:2] <= (3, 14):
+        return fails_leakcheck(func)
+    return func
+
 class LeakCheckError(AssertionError):
     pass
 
@@ -321,6 +344,12 @@ def __call__(self, args, kwargs):
 
 def wrap_refcount(method):
     if getattr(method, 'ignore_leakcheck', False) or SKIP_LEAKCHECKS:
+        reason = getattr(method, 'ignore_leakcheck_reason', None)
+        if reason and not SKIP_LEAKCHECKS:
+            print(
+                "Ignoring leakchecks for %s: %s" % (method.__name__, reason),
+                file=sys.stderr,
+            )
         return method
 
     @wraps(method)

src/greenlet/tests/test_gc.py

@@ -6,7 +6,7 @@
 
 
 from . import TestCase
-from .leakcheck import fails_leakcheck
+from .leakcheck import fails_leakcheck_on_py314_or_less
 # These only work with greenlet gc support
 # which is no longer optional.
 assert greenlet.GREENLET_USE_GC
@@ -43,7 +43,7 @@ def run(self):
         self.assertIsNone(o())
         self.assertFalse(gc.garbage, gc.garbage)
 
-    @fails_leakcheck
+    @fails_leakcheck_on_py314_or_less
     def test_finalizer_crash(self):
         # This test is designed to crash when active greenlets
         # are made garbage collectable, until the underlying

src/greenlet/tests/test_greenlet.py

@@ -13,9 +13,12 @@
 from . import RUNNING_ON_MANYLINUX
 from . import PY313
 from . import PY314
+from . import WIN
 from . import RUNNING_ON_FREETHREAD_BUILD
 from .leakcheck import fails_leakcheck
+from .leakcheck import fails_leakcheck_on_py314_or_less
 from .leakcheck import ignores_leakcheck
+from .leakcheck import ignores_leakcheck_if
 
 
 # We manually manage locks in many tests
@@ -506,7 +509,7 @@ def __getattribute__(self, name):
         g = mygreenlet(lambda: None)
         self.assertRaises(SomeError, g.throw, SomeError())
 
-    @fails_leakcheck
+    @fails_leakcheck_on_py314_or_less
     def _do_test_throw_to_dead_thread_doesnt_crash(self, wait_for_cleanup=False):
         result = []
         def worker():
@@ -564,7 +567,11 @@ def creator():
         # See issue 252
         self.expect_greenlet_leak = True # direct us not to wait for it to go away
 
-    @fails_leakcheck
+    @fails_leakcheck_on_py314_or_less
+    @ignores_leakcheck_if(
+        WIN and sys.version_info[:2] == (3, 10),
+        "does not leaks on Windows 3.10, but does on other platforms"
+    )
     def test_throw_to_dead_thread_doesnt_crash(self):
         self._do_test_throw_to_dead_thread_doesnt_crash()
 
@@ -1301,6 +1308,15 @@ def test_main_greenlet_is_greenlet(self):
         self._check_current_is_main()
         self.assertIsInstance(greenlet.getcurrent(), RawGreenlet)
 
+    def test_greenlet_run(self):
+        def do_it():
+            with self.assertRaises(AttributeError):
+                getattr(g, 'run')
+
+        g = greenlet.greenlet(do_it)
+        g.switch()
+        with self.assertRaises(AttributeError):
+            getattr(g, 'run')
 
 
 class TestBrokenGreenlets(TestCase):