8000 gh-104341: Call _PyEval_ReleaseLock() with NULL When Finalizing the C… · python/cpython@443f605 · GitHub
[go: up one dir, main page]
Skip to content

Commit 443f605

Browse files
ericsnowcurrentlypull[bot]
ericsnowcurrently
authored and
pull[bot]
committed
gh-104341: Call _PyEval_ReleaseLock() with NULL When Finalizing the Current Thread (gh-105109)
This avoids the problematic race in drop_gil() by skipping the FORCE_SWITCHING code there for finalizing threads. (The idea for this approach came out of discussions with @markshannon.)
1 parent 7007f45 commit 443f605

File tree

4 files changed

+46
-9
lines changed

4 files changed

+46
-9
lines changed

‎Include/internal/pycore_ceval.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ extern PyStatus _PyEval_InitGIL(PyThreadState *tstate, int own_gil);
100100
extern void _PyEval_FiniGIL(PyInterpreterState *interp);
101101

102102
extern void _PyEval_AcquireLock(PyThreadState *tstate);
103-
extern void _PyEval_ReleaseLock(PyThreadState *tstate);
103+
extern void _PyEval_ReleaseLock(PyInterpreterState *, PyThreadState *);
104104
extern PyThreadState * _PyThreadState_SwapNoGIL(PyThreadState *);
105105

106106
extern void _PyEval_DeactivateOpCache(void);

‎Python/ceval_gil.c

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,15 @@ static void recreate_gil(struct _gil_runtime_state *gil)
278278
static void
279279
drop_gil(struct _ceval_state *ceval, PyThreadState *tstate)
280280
{
281+
/* If tstate is NULL, the caller is indicating that we're releasing
282+
the GIL for the last time in this thread. This is particularly
283+
relevant when the current thread state is finalizing or its
284+
interpreter is finalizing (either may be in an inconsistent
285+
state). In that case the current thread will definitely
286+
never try to acquire the GIL again. */
287+
// XXX It may be more correct to check tstate->_status.finalizing.
288+
// XXX assert(tstate == NULL || !tstate->_status.cleared);
289+
281290
struct _gil_runtime_state *gil = ceval->gil;
282291
if (!_Py_atomic_load_relaxed(&gil->locked)) {
283292
Py_FatalError("drop_gil: GIL is not locked");
@@ -298,7 +307,15 @@ drop_gil(struct _ceval_state *ceval, PyThreadState *tstate)
298307
MUTEX_UNLOCK(gil->mutex);
299308

300309
#ifdef FORCE_SWITCHING
301-
if (_Py_atomic_load_relaxed(&ceval->gil_drop_request) && tstate != NULL) {
310+
/* We check tstate first in case we might be releasing the GIL for
311+
the last time in this thread. In that case there's a possible
312+
race with tstate->interp getting deleted after gil->mutex is
313+
unlocked and before the following code runs, leading to a crash.
314+
We can use (tstate == NULL) to indicate the thread is done with
315+
the GIL, and that's the only time we might delete the
316+
interpreter, so checking tstate first prevents the crash.
317+
See https://github.com/python/cpython/issues/104341. */
318+
if (tstate != NULL && _Py_atomic_load_relaxed(&ceval->gil_drop_request)) {
302319
MUTEX_LOCK(gil->switch_mutex);
303320
/* Not switched yet => wait */
304321
if (((PyThreadState*)_Py_atomic_load_relaxed(&gil->last_holder)) == tstate)
@@ -350,6 +367,9 @@ take_gil(PyThreadState *tstate)
350367
int err = errno;
351368

352369
assert(tstate != NULL);
370+
/* We shouldn't be using a thread state that isn't viable any more. */
371+
// XXX It may be more correct to check tstate->_status.finalizing.
372+
// XXX assert(!tstate->_status.cleared);
353373

354374
if (tstate_must_exit(tstate)) {
355375
/* bpo-39877: If Py_Finalize() has been called and tstate is not the
@@ -630,10 +650,12 @@ _PyEval_AcquireLock(PyThreadState *tstate)
630650
}
631651

632652
void
633-
_PyEval_ReleaseLock(PyThreadState *tstate)
653+
_PyEval_ReleaseLock(PyInterpreterState *interp, PyThreadState *tstate)
634654
{
635-
_Py_EnsureTstateNotNULL(tstate);
636-
struct _ceval_state *ceval = &tstate->interp->ceval;
655+
/* If tstate is NULL then we do not expect the current thread
656+
to acquire the GIL ever again. */
657+
assert(tstate == NULL || tstate->interp == interp);
658+
struct _ceval_state *ceval = &interp->ceval;
637659
drop_gil(ceval, tstate);
638660
}
639661

‎Python/pylifecycle.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2035,7 +2035,7 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config)
20352035
const PyConfig *src_config;
20362036
if (save_tstate != NULL) {
20372037
// XXX Might new_interpreter() have been called without the GIL held?
2038-
_PyEval_ReleaseLock(save_tstate);
2038+
_PyEval_ReleaseLock(save_tstate->interp, save_tstate);
20392039
src_config = _PyInterpreterState_GetConfig(save_tstate->interp);
20402040
}
20412041
else

‎Python/pystate.c

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -822,6 +822,12 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
822822
p = p->next;
823823
HEAD_UNLOCK(runtime);
824824
}
825+
if (tstate->interp == interp) {
826+
/* We fix tstate->_status below when we for sure aren't using it
827+
(e.g. no longer need the GIL). */
828+
// XXX Eliminate the need to do this.
829+
tstate->_status.cleared = 0;
830+
}
825831

826832
/* It is possible that any of the objects below have a finalizer
827833
that runs Python code or otherwise relies on a thread state
@@ -886,6 +892,12 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
886892
Py_CLEAR(interp->builtins);
887893
Py_CLEAR(interp->interpreter_trampoline);
888894

895+
if (tstate->interp == interp) {
896+
/* We are now safe to fix tstate->_status.cleared. */
897+
// XXX Do this (much) earlier?
898+
tstate->_status.cleared = 1;
899+
}
900+
889901
for (int i=0; i < DICT_MAX_WATCHERS; i++) {
890902
interp->dict_state.watchers[i] = NULL;
891903
}
@@ -930,6 +942,7 @@ _PyInterpreterState_Clear(PyThreadState *tstate)
930942
}
931943

932944

945+
static inline void tstate_deactivate(PyThreadState *tstate);
933946
static void zapthreads(PyInterpreterState *interp);
934947

935948
void
@@ -943,7 +956,9 @@ PyInterpreterState_Delete(PyInterpreterState *interp)
943956
PyThreadState *tcur = current_fast_get(runtime);
944957
if (tcur != NULL && interp == tcur->interp) {
945958
/* Unset current thread. After this, many C API calls become crashy. */
946-
_PyThreadState_Swap(runtime, NULL);
959+
current_fast_clear(runtime);
960+
tstate_deactivate(tcur);
961+
_PyEval_ReleaseLock(interp, NULL);
947962
}
948963

949964
zapthreads(interp);
@@ -1567,7 +1582,7 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate)
15671582
_Py_EnsureTstateNotNULL(tstate);
15681583
tstate_delete_common(tstate);
15691584
current_fast_clear(tstate->interp->runtime);
1570-
_PyEval_ReleaseLock(tstate);
1585+
_PyEval_ReleaseLock(tstate->interp, NULL);
15711586
free_threadstate(tstate);
15721587
}
15731588

@@ -1907,7 +1922,7 @@ _PyThreadState_Swap(_PyRuntimeState *runtime, PyThreadState *newts)
19071922
{
19081923
PyThreadState *oldts = current_fast_get(runtime);
19091924
if (oldts != NULL) {
1910-
_PyEval_ReleaseLock(oldts);
1925+
_PyEval_ReleaseLock(oldts->interp, oldts);
19111926
}
19121927
_swap_thread_states(runtime, oldts, newts);
19131928
if (newts != NULL) {

0 commit comments

Comments
 (0)
0