Address automated code audit results

CHANGES.rst

@@ -2,15 +2,25 @@
  Changes
 =========
 
-3.3.3 (unreleased)
+3.4.0 (unreleased)
 ==================
 
 - Publish binary wheels for RiscV 64.
-- Fix multiple rare crash paths during interpreter shutdown on.
+- Fix multiple rare crash paths during interpreter shutdown.
+
+  Note that this now relies on the ``atexit`` module, and introduces
+  subtle API changes during interpreter shutdown (for example,
+  ``getcurrent`` is no longer available once the ``atexit`` callback fires).
 
   See `PR #499
   <https://github.com/python-greenlet/greenlet/pull/499>`_ by Nicolas
   Bouvrette.
+- Address the results of an automated code audit performed by
+  devdanzin. This includes several minor correctness changes that
+  theoretically could have been crashing bugs, but typically only in
+  very rare circumstances.
+
+  See `PR 502 <https://github.com/python-greenlet/greenlet/pull/502>`_.
 
 3.3.2 (2026-02-20)
 ==================

docs/c_api.rst

@@ -83,6 +83,8 @@ Functions
     Switches to the greenlet *g*. Besides *g*, the remaining
     parameters are optional and may be ``NULL``.
 
+    Returns a new reference.
+
     :param args: If ``args`` is NULL, an empty tuple is passed to the
                  target greenlet. If given, must be a :class:`tuple`.
 

src/greenlet/PyGreenlet.cpp

@@ -240,9 +240,17 @@ _green_dealloc_kill_started_non_main_greenlet(BorrowedGreenlet self)
         PyObject* f = PySys_GetObject("stderr");
         Py_INCREF(self.borrow_o()); /* leak! */
         if (f != NULL) {
+            // PySys_GetObject returns a borrowed ref which could go
+            // away when we run arbitrary code, as we do for any of
+            // the ``PyFile_Write`` APIs.
+            Py_INCREF(f);
+            // Note that we're not handling errors here. They either
+            // work or they don't, and any exception they raised will
+            // be replaced by PyErrRestore.
             PyFile_WriteString("GreenletExit did not kill ", f);
             PyFile_WriteObject(self.borrow_o(), f, 0);
             PyFile_WriteString("\n", f);
+            Py_DECREF(f);
         }
     }
     /* Restore the saved exception. */
@@ -389,7 +397,6 @@ green_switch(PyGreenlet* self, PyObject* args, PyObject* kwargs)
     // second byte of the CALL_METHOD op for ``getcurrent()``).
 
     try {
-        //OwnedObject result = single_result(self->pimpl->g_switch());
         OwnedObject result(single_result(self->pimpl->g_switch()));
 #ifndef NDEBUG
         // Note that the current greenlet isn't necessarily self. If self

src/greenlet/PyGreenletUnswitchable.cpp

@@ -114,7 +114,7 @@ static PyGetSetDef green_unswitchable_getsets[] = {
       .name="force_switch_error",
       .get=(getter)green_unswitchable_getforce,
       .set=(setter)green_unswitchable_setforce,
-      .doc=NULL
+      .doc=nullptr
     },
     {
       .name="force_slp_switch_error",
@@ -126,21 +126,24 @@ static PyGetSetDef green_unswitchable_getsets[] = {
 };
 
 PyTypeObject PyGreenletUnswitchable_Type = {
-    .ob_base=PyVarObject_HEAD_INIT(NULL, 0)
-    .tp_name="greenlet._greenlet.UnswitchableGreenlet",
-    .tp_dealloc= (destructor)green_dealloc, /* tp_dealloc */
-    .tp_flags=G_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
-    .tp_doc="Undocumented internal class",                        /* tp_doc */
-    .tp_traverse=(traverseproc)green_traverse, /* tp_traverse */
-    .tp_clear=(inquiry)green_clear,         /* tp_clear */
-
-    .tp_getset=green_unswitchable_getsets,                      /* tp_getset */
-    .tp_base=&PyGreenlet_Type,                                  /* tp_base */
-    .tp_init=(initproc)green_init,               /* tp_init */
-    .tp_alloc=PyType_GenericAlloc,                  /* tp_alloc */
-    .tp_new=(newfunc)green_unswitchable_new,                          /* tp_new */
-    .tp_free=PyObject_GC_Del,                   /* tp_free */
-    .tp_is_gc=(inquiry)green_is_gc,         /* tp_is_gc */
+    .ob_base = PyVarObject_HEAD_INIT(NULL, 0)
+    .tp_name = "greenlet._greenlet.UnswitchableGreenlet",
+    .tp_dealloc = (destructor)green_dealloc,
+    .tp_flags = G_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
+    .tp_doc = "Undocumented internal class for testing error conditions",
+    .tp_traverse = (traverseproc)green_traverse,
+    .tp_clear = (inquiry)green_clear,
+
+    .tp_getset = green_unswitchable_getsets,
+    .tp_base = &PyGreenlet_Type,
+    .tp_init = (initproc)green_init,
+    .tp_alloc = PyType_GenericAlloc,
+    .tp_new = (newfunc)green_unswitchable_new,
+    .tp_free = PyObject_GC_Del,
+#ifndef Py_GIL_DISABLED
+    // See comments in the base type
+    .tp_is_gc = (inquiry)green_is_gc,
+#endif
 };
 
 

src/greenlet/TGreenlet.cpp

@@ -392,6 +392,10 @@ g_handle_exit(const OwnedObject& greenlet_result)
     if (!greenlet_result && mod_globs->PyExc_GreenletExit.PyExceptionMatches()) {
         /* catch and ignore GreenletExit */
         PyErrFetchParam val;
+        // TODO: When we run on 3.12+ only (GREENLET_312), switch to the
+        // ``PyErr_GetRaisedException`` family of functions. The
+        // ``PyErr_Fetch`` family is deprecated on 3.12+, but is part
+        // of the stable ABI so it's not going anywhere.
         PyErr_Fetch(PyErrFetchParam(), val, PyErrFetchParam());
         if (!val) {
             return OwnedObject::None();

src/greenlet/TGreenlet.hpp

@@ -18,6 +18,7 @@ using greenlet::refs::OwnedMainGreenlet;
 using greenlet::refs::BorrowedGreenlet;
 
 #if PY_VERSION_HEX < 0x30B00A6
+    // prior to 3.11.0a6
 #  define _PyCFrame CFrame
 #  define _PyInterpreterFrame _interpreter_frame
 #endif
@@ -781,9 +782,7 @@ class TracingGuard
 
     // Instantiate one on the stack to save the GC state,
     // and then disable GC. When it goes out of scope, GC will be
-    // restored to its original state. Sadly, these APIs are only
-    // available on 3.10+; luckily, we only need them on 3.11+.
-#if GREENLET_PY310
+    // restored to its original state.
     class GCDisabledGuard
     {
     private:
@@ -802,7 +801,6 @@ class TracingGuard
             }
         }
     };
-#endif
 
     OwnedObject& operator<<=(OwnedObject& lhs, greenlet::SwitchingArgs& rhs) noexcept;
 

src/greenlet/TPythonState.cpp

@@ -173,10 +173,27 @@ void PythonState::operator<<(const PyThreadState *const tstate) noexcept
     }
   #endif
   #if GREENLET_PY313
+    // By contract of _PyTrash_thread_deposit_object,
+    // the ``delete_later`` object has a refcount of 0.
+    // We take a strong reference to it.
+    //
+    // Now, ``delete_later`` is managed as a
+    // linked list whose objects are unconditionally deallocated
+    // WITHOUT calling DECREF on them, so it's not clear what that is
+    // actually accomplishing. That is, if another object is pushed on
+    // the list and then the list is deallocated, this object will
+    // still be deallocated. This strong reference serves as a form of
+    // resurrection, meaning that when operator>> DECREFs it, we might
+    // enter its ``tp_dealloc`` function again.
+    //
+    // In practice, it's quite difficult to arrange for this to be
+    // a non-null value during a greenlet switch.
+    // ``greenlet.tests.test_greenlet_trash`` tries, but under 3.14,
+    // at least, fails to do so.
     this->delete_later = Py_XNewRef(tstate->delete_later);
   #elif GREENLET_PY312
     this->trash_delete_nesting = tstate->trash.delete_nesting;
-  #else // not 312
+  #else // not 312 or 3.13+
     this->trash_delete_nesting = tstate->trash_delete_nesting;
   #endif // GREENLET_PY312
 #else // Not 311
@@ -257,9 +274,32 @@ void PythonState::operator>>(PyThreadState *const tstate) noexcept
 #endif
     this->_top_frame.relinquish_ownership();
   #if GREENLET_PY313
-    Py_XDECREF(tstate->delete_later);
-    tstate->delete_later = this->delete_later;
-    Py_CLEAR(this->delete_later);
+    // See comments in operator<<. We own a strong reference to
+    // this->delete_later, which may or may not be the same object as
+    // tstate->delete_later (depending if something pushed an object
+    // onto the trashcan). Again, because ``delete_later`` is managed
+    // as a linked list, it's not clear that saving and restoring the
+    // value, especially without ever setting it to NULL, accomplishes
+    // much...but the code was added by a core dev, so assume correct.
+    //
+    // Recall that tstate->delete_later is supposed to have a refcount
+    // of 0, because objects are added there from their ``tp_dealloc``
+    // method. So we should only need to DECREF it if we're the ones
+    // that INCREF'd it in operator<<. (This is different than the
+    // core dev's original code which always did this.)
+    if (this->delete_later == tstate->delete_later) {
+        Py_XDECREF(tstate->delete_later);
+        tstate->delete_later = this->delete_later;
+        this->delete_later = nullptr;
+    }
+    else {
+        // it got switched behind our back. So the reference we own
+        // needs to be explicitly cleared.
+        tstate->delete_later = this->delete_later;
+        Py_CLEAR(this->delete_later);
+    }
+
+
   #elif GREENLET_PY312
     tstate->trash.delete_nesting = this->trash_delete_nesting;
   #else // not 3.12
@@ -289,13 +329,18 @@ void PythonState::set_initial_state(const PyThreadState* const tstate) noexcept
 #if GREENLET_PY314
     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;
+    #endif
+    // this->stackpointer is left null because this->_top_frame is
+    // null so there is no value to copy.
 #elif GREENLET_PY312
     this->py_recursion_depth = tstate->py_recursion_limit - tstate->py_recursion_remaining;
-    // XXX: TODO: Comment from a reviewer:
-    //     Should this be ``Py_C_RECURSION_LIMIT - tstate->c_recursion_remaining``?
-    // But to me it looks more like that might not be the right
-    // initialization either?
-    this->c_recursion_depth = tstate->py_recursion_limit - tstate->py_recursion_remaining;
+#if GREENLET_314
+    this->c_recursion_depth = 0; // unused on 3.14
+#else
+    this->c_recursion_depth = Py_C_RECURSION_LIMIT - tstate->c_recursion_remaining;
+#endif
 #elif GREENLET_PY311
     this->recursion_depth = tstate->recursion_limit - tstate->recursion_remaining;
 #else
@@ -319,6 +364,12 @@ int PythonState::tp_traverse(visitproc visit, void* arg, bool own_top_frame) noe
     // The naive way of looping over c_stack_refs->ref and visiting
     // those crashes the process (at least with GIL disabled).
 #endif
+    // Note that we DO NOT visit ``delete_later``. Even if it's
+    // non-null and we technically own a reference to it, its
+    // reference count already went to 0 once and it was in the
+    // process of being deallocated. The trash can mechanism linked it
+    // into a list that will be cleaned at some later time, and it has
+    // become untracked by the GC.
     return 0;
 }