gh-126907: Lock the atexit state on the free-threaded build

Include/internal/pycore_atexit.h

@@ -52,6 +52,9 @@ struct atexit_state {
     atexit_py_callback **callbacks;
     int ncallbacks;
     int callback_len;
+#ifdef Py_GIL_DISABLED
+    PyMutex lock;
+#endif
 };
 
 // Export for '_interpchannels' shared extension

Lib/test/test_atexit.py

@@ -4,6 +4,7 @@
 import unittest
 from test import support
 from test.support import script_helper
+from test.support import threading_helper
 
 
 class GeneralTest(unittest.TestCase):
@@ -46,6 +47,29 @@ def test_atexit_instances(self):
         self.assertEqual(res.out.decode().splitlines(), ["atexit2", "atexit1"])
         self.assertFalse(res.err)
 
+    @threading_helper.requires_working_threading()
+    @support.requires_resource("cpu")
+    @unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful without the GIL")
+    def test_atexit_thread_safety(self):
+        # GH-126907: atexit was not thread safe on the free-threaded build
+        from threading import Thread
+
+        def dummy():
+            pass
+
+
+        def thready():
+            for _ in range(100):
+                atexit.register(dummy)
+                atexit._clear()
+                atexit.register(dummy)
+                atexit.unregister(dummy)
+
+
+        threads = [Thread(target=thready) for _ in range(100)]
+        with threading_helper.start_threads(threads):
+            pass
+
 
 @support.cpython_only
 class SubinterpreterTest(unittest.TestCase):

Misc/NEWS.d/next/Library/2024-11-16-10-39-02.gh-issue-126907.fWRL_R.rst

@@ -0,0 +1,2 @@
+Fix crash when using :mod:`atexit` concurrently on the :term:`free-threaded
+<free threading>` build.

Modules/atexitmodule.c

@@ -12,6 +12,16 @@
 #include "pycore_interp.h"        // PyInterpreterState.atexit
 #include "pycore_pystate.h"       // _PyInterpreterState_GET
 
+#ifdef Py_GIL_DISABLED
+/* Note: anything declared as static in this file assumes the lock is held
+ * (except for the Python-level functions) */
+#define _PyAtExit_LOCK(state) PyMutex_Lock(&state->lock);
+#define _PyAtExit_UNLOCK(state) PyMutex_Unlock(&state->lock);
+#else
+#define _PyAtExit_LOCK(state)
+#define _PyAtExit_UNLOCK(state)
+#endif
+
 /* ===================================================================== */
 /* Callback machinery. */
 
@@ -38,13 +48,15 @@ PyUnstable_AtExit(PyInterpreterState *interp,
     callback->next = NULL;
 
     struct atexit_state *state = &interp->atexit;
+    _PyAtExit_LOCK(state);
     if (state->ll_callbacks == NULL) {
         state->ll_callbacks = callback;
         state->last_ll_callback = callback;
     }
     else {
         state->last_ll_callback->next = callback;
     }
+    _PyAtExit_UNLOCK(state);
     return 0;
 }
 
@@ -55,9 +67,12 @@ atexit_delete_cb(struct atexit_state *state, int i)
     atexit_py_callback *cb = state->callbacks[i];
     state->callbacks[i] = NULL;
 
+    // This can execute code via finalizers
+    _PyAtExit_UNLOCK(state);
     Py_DECREF(cb->func);
     Py_DECREF(cb->args);
     Py_XDECREF(cb->kwargs);
+    _PyAtExit_LOCK(state);
     PyMem_Free(cb);
 }
 
@@ -99,6 +114,8 @@ void
 _PyAtExit_Fini(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
+    // XXX Can this be unlocked?
+    _PyAtExit_LOCK(state);
     atexit_cleanup(state);
     PyMem_Free(state->callbacks);
     state->callbacks = NULL;
@@ -114,6 +131,7 @@ _PyAtExit_Fini(PyInterpreterState *interp)
         PyMem_Free(callback);
         exitfunc(data);
     }
+    _PyAtExit_UNLOCK(state);
 }
 
 
@@ -133,8 +151,12 @@ atexit_callfuncs(struct atexit_state *state)
         }
 
         // bpo-46025: Increment the refcount of cb->func as the call itself may unregister it
-        PyObject* the_func = Py_NewRef(cb->func);
-        PyObject *res = PyObject_Call(cb->func, cb->args, cb->kwargs);
+        PyObject *the_func = Py_NewRef(cb->func);
+        PyObject *the_args = cb->args;
+        PyObject *the_kwargs = cb->kwargs;
+        // Unlock for re-entrancy problems
+        _PyAtExit_UNLOCK(state);
+        PyObject *res = PyObject_Call(the_func, the_args, the_kwargs);
         if (res == NULL) {
             PyErr_FormatUnraisable(
                 "Exception ignored in atexit callback %R", the_func);
@@ -143,6 +165,7 @@ atexit_callfuncs(struct atexit_state *state)
             Py_DECREF(res);
         }
         Py_DECREF(the_func);
+        _PyAtExit_LOCK(state);
     }
 
     atexit_cleanup(state);
@@ -155,7 +178,9 @@ void
 _PyAtExit_Call(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
+    _PyAtExit_LOCK(state);
     atexit_callfuncs(state);
+    _PyAtExit_UNLOCK(state);
 }
 
 
@@ -192,31 +217,39 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
     }
 
     struct atexit_state *state = get_atexit_state();
+    /* In theory, we could hold the lock for a shorter amount of time
+     * using some fancy compare-exchanges with the length. However, I'm lazy.
+     */
+    _PyAtExit_LOCK(state);
     if (state->ncallbacks >= state->callback_len) {
         atexit_py_callback **r;
         state->callback_len += 16;
         size_t size = sizeof(atexit_py_callback*) * (size_t)state->callback_len;
         r = (atexit_py_callback**)PyMem_Realloc(state->callbacks, size);
         if (r == NULL) {
+            _PyAtExit_UNLOCK(state);
             return PyErr_NoMemory();
         }
         state->callbacks = r;
     }
 
     atexit_py_callback *callback = PyMem_Malloc(sizeof(atexit_py_callback));
     if (callback == NULL) {
+        _PyAtExit_UNLOCK(state);
         return PyErr_NoMemory();
     }
 
     callback->args = PyTuple_GetSlice(args, 1, PyTuple_GET_SIZE(args));
     if (callback->args == NULL) {
         PyMem_Free(callback);
+        _PyAtExit_UNLOCK(state);
         return NULL;
     }
     callback->func = Py_NewRef(func);
     callback->kwargs = Py_XNewRef(kwargs);
 
     state->callbacks[state->ncallbacks++] = callback;
+    _PyAtExit_UNLOCK(state);
 
     return Py_NewRef(func);
 }
@@ -233,7 +266,9 @@ static PyObject *
 atexit_run_exitfuncs(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
+    _PyAtExit_LOCK(state);
     atexit_callfuncs(state);
+    _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
 
@@ -246,7 +281,10 @@ Clear the list of previously registered exit functions.");
 static PyObject *
 atexit_clear(PyObject *module, PyObject *unused)
 {
-    atexit_cleanup(get_atexit_state());
+    struct atexit_state *state = get_atexit_state();
+    _PyAtExit_LOCK(state);
+    atexit_cleanup(state);
+    _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
 
@@ -260,7 +298,10 @@ static PyObject *
 atexit_ncallbacks(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
-    return PyLong_FromSsize_t(state->ncallbacks);
+    _PyAtExit_LOCK(state);
+    PyObject *res = PyLong_FromSsize_t(state->ncallbacks);
+    _PyAtExit_UNLOCK(state);
+    return res;
 }
 
 PyDoc_STRVAR(atexit_unregister__doc__,
@@ -276,21 +317,33 @@ static PyObject *
 atexit_unregister(PyObject *module, PyObject *func)
 {
     struct atexit_state *state = get_atexit_state();
+    _PyAtExit_LOCK(state);
     for (int i = 0; i < state->ncallbacks; i++)
     {
         atexit_py_callback *cb = state->callbacks[i];
         if (cb == NULL) {
             continue;
         }
+        PyObject *to_compare = cb->func;
 
-        int eq = PyObject_RichCompareBool(cb->func, func, Py_EQ);
+        // Unlock for fear of a custom __eq__ causing re-entrancy
+        _PyAtExit_UNLOCK(state);
+        int eq = PyObject_RichCompareBool(to_compare, func, Py_EQ);
         if (eq < 0) {
             return NULL;
         }
+        _PyAtExit_LOCK(state);
+        if (state->callbacks[i] == NULL)
+        {
+            // Edge case: another thread might have
+            // unregistered the function while we released the lock.
+            continue;
+        }
         if (eq) {
             atexit_delete_cb(state, i);
         }
     }
+    _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }