pythongh-76785: Use Pending Calls When Releasing Cross-Interpreter Data (pythongh-109556)

ericsnowcurrently · ericsnowcurrently · commit b1e4a2495a8b · 2023-10-12T01:41:03.000-06:00
This fixes some crashes in the _xxinterpchannels module, due to a race between interpreters. (cherry picked from commit fd7e08a) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h
@@ -431,6 +431,7 @@ PyAPI_FUNC(void) _PyCrossInterpreterData_Clear(
 PyAPI_FUNC(int) _PyObject_GetCrossInterpreterData(PyObject *, _PyCrossInterpreterData *);
 PyAPI_FUNC(PyObject *) _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *);
 PyAPI_FUNC(int) _PyCrossInterpreterData_Release(_PyCrossInterpreterData *);
+PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *);
 
 PyAPI_FUNC(int) _PyObject_CheckCrossInterpreterData(PyObject *);
 
diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h
@@ -26,7 +26,7 @@ extern void _PyEval_FiniState(struct _ceval_state *ceval);
 PyAPI_FUNC(void) _PyEval_SignalReceived(PyInterpreterState *interp);
 PyAPI_FUNC(int) _PyEval_AddPendingCall(
     PyInterpreterState *interp,
-    int (*func)(void *),
+    _Py_pending_call_func func,
     void *arg,
     int mainthreadonly);
 PyAPI_FUNC(void) _PyEval_SignalAsyncExc(PyInterpreterState *interp);
diff --git a/Include/internal/pycore_ceval_state.h b/Include/internal/pycore_ceval_state.h
@@ -13,6 +13,8 @@ extern "C" {
 #include "pycore_gil.h"             // struct _gil_runtime_state
 
 
+typedef int (*_Py_pending_call_func)(void *);
+
 struct _pending_calls {
     int busy;
     PyThread_type_lock lock;
@@ -24,7 +26,7 @@ struct _pending_calls {
     int async_exc;
 #define NPENDINGCALLS 32
     struct _pending_call {
-        int (*func)(void *);
+        _Py_pending_call_func func;
         void *arg;
     } calls[NPENDINGCALLS];
     int first;
diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c
@@ -161,21 +161,34 @@ add_new_type(PyObject *mod, PyType_Spec *spec, crossinterpdatafunc shared)
     return cls;
 }
 
+#define XID_IGNORE_EXC 1
+#define XID_FREE 2
+
 static int
-_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc)
+_release_xid_data(_PyCrossInterpreterData *data, int flags)
 {
+    int ignoreexc = flags & XID_IGNORE_EXC;
     PyObject *exc;
     if (ignoreexc) {
         exc = PyErr_GetRaisedException();
     }
-    int res = _PyCrossInterpreterData_Release(data);
+    int res;
+    if (flags & XID_FREE) {
+        res = _PyCrossInterpreterData_ReleaseAndRawFree(data);
+    }
+    else {
+        res = _PyCrossInterpreterData_Release(data);
+    }
     if (res < 0) {
         /* The owning interpreter is already destroyed. */
         if (ignoreexc) {
             // XXX Emit a warning?
             PyErr_Clear();
         }
     }
+    if (flags & XID_FREE) {
+        /* Either way, we free the data. */
+    }
     if (ignoreexc) {
         PyErr_SetRaisedException(exc);
     }
@@ -367,9 +380,8 @@ static void
 _channelitem_clear(_channelitem *item)
 {
     if (item->data != NULL) {
-        (void)_release_xid_data(item->data, 1);
         // It was allocated in _channel_send().
-        GLOBAL_FREE(item->data);
+        (void)_release_xid_data(item->data, XID_IGNORE_EXC & XID_FREE);
         item->data = NULL;
     }
     item->next = NULL;
@@ -1440,14 +1452,12 @@ _channel_recv(_channels *channels, int64_t id, PyObject **res)
     PyObject *obj = _PyCrossInterpreterData_NewObject(data);
     if (obj == NULL) {
         assert(PyErr_Occurred());
-        (void)_release_xid_data(data, 1);
-        // It was allocated in _channel_send().
-        GLOBAL_FREE(data);
+        // It was allocated in _channel_send(), so we free it.
+        (void)_release_xid_data(data, XID_IGNORE_EXC | XID_FREE);
         return -1;
     }
-    int release_res = _release_xid_data(data, 0);
-    // It was allocated in _channel_send().
-    GLOBAL_FREE(data);
+    // It was allocated in _channel_send(), so we free it.
+    int release_res = _release_xid_data(data, XID_FREE);
     if (release_res < 0) {
         // The source interpreter has been destroyed already.
         assert(PyErr_Occurred());
diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c
@@ -60,24 +60,17 @@ add_new_exception(PyObject *mod, const char *name, PyObject *base)
     add_new_exception(MOD, MODULE_NAME "." Py_STRINGIFY(NAME), BASE)
 
 static int
-_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc)
+_release_xid_data(_PyCrossInterpreterData *data)
 {
-    PyObject *exc;
-    if (ignoreexc) {
-        exc = PyErr_GetRaisedException();
-    }
+    PyObject *exc = PyErr_GetRaisedException();
     int res = _PyCrossInterpreterData_Release(data);
     if (res < 0) {
         /* The owning interpreter is already destroyed. */
         _PyCrossInterpreterData_Clear(NULL, data);
-        if (ignoreexc) {
-            // XXX Emit a warning?
-            PyErr_Clear();
-        }
-    }
-    if (ignoreexc) {
-        PyErr_SetRaisedException(exc);
+        // XXX Emit a warning?
+        PyErr_Clear();
     }
+    PyErr_SetRaisedException(exc);
     return res;
 }
 
@@ -147,7 +140,7 @@ _sharednsitem_clear(struct _sharednsitem *item)
         PyMem_RawFree((void *)item->name);
         item->name = NULL;
     }
-    (void)_release_xid_data(&item->data, 1);
+    (void)_release_xid_data(&item->data);
 }
 
 static int
@@ -176,16 +169,16 @@ typedef struct _sharedns {
 static _sharedns *
 _sharedns_new(Py_ssize_t len)
 {
-    _sharedns *shared = PyMem_NEW(_sharedns, 1);
+    _sharedns *shared = PyMem_RawCalloc(sizeof(_sharedns), 1);
     if (shared == NULL) {
         PyErr_NoMemory();
         return NULL;
     }
     shared->len = len;
-    shared->items = PyMem_NEW(struct _sharednsitem, len);
+    shared->items = PyMem_RawCalloc(sizeof(struct _sharednsitem), len);
     if (shared->items == NULL) {
         PyErr_NoMemory();
-        PyMem_Free(shared);
+        PyMem_RawFree(shared);
         return NULL;
     }
     return shared;
@@ -197,8 +190,8 @@ _sharedns_free(_sharedns *shared)
     for (Py_ssize_t i=0; i < shared->len; i++) {
         _sharednsitem_clear(&shared->items[i]);
     }
-    PyMem_Free(shared->items);
-    PyMem_Free(shared);
+    PyMem_RawFree(shared->items);
+    PyMem_RawFree(shared);
 }
 
 static _sharedns *
diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c
@@ -757,7 +757,7 @@ _PyEval_SignalReceived(PyInterpreterState *interp)
 /* Push one item onto the queue while holding the lock. */
 static int
 _push_pending_call(struct _pending_calls *pending,
-                   int (*func)(void *), void *arg)
+                   _Py_pending_call_func func, void *arg)
 {
     int i = pending->last;
     int j = (i + 1) % NPENDINGCALLS;
@@ -804,7 +804,7 @@ _pop_pending_call(struct _pending_calls *pending,
 
 int
 _PyEval_AddPendingCall(PyInterpreterState *interp,
-                       int (*func)(void *), void *arg,
+                       _Py_pending_call_func func, void *arg,
                        int mainthreadonly)
 {
     assert(!mainthreadonly || _Py_IsMainInterpreter(interp));
@@ -828,7 +828,7 @@ _PyEval_AddPendingCall(PyInterpreterState *interp,
 }
 
 int
-Py_AddPendingCall(int (*func)(void *), void *arg)
+Py_AddPendingCall(_Py_pending_call_func func, void *arg)
 {
     /* Legacy users of this API will continue to target the main thread
        (of the main interpreter). */
@@ -872,7 +872,7 @@ _make_pending_calls(struct _pending_calls *pending)
 {
     /* perform a bounded number of calls, in case of recursion */
     for (int i=0; i<NPENDINGCALLS; i++) {
-        int (*func)(void *) = NULL;
+        _Py_pending_call_func func = NULL;
         void *arg = NULL;
 
         /* pop one item off the queue while holding the lock */
diff --git a/Python/pystate.c b/Python/pystate.c
@@ -2292,10 +2292,16 @@ _xidata_init(_PyCrossInterpreterData *data)
 static inline void
 _xidata_clear(_PyCrossInterpreterData *data)
 {
-    if (data->free != NULL) {
-        data->free(data->data);
+    // _PyCrossInterpreterData only has two members that need to be
+    // cleaned up, if set: "data" must be freed and "obj" must be decref'ed.
+    // In both cases the original (owning) interpreter must be used,
+    // which is the caller's responsibility to ensure.
+    if (data->data != NULL) {
+        if (data->free != NULL) {
+            data->free(data->data);
+        }
+        data->data = NULL;
     }
-    data->data = NULL;
     Py_CLEAR(data->obj);
 }
 
@@ -2440,40 +2446,32 @@ _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *data)
     return data->new_object(data);
 }
 
-typedef void (*releasefunc)(PyInterpreterState *, void *);
-
-static void
-_call_in_interpreter(PyInterpreterState *interp, releasefunc func, void *arg)
+static int
+_release_xidata_pending(void *data)
 {
-    /* We would use Py_AddPendingCall() if it weren't specific to the
-     * main interpreter (see bpo-33608).  In the meantime we take a
-     * naive approach.
-     */
-    _PyRuntimeState *runtime = interp->runtime;
-    PyThreadState *save_tstate = NULL;
-    if (interp != current_fast_get(runtime)->interp) {
-        // XXX Using the "head" thread isn't strictly correct.
-        PyThreadState *tstate = PyInterpreterState_ThreadHead(interp);
-        // XXX Possible GILState issues?
-        save_tstate = _PyThreadState_Swap(runtime, tstate);
-    }
-
-    // XXX Once the GIL is per-interpreter, this should be called with the
-    // calling interpreter's GIL released and the target interpreter's held.
-    func(interp, arg);
+    _xidata_clear((_PyCrossInterpreterData *)data);
+    return 0;
+}
 
-    // Switch back.
-    if (save_tstate != NULL) {
-        _PyThreadState_Swap(runtime, save_tstate);
-    }
+static int
+_xidata_release_and_rawfree_pending(void *data)
+{
+    _xidata_clear((_PyCrossInterpreterData *)data);
+    PyMem_RawFree(data);
+    return 0;
 }
 
-int
-_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
+static int
+_xidata_release(_PyCrossInterpreterData *data, int rawfree)
 {
-    if (data->free == NULL && data->obj == NULL) {
+    if ((data->data == NULL || data->free == NULL) && data->obj == NULL) {
         // Nothing to release!
-        data->data = NULL;
+        if (rawfree) {
+            PyMem_RawFree(data);
+        }
+        else {
+            data->data = NULL;
+        }
         return 0;
     }
 
@@ -2484,15 +2482,42 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
         // This function shouldn't have been called.
         // XXX Someone leaked some memory...
         assert(PyErr_Occurred());
+        if (rawfree) {
+            PyMem_RawFree(data);
+        }
         return -1;
     }
 
     // "Release" the data and/or the object.
-    _call_in_interpreter(interp,
-                         (releasefunc)_PyCrossInterpreterData_Clear, data);
+    if (interp == current_fast_get(interp->runtime)->interp) {
+        _xidata_clear(data);
+        if (rawfree) {
+            PyMem_RawFree(data);
+        }
+    }
+    else {
+        _Py_pending_call_func func = _release_xidata_pending;
+        if (rawfree) {
+            func = _xidata_release_and_rawfree_pending;
+        }
+        // XXX Emit a warning if this fails?
+        _PyEval_AddPendingCall(interp, func, data, 0);
+    }
     return 0;
 }
 
+int
+_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
+{
+    return _xidata_release(data, 0);
+}
+
+int
+_PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data)
+{
+    return _xidata_release(data, 1);
+}
+
 /* registry of {type -> crossinterpdatafunc} */
 
 /* For now we use a global registry of shareable classes.  An
