From be55a59f03b5b50d289bac38e6c2997bfe548d78 Mon Sep 17 00:00:00 2001 From: Dorian Hoxha Date: Thu, 23 Jul 2026 13:33:15 +0200 Subject: [PATCH] Check that the suspended greenlet is what keeps the class alive The test only asked whether the class survived the collection, which a build without the fix can also satisfy: if the attribute lookup happened to take a strong _PyCStackRef instead of a deferred one, or any other reference lingered, the class stays alive and the test passes. That is why it kept passing on mac/3.15b3 while failing on manylinux/3.15b4. Look for the suspended greenlet in gc.get_referrers() of the class instead. That is the reference the fix creates, so a build without it reports the masking reference and fails wherever the class survives for some unrelated reason. --- .../tests/fail_c_stack_refs_suspended_gc.py | 80 ++++++++++--------- src/greenlet/tests/test_gc.py | 13 +-- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py b/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py index 8783185d..43bf7bb9 100644 --- a/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py +++ b/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py @@ -1,45 +1,35 @@ -"""Regression test for GC of a suspended greenlet's C-stack refs (issue #515). +"""Regression test for issue #515: GC of a suspended greenlet's C-stack refs. -When a greenlet suspends while the interpreter is holding a ``_PyCStackRef`` -(for example, mid attribute resolution), that deferred reference sits on the -greenlet's C stack. The free-threaded collector only walks the running -thread's list in ``gc_visit_thread_stacks``, so unless greenlet visits the -suspended one from ``tp_traverse`` an object reachable only through it is freed -early and used after free once the greenlet resumes. +A greenlet that suspends mid attribute-resolution holds a ``_PyCStackRef`` to +the looked-up object on its C stack. The free-threaded collector only walks the +running thread's refs, so greenlet must visit a suspended greenlet's from +``tp_traverse``, or an object reachable only through them is freed early. -Reproducing that needs the pinned object to be deferred-refcounted (so its -refcount doesn't keep it alive) and reachable only through the C-stack ref. A -class is deferred-refcounted, and a metaclass ``__get__`` makes a class act as a -descriptor -- so ``obj.attr``, where ``attr`` is such a class, pins the class in -a ``_PyCStackRef`` across the (Python) ``__get__``. We switch away from inside -``__get__``, drop every other reference to the class, and collect: on a -regressed free-threaded build the class is gone by the time we resume; the fix -keeps it alive because ``tp_traverse`` visited the snapshot taken at suspend. - -Prints "C STACK REFS GC OK" on a fixed build (and on with-GIL builds, where -ordinary refcounting keeps the class alive); a regressed free-threaded build -reports the class was collected and exits non-zero. +The test pins a deferred-refcounted class (a metaclass ``__get__`` makes the +class act as a descriptor), switches away from inside ``__get__``, drops every +other reference, and collects. It passes only if the class is still alive and +the suspended greenlet is what keeps it alive. Prints "C STACK REFS GC OK" and +exits 0 on a fixed build, non-zero on a regressed one. """ import gc import sys +import sysconfig import weakref import greenlet +# Only free-threaded builds have the C-stack-ref / deferred-refcount machinery. +FREETHREAD = bool(sysconfig.get_config_var("Py_GIL_DISABLED")) + parent = greenlet.getcurrent() observed = {} class Meta(type): def __get__(cls, obj, objtype=None): - # type(cls) is Meta, which defines __get__, so ``cls`` is a descriptor; - # the getattr machinery pins it in a _PyCStackRef across this call. A - # class is deferred-refcounted on a free-threaded build, so only that - # (deferred) C-stack ref will be keeping it alive in a moment. - # - # Drop the locals the descriptor protocol gave us (the class is ``cls``): - # greenlet visits a suspended greenlet's frames, so a leftover frame ref - # would keep the class alive on its own and mask the bug. + # ``cls`` is the class being resolved, pinned in a _PyCStackRef across + # this call. Drop the descriptor-protocol locals so a suspended frame + # can't keep the class alive on its own and mask the bug. del cls, obj, objtype child.switch() return 42 @@ -52,26 +42,44 @@ def __get__(cls, obj, objtype=None): def child_work(): - # ``parent`` is suspended inside Meta.__get__, holding a C-stack ref to the - # class. Drop every *other* reference to it, then collect: now only greenlet - # visiting the suspended C-stack ref can keep the class alive. + # parent is suspended inside Meta.__get__ holding a C-stack ref to the class. + # Drop every other reference, then collect. ref = weakref.ref(box[0]) box[0] = None del holder.attr for _ in range(5): gc.collect() - observed['alive'] = ref() is not None + cls = ref() + if cls is None: + observed['status'] = 'collected' + elif not FREETHREAD or any(r is parent for r in gc.get_referrers(cls)): + # Kept alive by the suspended greenlet (or, with the GIL, by refcounting). + observed['status'] = 'ok' + else: + # Alive, but not because of the greenlet: a masking reference hid the + # C-stack-ref path (the class's own mro/bases are a self-cycle, not one). + observed['status'] = 'masked' + own = (cls.__mro__, cls.__bases__) + observed['maskers'] = sorted( + {type(r).__name__ for r in gc.get_referrers(cls) + if r is not parent and r is not own[0] and r is not own[1]} + ) or [''] + del cls parent.switch() child = greenlet.greenlet(child_work) -result = holder().attr # Meta.__get__ -> switch -> gc -> back +result = holder().attr assert result == 42, result -alive = observed.get('alive') +status = observed.get('status') print(f"py={sys.version.split()[0]} gil={getattr(sys, '_is_gil_enabled', lambda: True)()} " - f"greenlet={greenlet.__version__} alive={alive}", flush=True) -if not alive: - raise SystemExit("REGRESSED: a class reachable only through a suspended " + f"greenlet={greenlet.__version__} status={status}", flush=True) +if status == 'collected': + raise SystemExit("REGRESSED: class reachable only through a suspended " "greenlet's C-stack ref was collected early") +if status == 'masked': + raise SystemExit("REGRESSED: class stayed alive but not through the suspended " + "greenlet; a masking reference (%s) hid the C-stack-ref path" + % ', '.join(observed.get('maskers', ()))) print("C STACK REFS GC OK", flush=True) diff --git a/src/greenlet/tests/test_gc.py b/src/greenlet/tests/test_gc.py index d725143a..fe075baa 100644 --- a/src/greenlet/tests/test_gc.py +++ b/src/greenlet/tests/test_gc.py @@ -97,15 +97,10 @@ def test_issue515_freethread_c_stack_refs(self): self.assertIn('ISSUE 515 OK', output) def test_c_stack_refs_suspended_gc(self): - # Review follow-up to #515: a greenlet that suspends while holding a - # _PyCStackRef has those deferred references visited by tp_traverse (the - # snapshot in TPythonState.cpp), so the free-threaded collector can't - # free an object reachable only through the suspended greenlet's C stack. - # The script pins a (deferred-refcounted) class via a metaclass __get__, - # switches away from inside it, drops every other reference, and - # collects; without the fix the class is gone on resume. Out of process - # because the regression is a use-after-free. - # https://github.com/python-greenlet/greenlet/issues/515 + # Issue #515: a greenlet suspended while holding a _PyCStackRef must have + # those refs visited by tp_traverse, or the free-threaded collector frees + # an object reachable only through the suspended C stack. Runs the repro + # out of process. https://github.com/python-greenlet/greenlet/issues/515 if not RUNNING_ON_FREETHREAD_BUILD: self.skipTest("Only free-threaded builds are affected") output = self.run_script('fail_c_stack_refs_suspended_gc.py')