Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d2be5c7

Browse filesBrowse files
miss-islingtonmiss-islington
authored
[3.12] gh-104341: Call _PyEval_ReleaseLock() with NULL When Finalizing the Current Thread (gh-105109) (gh-105209)
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.) (cherry picked from commit 3698fda) Co-authored-by: Eric Snow ericsnowcurrently@gmail.com
1 parent c38ceb0 commit d2be5c7
Copy full SHA for d2be5c7

File tree

4 files changed

+46
-9
lines changed
Filter options

4 files changed

+46
-9
lines changed

‎Include/internal/pycore_ceval.h

Copy file name to clipboardExpand all lines: Include/internal/pycore_ceval.h
+1-1Lines 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

Copy file name to clipboardExpand all lines: Python/ceval_gil.c
+26-4Lines 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
@@ -625,10 +645,12 @@ _PyEval_AcquireLock(PyThreadState *tstate)
625645
}
626646

627647
void
628-
_PyEval_ReleaseLock(PyThreadState *tstate)
648+
_PyEval_ReleaseLock(PyInterpreterState *interp, PyThreadState *tstate)
629649
{
630-
_Py_EnsureTstateNotNULL(tstate);
631-
struct _ceval_state *ceval = &tstate->interp->ceval;
650+
/* If tstate is NULL then we do not expect the current thread
651+
to acquire the GIL ever again. */
652+
assert(tstate == NULL || tstate->interp == interp);
653+
struct _ceval_state *ceval = &interp->ceval;
632654
drop_gil(ceval, tstate);
633655
}
634656

‎Python/pylifecycle.c

Copy file name to clipboardExpand all lines: Python/pylifecycle.c
+1-1Lines 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

Copy file name to clipboardExpand all lines: Python/pystate.c
+18-3Lines 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)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.