Skip to content

Commit 592a849

Browse files
[3.12] gh-76785: Use Pending Calls When Releasing Cross-Interpreter Data (gh-109556) (gh-112288)
This fixes some crashes in the _xxinterpchannels module, due to a race between interpreters. (cherry picked from commit fd7e08a)
1 parent a4aac7d commit 592a849

File tree

4 files changed

+96
-61
lines changed

4 files changed

+96
-61
lines changed

Include/internal/pycore_pystate.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,8 @@ extern PyStatus _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime);
148148
extern void _PySignal_AfterFork(void);
149149
#endif
150150

151+
PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *);
152+
151153

152154
PyAPI_FUNC(int) _PyState_AddModule(
153155
PyThreadState *tstate,

Modules/_xxinterpchannelsmodule.c

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,13 @@
22
/* interpreters module */
33
/* low-level access to interpreter primitives */
44

5+
#ifndef Py_BUILD_CORE_BUILTIN
6+
# define Py_BUILD_CORE_MODULE 1
7+
#endif
8+
59
#include "Python.h"
610
#include "interpreteridobject.h"
11+
#include "pycore_pystate.h" // _PyCrossInterpreterData_ReleaseAndRawFree()
712

813

914
/*
@@ -161,21 +166,34 @@ add_new_type(PyObject *mod, PyType_Spec *spec, crossinterpdatafunc shared)
161166
return cls;
162167
}
163168

169+
#define XID_IGNORE_EXC 1
170+
#define XID_FREE 2
171+
164172
static int
165-
_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc)
173+
_release_xid_data(_PyCrossInterpreterData *data, int flags)
166174
{
175+
int ignoreexc = flags & XID_IGNORE_EXC;
167176
PyObject *exc;
168177
if (ignoreexc) {
169178
exc = PyErr_GetRaisedException();
170179
}
171-
int res = _PyCrossInterpreterData_Release(data);
180+
int res;
181+
if (flags & XID_FREE) {
182+
res = _PyCrossInterpreterData_ReleaseAndRawFree(data);
183+
}
184+
else {
185+
res = _PyCrossInterpreterData_Release(data);
186+
}
172187
if (res < 0) {
173188
/* The owning interpreter is already destroyed. */
174189
if (ignoreexc) {
175190
// XXX Emit a warning?
176191
PyErr_Clear();
177192
}
178193
}
194+
if (flags & XID_FREE) {
195+
/* Either way, we free the data. */
196+
}
179197
if (ignoreexc) {
180198
PyErr_SetRaisedException(exc);
181199
}
@@ -367,9 +385,8 @@ static void
367385
_channelitem_clear(_channelitem *item)
368386
{
369387
if (item->data != NULL) {
370-
(void)_release_xid_data(item->data, 1);
371388
// It was allocated in _channel_send().
372-
GLOBAL_FREE(item->data);
389+
(void)_release_xid_data(item->data, XID_IGNORE_EXC & XID_FREE);
373390
item->data = NULL;
374391
}
375392
item->next = NULL;
@@ -1440,14 +1457,12 @@ _channel_recv(_channels *channels, int64_t id, PyObject **res)
14401457
PyObject *obj = _PyCrossInterpreterData_NewObject(data);
14411458
if (obj == NULL) {
14421459
assert(PyErr_Occurred());
1443-
(void)_release_xid_data(data, 1);
1444-
// It was allocated in _channel_send().
1445-
GLOBAL_FREE(data);
1460+
// It was allocated in _channel_send(), so we free it.
1461+
(void)_release_xid_data(data, XID_IGNORE_EXC | XID_FREE);
14461462
return -1;
14471463
}
1448-
int release_res = _release_xid_data(data, 0);
1449-
// It was allocated in _channel_send().
1450-
GLOBAL_FREE(data);
1464+
// It was allocated in _channel_send(), so we free it.
1465+
int release_res = _release_xid_data(data, XID_FREE);
14511466
if (release_res < 0) {
14521467
// The source interpreter has been destroyed already.
14531468
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/pystate.c

Lines changed: 58 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2259,10 +2259,16 @@ _xidata_init(_PyCrossInterpreterData *data)
22592259
static inline void
22602260
_xidata_clear(_PyCrossInterpreterData *data)
22612261
{
2262-
if (data->free != NULL) {
2263-
data->free(data->data);
2262+
// _PyCrossInterpreterData only has two members that need to be
2263+
// cleaned up, if set: "data" must be freed and "obj" must be decref'ed.
2264+
// In both cases the original (owning) interpreter must be used,
2265+
// which is the caller's responsibility to ensure.
2266+
if (data->data != NULL) {
2267+
if (data->free != NULL) {
2268+
data->free(data->data);
2269+
}
2270+
data->data = NULL;
22642271
}
2265-
data->data = NULL;
22662272
Py_CLEAR(data->obj);
22672273
}
22682274

@@ -2407,40 +2413,32 @@ _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *data)
24072413
return data->new_object(data);
24082414
}
24092415

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

2432-
// Switch back.
2433-
if (save_tstate != NULL) {
2434-
_PyThreadState_Swap(runtime, save_tstate);
2435-
}
2423+
static int
2424+
_xidata_release_and_rawfree_pending(void *data)
2425+
{
2426+
_xidata_clear((_PyCrossInterpreterData *)data);
2427+
PyMem_RawFree(data);
2428+
return 0;
24362429
}
24372430

2438-
int
2439-
_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
2431+
static int
2432+
_xidata_release(_PyCrossInterpreterData *data, int rawfree)
24402433
{
2441-
if (data->free == NULL && data->obj == NULL) {
2434+
if ((data->data == NULL || data->free == NULL) && data->obj == NULL) {
24422435
// Nothing to release!
2443-
data->data = NULL;
2436+
if (rawfree) {
2437+
PyMem_RawFree(data);
2438+
}
2439+
else {
2440+
data->data = NULL;
2441+
}
24442442
return 0;
24452443
}
24462444

@@ -2451,15 +2449,42 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
24512449
// This function shouldn't have been called.
24522450
// XXX Someone leaked some memory...
24532451
assert(PyErr_Occurred());
2452+
if (rawfree) {
2453+
PyMem_RawFree(data);
2454+
}
24542455
return -1;
24552456
}
24562457

24572458
// "Release" the data and/or the object.
2458-
_call_in_interpreter(interp,
2459-
(releasefunc)_PyCrossInterpreterData_Clear, data);
2459+
if (interp == current_fast_get(interp->runtime)->interp) {
2460+
_xidata_clear(data);
2461+
if (rawfree) {
2462+
PyMem_RawFree(data);
2463+
}
2464+
}
2465+
else {
2466+
int (*func)(void *) = _release_xidata_pending;
2467+
if (rawfree) {
2468+
func = _xidata_release_and_rawfree_pending;
2469+
}
2470+
// XXX Emit a warning if this fails?
2471+
_PyEval_AddPendingCall(interp, func, data, 0);
2472+
}
24602473
return 0;
24612474
}
24622475

2476+
int
2477+
_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
2478+
{
2479+
return _xidata_release(data, 0);
2480+
}
2481+
2482+
int
2483+
_PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data)
2484+
{
2485+
return _xidata_release(data, 1);
2486+
}
2487+
24632488
/* registry of {type -> crossinterpdatafunc} */
24642489

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

0 commit comments

Comments
 (0)