Skip to content

Commit 9b79723

Browse files
ericsnowcurrentlymiss-islington
authored andcommitted
pythongh-76785: Use Pending Calls When Releasing Cross-Interpreter Data (pythongh-109556)
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>
1 parent 4a0c118 commit 9b79723

7 files changed

Lines changed: 98 additions & 67 deletions

File tree

‎Include/cpython/pystate.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,7 @@ PyAPI_FUNC(void) _PyCrossInterpreterData_Clear(
431431
PyAPI_FUNC(int) _PyObject_GetCrossInterpreterData(PyObject *, _PyCrossInterpreterData *);
432432
PyAPI_FUNC(PyObject *) _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *);
433433
PyAPI_FUNC(int) _PyCrossInterpreterData_Release(_PyCrossInterpreterData *);
434+
PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *);
434435

435436
PyAPI_FUNC(int) _PyObject_CheckCrossInterpreterData(PyObject *);
436437

‎Include/internal/pycore_ceval.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ extern void _PyEval_FiniState(struct _ceval_state *ceval);
2626
PyAPI_FUNC(void) _PyEval_SignalReceived(PyInterpreterState *interp);
2727
PyAPI_FUNC(int) _PyEval_AddPendingCall(
2828
PyInterpreterState *interp,
29-
int (*func)(void *),
29+
_Py_pending_call_func func,
3030
void *arg,
3131
int mainthreadonly);
3232
PyAPI_FUNC(void) _PyEval_SignalAsyncExc(PyInterpreterState *interp);

‎Include/internal/pycore_ceval_state.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ extern "C" {
1313
#include "pycore_gil.h" // struct _gil_runtime_state
1414

1515

16+
typedef int (*_Py_pending_call_func)(void *);
17+
1618
struct _pending_calls {
1719
int busy;
1820
PyThread_type_lock lock;
@@ -24,7 +26,7 @@ struct _pending_calls {
2426
int async_exc;
2527
#define NPENDINGCALLS 32
2628
struct _pending_call {
27-
int (*func)(void *);
29+
_Py_pending_call_func func;
2830
void *arg;
2931
} calls[NPENDINGCALLS];
3032
int first;

‎Modules/_xxinterpchannelsmodule.c‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -161,21 +161,34 @@ add_new_type(PyObject *mod, PyType_Spec *spec, crossinterpdatafunc shared)
161161
return cls;
162162
}
163163

164+
#define XID_IGNORE_EXC 1
165+
#define XID_FREE 2
166+
164167
static int
165-
_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc)
168+
_release_xid_data(_PyCrossInterpreterData *data, int flags)
166169
{
170+
int ignoreexc = flags & XID_IGNORE_EXC;
167171
PyObject *exc;
168172
if (ignoreexc) {
169173
exc = PyErr_GetRaisedException();
170174
}
171-
int res = _PyCrossInterpreterData_Release(data);
175+
int res;
176+
if (flags & XID_FREE) {
177+
res = _PyCrossInterpreterData_ReleaseAndRawFree(data);
178+
}
179+
else {
180+
res = _PyCrossInterpreterData_Release(data);
181+
}
172182
if (res < 0) {
173183
/* The owning interpreter is already destroyed. */
174184
if (ignoreexc) {
175185
// XXX Emit a warning?
176186
PyErr_Clear();
177187
}
178188
}
189+
if (flags & XID_FREE) {
190+
/* Either way, we free the data. */
191+
}
179192
if (ignoreexc) {
180193
PyErr_SetRaisedException(exc);
181194
}
@@ -367,9 +380,8 @@ static void
367380
_channelitem_clear(_channelitem *item)
368381
{
369382
if (item->data != NULL) {
370-
(void)_release_xid_data(item->data, 1);
371383
// It was allocated in _channel_send().
372-
GLOBAL_FREE(item->data);
384+
(void)_release_xid_data(item->data, XID_IGNORE_EXC & XID_FREE);
373385
item->data = NULL;
374386
}
375387
item->next = NULL;
@@ -1440,14 +1452,12 @@ _channel_recv(_channels *channels, int64_t id, PyObject **res)
14401452
PyObject *obj = _PyCrossInterpreterData_NewObject(data);
14411453
if (obj == NULL) {
14421454
assert(PyErr_Occurred());
1443-
(void)_release_xid_data(data, 1);
1444-
// It was allocated in _channel_send().
1445-
GLOBAL_FREE(data);
1455+
// It was allocated in _channel_send(), so we free it.
1456+
(void)_release_xid_data(data, XID_IGNORE_EXC | XID_FREE);
14461457
return -1;
14471458
}
1448-
int release_res = _release_xid_data(data, 0);
1449-
// It was allocated in _channel_send().
1450-
GLOBAL_FREE(data);
1459+
// It was allocated in _channel_send(), so we free it.
1460+
int release_res = _release_xid_data(data, XID_FREE);
14511461
if (release_res < 0) {
14521462
// The source interpreter has been destroyed already.
14531463
assert(PyErr_Occurred());

‎Modules/_xxsubinterpretersmodule.c‎

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -53,24 +53,17 @@ add_new_exception(PyObject *mod, const char *name, PyObject *base)
5353
add_new_exception(MOD, MODULE_NAME "." Py_STRINGIFY(NAME), BASE)
5454

5555
static int
56-
_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc)
56+
_release_xid_data(_PyCrossInterpreterData *data)
5757
{
58-
PyObject *exc;
59-
if (ignoreexc) {
60-
exc = PyErr_GetRaisedException();
61-
}
58+
PyObject *exc = PyErr_GetRaisedException();
6259
int res = _PyCrossInterpreterData_Release(data);
6360
if (res < 0) {
6461
/* The owning interpreter is already destroyed. */
6562
_PyCrossInterpreterData_Clear(NULL, data);
66-
if (ignoreexc) {
67-
// XXX Emit a warning?
68-
PyErr_Clear();
69-
}
70-
}
71-
if (ignoreexc) {
72-
PyErr_SetRaisedException(exc);
63+
// XXX Emit a warning?
64+
PyErr_Clear();
7365
}
66+
PyErr_SetRaisedException(exc);
7467
return res;
7568
}
7669

@@ -140,7 +133,7 @@ _sharednsitem_clear(struct _sharednsitem *item)
140133
PyMem_RawFree((void *)item->name);
141134
item->name = NULL;
142135
}
143-
(void)_release_xid_data(&item->data, 1);
136+
(void)_release_xid_data(&item->data);
144137
}
145138

146139
static int
@@ -169,16 +162,16 @@ typedef struct _sharedns {
169162
static _sharedns *
170163
_sharedns_new(Py_ssize_t len)
171164
{
172-
_sharedns *shared = PyMem_NEW(_sharedns, 1);
165+
_sharedns *shared = PyMem_RawCalloc(sizeof(_sharedns), 1);
173166
if (shared == NULL) {
174167
PyErr_NoMemory();
175168
return NULL;
176169
}
177170
shared->len = len;
178-
shared->items = PyMem_NEW(struct _sharednsitem, len);
171+
shared->items = PyMem_RawCalloc(sizeof(struct _sharednsitem), len);
179172
if (shared->items == NULL) {
180173
PyErr_NoMemory();
181-
PyMem_Free(shared);
174+
PyMem_RawFree(shared);
182175
return NULL;
183176
}
184177
return shared;
@@ -190,8 +183,8 @@ _sharedns_free(_sharedns *shared)
190183
for (Py_ssize_t i=0; i < shared->len; i++) {
191184
_sharednsitem_clear(&shared->items[i]);
192185
}
193-
PyMem_Free(shared->items);
194-
PyMem_Free(shared);
186+
PyMem_RawFree(shared->items);
187+
PyMem_RawFree(shared);
195188
}
196189

197190
static _sharedns *

‎Python/ceval_gil.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -789,7 +789,7 @@ _PyEval_SignalReceived(PyInterpreterState *interp)
789789
/* Push one item onto the queue while holding the lock. */
790790
static int
791791
_push_pending_call(struct _pending_calls *pending,
792-
int (*func)(void *), void *arg)
792+
_Py_pending_call_func func, void *arg)
793793
{
794794
int i = pending->last;
795795
int j = (i + 1) % NPENDINGCALLS;
@@ -836,7 +836,7 @@ _pop_pending_call(struct _pending_calls *pending,
836836

837837
int
838838
_PyEval_AddPendingCall(PyInterpreterState *interp,
839-
int (*func)(void *), void *arg,
839+
_Py_pending_call_func func, void *arg,
840840
int mainthreadonly)
841841
{
842842
assert(!mainthreadonly || _Py_IsMainInterpreter(interp));
@@ -860,7 +860,7 @@ _PyEval_AddPendingCall(PyInterpreterState *interp,
860860
}
861861

862862
int
863-
Py_AddPendingCall(int (*func)(void *), void *arg)
863+
Py_AddPendingCall(_Py_pending_call_func func, void *arg)
864864
{
865865
/* Legacy users of this API will continue to target the main thread
866866
(of the main interpreter). */
@@ -904,7 +904,7 @@ _make_pending_calls(struct _pending_calls *pending)
904904
{
905905
/* perform a bounded number of calls, in case of recursion */
906906
for (int i=0; i<NPENDINGCALLS; i++) {
907-
int (*func)(void *) = NULL;
907+
_Py_pending_call_func func = NULL;
908908
void *arg = NULL;
909909

910910
/* pop one item off the queue while holding the lock */

‎Python/pystate.c‎

Lines changed: 58 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2255,10 +2255,16 @@ _xidata_init(_PyCrossInterpreterData *data)
22552255
static inline void
22562256
_xidata_clear(_PyCrossInterpreterData *data)
22572257
{
2258-
if (data->free != NULL) {
2259-
data->free(data->data);
2258+
// _PyCrossInterpreterData only has two members that need to be
2259+
// cleaned up, if set: "data" must be freed and "obj" must be decref'ed.
2260+
// In both cases the original (owning) interpreter must be used,
2261+
// which is the caller's responsibility to ensure.
2262+
if (data->data != NULL) {
2263+
if (data->free != NULL) {
2264+
data->free(data->data);
2265+
}
2266+
data->data = NULL;
22602267
}
2261-
data->data = NULL;
22622268
Py_CLEAR(data->obj);
22632269
}
22642270

@@ -2403,40 +2409,32 @@ _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *data)
24032409
return data->new_object(data);
24042410
}
24052411

2406-
typedef void (*releasefunc)(PyInterpreterState *, void *);
2407-
2408-
static void
2409-
_call_in_interpreter(PyInterpreterState *interp, releasefunc func, void *arg)
2412+
static int
2413+
_release_xidata_pending(void *data)
24102414
{
2411-
/* We would use Py_AddPendingCall() if it weren't specific to the
2412-
* main interpreter (see bpo-33608). In the meantime we take a
2413-
* naive approach.
2414-
*/
2415-
_PyRuntimeState *runtime = interp->runtime;
2416-
PyThreadState *save_tstate = NULL;
2417-
if (interp != current_fast_get(runtime)->interp) {
2418-
// XXX Using the "head" thread isn't strictly correct.
2419-
PyThreadState *tstate = PyInterpreterState_ThreadHead(interp);
2420-
// XXX Possible GILState issues?
2421-
save_tstate = _PyThreadState_Swap(runtime, tstate);
2422-
}
2423-
2424-
// XXX Once the GIL is per-interpreter, this should be called with the
2425-
// calling interpreter's GIL released and the target interpreter's held.
2426-
func(interp, arg);
2415+
_xidata_clear((_PyCrossInterpreterData *)data);
2416+
return 0;
2417+
}
24272418

2428-
// Switch back.
2429-
if (save_tstate != NULL) {
2430-
_PyThreadState_Swap(runtime, save_tstate);
2431-
}
2419+
static int
2420+
_xidata_release_and_rawfree_pending(void *data)
2421+
{
2422+
_xidata_clear((_PyCrossInterpreterData *)data);
2423+
PyMem_RawFree(data);
2424+
return 0;
24322425
}
24332426

2434-
int
2435-
_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
2427+
static int
2428+
_xidata_release(_PyCrossInterpreterData *data, int rawfree)
24362429
{
2437-
if (data->free == NULL && data->obj == NULL) {
2430+
if ((data->data == NULL || data->free == NULL) && data->obj == NULL) {
24382431
// Nothing to release!
2439-
data->data = NULL;
2432+
if (rawfree) {
2433+
PyMem_RawFree(data);
2434+
}
2435+
else {
2436+
data->data = NULL;
2437+
}
24402438
return 0;
24412439
}
24422440

@@ -2447,15 +2445,42 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
24472445
// This function shouldn't have been called.
24482446
// XXX Someone leaked some memory...
24492447
assert(PyErr_Occurred());
2448+
if (rawfree) {
2449+
PyMem_RawFree(data);
2450+
}
24502451
return -1;
24512452
}
24522453

24532454
// "Release" the data and/or the object.
2454-
_call_in_interpreter(interp,
2455-
(releasefunc)_PyCrossInterpreterData_Clear, data);
2455+
if (interp == current_fast_get(interp->runtime)->interp) {
2456+
_xidata_clear(data);
2457+
if (rawfree) {
2458+
PyMem_RawFree(data);
2459+
}
2460+
}
2461+
else {
2462+
_Py_pending_call_func func = _release_xidata_pending;
2463+
if (rawfree) {
2464+
func = _xidata_release_and_rawfree_pending;
2465+
}
2466+
// XXX Emit a warning if this fails?
2467+
_PyEval_AddPendingCall(interp, func, data, 0);
2468+
}
24562469
return 0;
24572470
}
24582471

2472+
int
2473+
_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
2474+
{
2475+
return _xidata_release(data, 0);
2476+
}
2477+
2478+
int
2479+
_PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data)
2480+
{
2481+
return _xidata_release(data, 1);
2482+
}
2483+
24592484
/* registry of {type -> crossinterpdatafunc} */
24602485

24612486
/* For now we use a global registry of shareable classes. An

0 commit comments

Comments
 (0)