diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d9c7551a..71afd61b 100644 --- a/.github/workflows/tests.yml +++ b/.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 diff --git a/docs/greenlet_gc.rst b/docs/greenlet_gc.rst index 18afa9f6..092a5c38 100644 --- a/docs/greenlet_gc.rst +++ b/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 diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index 0d4e0d9c..074aa973 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/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 diff --git a/src/greenlet/TPythonState.cpp b/src/greenlet/TPythonState.cpp index 7c493b8e..cf105fcb 100644 --- a/src/greenlet/TPythonState.cpp +++ b/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 diff --git a/src/greenlet/greenlet_msvc_compat.hpp b/src/greenlet/greenlet_msvc_compat.hpp index 9635a1b9..fa25d715 100644 --- a/src/greenlet/greenlet_msvc_compat.hpp +++ b/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; diff --git a/src/greenlet/tests/test_gc.py b/src/greenlet/tests/test_gc.py index 898452f1..b981bc73 100644 --- a/src/greenlet/tests/test_gc.py +++ b/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())