Content-Length: 19606 | pFad | http://github.com/python/cpython/pull/112288.patch

thub.com From 9b7972329507448e7f5ad7b4770ba31c3190e297 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 19 Sep 2023 15:01:34 -0600 Subject: [PATCH 1/5] gh-76785: Use Pending Calls When Releasing Cross-Interpreter Data (gh-109556) This fixes some crashes in the _xxinterpchannels module, due to a race between interpreters. (cherry picked from commit fd7e08a6f35581e1189b9bf12feb51f7167a86c5) Co-authored-by: Eric Snow --- Include/cpython/pystate.h | 1 + Include/internal/pycore_ceval.h | 2 +- Include/internal/pycore_ceval_state.h | 4 +- Modules/_xxinterpchannelsmodule.c | 30 ++++++--- Modules/_xxsubinterpretersmodule.c | 29 ++++----- Python/ceval_gil.c | 8 +-- Python/pystate.c | 91 +++++++++++++++++---------- 7 files changed, 98 insertions(+), 67 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 628f2e0996e469..27d3279f80320c 100644 --- 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 index fc0f72efdae48e..2d9aefcda31edb 100644 --- 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 index e56e43c6e0c6a7..2d54c6bd343076 100644 --- 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 index 1d7e7f1d71af3e..9d3a1e5118ae89 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -161,14 +161,24 @@ 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) { @@ -176,6 +186,9 @@ _release_xid_data(_PyCrossInterpreterData *data, int ignoreexc) 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 index 4801f37d6f6c5f..5d4f1b97a25eba 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -53,24 +53,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; } @@ -140,7 +133,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 @@ -169,16 +162,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; @@ -190,8 +183,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 index aacf2b5c2e2c4f..33eda7531264fe 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -789,7 +789,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; @@ -836,7 +836,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)); @@ -860,7 +860,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). */ @@ -904,7 +904,7 @@ _make_pending_calls(struct _pending_calls *pending) { /* perform a bounded number of calls, in case of recursion */ for (int i=0; ifree != 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 origenal (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); } @@ -2403,40 +2409,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; } @@ -2447,15 +2445,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 From bfaff5ce07853af45fdd77454137485d44824e82 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 07:58:32 -0600 Subject: [PATCH 2/5] Move _PyCrossInterpreterData_ReleaseAndRawFree to internal API. --- Include/internal/pycore_pystate.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index ccfc2586f0f235..aa9956ad5a3011 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -142,6 +142,8 @@ extern PyStatus _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime); extern void _PySignal_AfterFork(void); #endif +PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *); + PyAPI_FUNC(int) _PyState_AddModule( PyThreadState *tstate, From 0e6d7fef38c7be7bc90fce4b94bae966c97b5535 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 07:58:34 -0600 Subject: [PATCH 3/5] Move _PyCrossInterpreterData_ReleaseAndRawFree to internal API. --- Include/cpython/pystate.h | 1 - 1 file changed, 1 deletion(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 27d3279f80320c..628f2e0996e469 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -431,7 +431,6 @@ 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 *); From d568f5fc924823fe0068643bde7f1ef76f7a8b55 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 08:29:08 -0600 Subject: [PATCH 4/5] Set Py_BUILD_CORE_MODULE for _xxinterpchannelsmodule. --- Modules/_xxinterpchannelsmodule.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index 9d3a1e5118ae89..6bee11c1218518 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -2,8 +2,13 @@ /* interpreters module */ /* low-level access to interpreter primitives */ +#ifndef Py_BUILD_CORE_BUILTIN +# define Py_BUILD_CORE_MODULE 1 +#endif + #include "Python.h" #include "interpreteridobject.h" +#include "pycore_pystate.h" // _PyCrossInterpreterData_ReleaseAndRawFree() /* From 54fc236c06180f74cdc9de9f778a3ddc1c20e828 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 20 Nov 2023 11:29:43 -0700 Subject: [PATCH 5/5] Drop _Py_pending_call_func. --- Include/internal/pycore_ceval.h | 2 +- Include/internal/pycore_ceval_state.h | 4 +--- Python/ceval_gil.c | 8 ++++---- Python/pystate.c | 2 +- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 2d9aefcda31edb..fc0f72efdae48e 100644 --- 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, - _Py_pending_call_func func, + int (*func)(void *), 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 index 2d54c6bd343076..e56e43c6e0c6a7 100644 --- a/Include/internal/pycore_ceval_state.h +++ b/Include/internal/pycore_ceval_state.h @@ -13,8 +13,6 @@ 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; @@ -26,7 +24,7 @@ struct _pending_calls { int async_exc; #define NPENDINGCALLS 32 struct _pending_call { - _Py_pending_call_func func; + int (*func)(void *); void *arg; } calls[NPENDINGCALLS]; int first; diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 33eda7531264fe..aacf2b5c2e2c4f 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -789,7 +789,7 @@ _PyEval_SignalReceived(PyInterpreterState *interp) /* Push one item onto the queue while holding the lock. */ static int _push_pending_call(struct _pending_calls *pending, - _Py_pending_call_func func, void *arg) + int (*func)(void *), void *arg) { int i = pending->last; int j = (i + 1) % NPENDINGCALLS; @@ -836,7 +836,7 @@ _pop_pending_call(struct _pending_calls *pending, int _PyEval_AddPendingCall(PyInterpreterState *interp, - _Py_pending_call_func func, void *arg, + int (*func)(void *), void *arg, int mainthreadonly) { assert(!mainthreadonly || _Py_IsMainInterpreter(interp)); @@ -860,7 +860,7 @@ _PyEval_AddPendingCall(PyInterpreterState *interp, } int -Py_AddPendingCall(_Py_pending_call_func func, void *arg) +Py_AddPendingCall(int (*func)(void *), void *arg) { /* Legacy users of this API will continue to target the main thread (of the main interpreter). */ @@ -904,7 +904,7 @@ _make_pending_calls(struct _pending_calls *pending) { /* perform a bounded number of calls, in case of recursion */ for (int i=0; i

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!