diff --git a/CHANGES.rst b/CHANGES.rst index c4a3c07a..0b2c1428 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,11 +5,25 @@ 3.5.4 (unreleased) ================== +- Fix a crash (segfault) on free-threaded builds of Python 3.14 and + later when the garbage collector runs while a greenlet that was + started from a non-empty C-stack-reference state is active. + See `issue 515 + `_. + Thanks to ddorian and Kumar Aditya. + +- Fix a potential use-after-free on free-threaded builds of Python 3.14 + and later when the garbage collector runs while a greenlet is + suspended holding a ``_PyCStackRef`` (for example, mid attribute + resolution). See + `issue 515 `_. + Thanks to ddorian and Kumar Aditya. + - Fix a deadlock on free-threaded builds when a greenlet switch happened while a ``PyCriticalSection`` was held -- for example inside asyncio's ``Task.__step``, which holds one on the running task for the duration of the step. See `PR 519 `. - Thank to ddorian and Kumar Aditya. + Thanks to ddorian and Kumar Aditya. 3.5.3 (2026-06-26) ================== diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index 68591b0a..7838ab92 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/src/greenlet/TGreenlet.hpp @@ -8,6 +8,7 @@ #include #include +#include #include "greenlet_compiler_compat.hpp" #include "greenlet_refs.hpp" @@ -140,6 +141,12 @@ namespace greenlet _PyStackRef* stackpointer; #ifdef Py_GIL_DISABLED _PyCStackRef* c_stack_refs; + // Strong references to the objects held by the _PyCStackRef nodes on + // our C stack, snapshotted by operator<< when we suspend so tp_traverse + // can keep them alive for the free-threaded GC (capture_c_stack_refs + // explains why we snapshot rather than walk the list). Empty while we + // run. + std::vector c_stack_ref_snapshot; #endif #elif GREENLET_PY312 int py_recursion_depth; @@ -171,6 +178,14 @@ namespace greenlet // need to be present for the eval loop to work. void unexpose_frames(); +#if GREENLET_PY314 && defined(Py_GIL_DISABLED) + // Take a strong reference to every object held by tstate's _PyCStackRef + // list into c_stack_ref_snapshot so tp_traverse can keep them alive + // while we're suspended. Must run while our C stack is still live + // (operator<<). The snapshot is dropped by clear()ing the vector. + void capture_c_stack_refs(const PyThreadState* tstate) noexcept; +#endif + public: PythonState(); diff --git a/src/greenlet/TPythonState.cpp b/src/greenlet/TPythonState.cpp index 321c0465..bdedebe5 100644 --- a/src/greenlet/TPythonState.cpp +++ b/src/greenlet/TPythonState.cpp @@ -92,6 +92,30 @@ PythonState::PythonState() #endif } +#if GREENLET_PY314 && defined(Py_GIL_DISABLED) +void PythonState::capture_c_stack_refs(const PyThreadState* tstate) noexcept +{ + // Runs from operator<< while our C stack is still live and coherent, so we + // can walk tstate's _PyCStackRef list and take a strong reference to every + // object it holds. tp_traverse visits these once we're suspended, because + // by then the nodes themselves have been relocated into the heap stack copy + // and the saved list head no longer points at them. Strong references (not + // _Py_VISIT_STACKREF, whose _PyGC_VisitStackRef isn't exported before 3.15); + // a std::vector rather than a Python list/tuple because operator<< must not + // allocate a GC-tracked object mid-switch. Rebuilt from scratch each time; + // the list is empty at a typical switch, so this is usually just an empty + // loop. + this->c_stack_ref_snapshot.clear(); + for (const _PyCStackRef* node = ((_PyThreadStateImpl*)tstate)->c_stack_refs; + node != nullptr; node = node->next) { + if (!PyStackRef_IsNullOrInt(node->ref)) { + this->c_stack_ref_snapshot.push_back( + OwnedObject::owning(PyStackRef_AsPyObjectBorrow(node->ref))); + } + } +} +#endif + inline void PythonState::may_switch_away() noexcept { @@ -145,6 +169,9 @@ void PythonState::operator<<(const PyThreadState *const tstate) noexcept this->current_executor = tstate->current_executor; #ifdef Py_GIL_DISABLED this->c_stack_refs = ((_PyThreadStateImpl*)tstate)->c_stack_refs; + // Capture the deferred references now, while our C stack is still live, so + // tp_traverse can keep them from being collected while we're suspended. + this->capture_c_stack_refs(tstate); #endif #elif GREENLET_PY312 this->py_recursion_depth = tstate->py_recursion_limit - tstate->py_recursion_remaining; @@ -265,6 +292,10 @@ void PythonState::operator>>(PyThreadState *const tstate) noexcept tstate->current_executor = this->current_executor; #ifdef Py_GIL_DISABLED ((_PyThreadStateImpl*)tstate)->c_stack_refs = this->c_stack_refs; + // We're the running greenlet again: our C-stack refs live in the thread + // state now and gc_visit_thread_stacks() covers them, so drop the strong + // references tp_traverse held on our behalf while we were suspended. + this->c_stack_ref_snapshot.clear(); #endif this->unexpose_frames(); #elif GREENLET_PY312 @@ -355,7 +386,14 @@ void PythonState::set_initial_state(const PyThreadState* const tstate) noexcept this->py_recursion_depth = tstate->py_recursion_limit - tstate->py_recursion_remaining; this->current_executor = tstate->current_executor; #ifdef Py_GIL_DISABLED - this->c_stack_refs = ((_PyThreadStateImpl*)tstate)->c_stack_refs; + // Start with an empty C-stack-ref list, the way a brand-new thread does; + // do NOT copy the parent thread state's head. Those _PyCStackRef nodes sit + // on the parent greenlet's C stack, so once we start running on our own + // stack and overwrite that region, following them reads garbage. The + // free-threaded collector walks c_stack_refs for every thread in + // gc_visit_thread_stacks(), so leaving the stale head here crashed it. + // See https://github.com/python-greenlet/greenlet/issues/515. + this->c_stack_refs = nullptr; #endif // this->stackpointer is left null because this->_top_frame is // null so there is no value to copy. @@ -405,6 +443,18 @@ int PythonState::tp_traverse(visitproc visit, void* arg, bool visit_top_frame) n } } } +#endif +#if GREENLET_PY314 && defined(Py_GIL_DISABLED) + // Visit the objects this greenlet's C-stack refs were holding when it + // suspended (captured by capture_c_stack_refs). The free-threaded collector + // only walks the running thread's _PyCStackRef list in + // gc_visit_thread_stacks(), so without this a collection could free an + // object reachable only through a suspended greenlet's C-stack ref and we'd + // use it after free once the greenlet resumed. The snapshot is empty while + // we're the running greenlet, so this is a no-op there. + for (const OwnedObject& ref : this->c_stack_ref_snapshot) { + Py_VISIT(ref.borrow()); + } #endif // Note that we DO NOT visit ``delete_later``. Even if it's // non-null and we technically own a reference to it, its @@ -418,6 +468,9 @@ int PythonState::tp_traverse(visitproc visit, void* arg, bool visit_top_frame) n void PythonState::tp_clear(bool own_top_frame) noexcept { PythonStateContext::tp_clear(); +#if GREENLET_PY314 && defined(Py_GIL_DISABLED) + this->c_stack_ref_snapshot.clear(); +#endif // If we get here owning a frame, // we got dealloc'd without being finished. We may or may not be // in the same thread. diff --git a/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py b/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py new file mode 100644 index 00000000..8783185d --- /dev/null +++ b/src/greenlet/tests/fail_c_stack_refs_suspended_gc.py @@ -0,0 +1,77 @@ +"""Regression test for GC of a suspended greenlet's C-stack refs (issue #515). + +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. + +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. +""" +import gc +import sys +import weakref + +import greenlet + +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. + del cls, obj, objtype + child.switch() + return 42 + + +pinned = Meta('pinned', (), {}) # a class => deferred-refcounted +holder = type('holder', (), {'attr': pinned}) # holder.attr invokes Meta.__get__ +box = [pinned] +del pinned + + +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. + ref = weakref.ref(box[0]) + box[0] = None + del holder.attr + for _ in range(5): + gc.collect() + observed['alive'] = ref() is not None + parent.switch() + + +child = greenlet.greenlet(child_work) +result = holder().attr # Meta.__get__ -> switch -> gc -> back +assert result == 42, result + +alive = observed.get('alive') +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 " + "greenlet's C-stack ref was collected early") +print("C STACK REFS GC OK", flush=True) diff --git a/src/greenlet/tests/fail_issue_515_freethread_gc.py b/src/greenlet/tests/fail_issue_515_freethread_gc.py new file mode 100644 index 00000000..42b11be7 --- /dev/null +++ b/src/greenlet/tests/fail_issue_515_freethread_gc.py @@ -0,0 +1,112 @@ +"""Reproducer for the free-threaded GC crash in issue #515. + +https://github.com/python-greenlet/greenlet/issues/515 + +Before the fix, ``set_initial_state`` copied the parent thread state's +``_PyCStackRef`` list head into each new greenlet. Those nodes sit on the +parent greenlet's C stack, so once the child starts running on its own stack +and overwrites that region their ``next`` pointers dangle. The free-threaded +collector walks ``c_stack_refs`` for every thread, so a GC on any thread ends +up chasing those dangling nodes (inside ``gc_collect_main`` -> +``gc_visit_thread_stacks``) while such a child is active, and segfaults. + +The fiddly part is getting a *non-empty* list to inherit at all. The +interpreter only holds a ``_PyCStackRef`` transiently, while it is resolving an +attribute -- ``_Py_LoadAttr_StackRefSteal`` pins ``self`` across ``obj.attr()`` +-- so the greenlet has to be primed from inside that window. Priming it from a +property getter does exactly that; it is also the same deep-attribute context +Playwright's sync bridge happens to start its fiber from. A churn thread then +keeps the collector busy while the dispatcher runs on its stale list. + +A fixed build prints "ISSUE 515 OK"; a regressed one segfaults in about a +second. With-GIL builds are unaffected -- ordinary refcounting keeps the nodes +alive -- so there it simply prints the OK line. +""" +import gc +import os +import sys +import threading + +import greenlet + +# The crash lands within a second, so a few seconds leaves plenty of margin on +# a slow machine without dragging out a fixed build. +DURATION = float(os.environ.get("REPRO_SECONDS", "4")) +NWORKERS = int(os.environ.get("REPRO_THREADS", "6")) + + +def _busy(): + # Keep the dispatcher doing work (resolving str methods) so a collection on + # the churn thread catches it while it is the active greenlet. + return "abc".upper().lower().strip().title() + + +class Bridge: + def __init__(self, stop): + self.stop = stop + self.main = greenlet.getcurrent() + self.disp = greenlet.greenlet(self._loop) + + def _loop(self): + while not self.stop[0]: + for _ in range(50): + _busy() + self.main.switch() + + @property + def _prime(self): + # Reaching this getter means we are mid attribute-resolution, so a + # _PyCStackRef for ``self`` is live right now. Priming the dispatcher + # from here is what makes it inherit a non-empty c_stack_refs head. + self.disp.switch() + return None + + def start(self): + # Reading the property (rather than calling a method) is deliberate: the + # getter runs *during* attribute resolution, so it primes the dispatcher + # while the _PyCStackRef for ``self`` is still held. We return the value + # only so this isn't a bare, pointless-looking expression statement. + return self._prime + + def resume(self): + self.disp.switch() + + +def worker(stop): + bridge = Bridge(stop) + bridge.start() + # start() has returned, so the stack the inherited nodes point at is being + # reused -- the dispatcher's list is stale now. Keep it active to trip the GC. + while not stop[0]: + bridge.resume() + + +def churn(stop): + while not stop[0]: + # A cycle forces a real (stop-the-world) collection rather than just a + # refcount drop. + nodes = [{"i": i, "next": None} for i in range(500)] + for a, b in zip(nodes, nodes[1:]): + a["next"] = b + nodes[-1]["next"] = nodes[0] + del nodes + gc.collect() + + +def main(): + stop = [False] + threads = [threading.Thread(target=churn, args=(stop,), daemon=True)] + threads += [threading.Thread(target=worker, args=(stop,), daemon=True) + for _ in range(NWORKERS)] + for t in threads: + t.start() + threading.Event().wait(DURATION) + stop[0] = True + for t in threads: + t.join(timeout=5) + print("ISSUE 515 OK") + + +if __name__ == "__main__": + main() + sys.exit(0) diff --git a/src/greenlet/tests/test_gc.py b/src/greenlet/tests/test_gc.py index 340fdbe3..d725143a 100644 --- a/src/greenlet/tests/test_gc.py +++ b/src/greenlet/tests/test_gc.py @@ -6,6 +6,7 @@ from . import TestCase +from . import RUNNING_ON_FREETHREAD_BUILD from .leakcheck import fails_leakcheck_on_py314_or_less # These only work with greenlet gc support # which is no longer optional. @@ -84,6 +85,32 @@ def greenlet_body(): greenlet.getcurrent() gc.collect() + def test_issue515_freethread_c_stack_refs(self): + # Guards issue #515: a new greenlet inherited the parent's C-stack refs + # and the free-threaded collector segfaulted following the dangling + # nodes. This has to run out of process because the regression is a hard + # crash, not something we can catch. + # 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_issue_515_freethread_gc.py') + 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 + 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') + self.assertIn('C STACK REFS GC OK', output) + def test_crashing_deferred_object(self): if sys.version_info < (3, 15): self.skipTest("Test is 3.15+ only")