gh-112723: Call `PyThreadState_Clear()` from the correct interpreter (#112776)

The `PyThreadState_Clear()` function must only be called with the GIL
held and must be called from the same interpreter as the passed in
thread state. Otherwise, any Python objects on the thread state may be
destroyed using the wrong interpreter, leading to memory corruption.

This is also important for `Py_GIL_DISABLED` builds because free lists
will be associated with PyThreadStates and cleared in
`PyThreadState_Clear()`.

This fixes two places that called `PyThreadState_Clear()` from the wrong
interpreter and adds an assertion to `PyThreadState_Clear()`.

diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h
index 64fb4034669e1958607203775700f4a5fd79b680..a357bfa3a2606429cfe96a3c48fb48086fa6c286 100644 (file)
--- a/Include/internal/pycore_ceval.h
+++ b/Include/internal/pycore_ceval.h
@@ -124,7 +124,7 @@ _PyEval_Vector(PyThreadState *tstate,
             PyObject *kwnames);
 
 extern int _PyEval_ThreadsInitialized(void);
-extern PyStatus _PyEval_InitGIL(PyThreadState *tstate, int own_gil);
+extern void _PyEval_InitGIL(PyThreadState *tstate, int own_gil);
 extern void _PyEval_FiniGIL(PyInterpreterState *interp);
 
 extern void _PyEval_AcquireLock(PyThreadState *tstate);

diff --git a/Include/internal/pycore_pylifecycle.h b/Include/internal/pycore_pylifecycle.h
index daf7cb77dcc63ae5f27a6d0b11a7f43074f959c5..c675098685764c48481397ceef1b0424de47c89a 100644 (file)
--- a/Include/internal/pycore_pylifecycle.h
+++ b/Include/internal/pycore_pylifecycle.h
@@ -63,7 +63,7 @@ extern void _PyArg_Fini(void);
 extern void _Py_FinalizeAllocatedBlocks(_PyRuntimeState *);
 
 extern PyStatus _PyGILState_Init(PyInterpreterState *interp);
-extern PyStatus _PyGILState_SetTstate(PyThreadState *tstate);
+extern void _PyGILState_SetTstate(PyThreadState *tstate);
 extern void _PyGILState_Fini(PyInterpreterState *interp);
 
 extern void _PyGC_DumpShutdownStats(PyInterpreterState *interp);

diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c
index 4bb54c93b0a61b8f9d8d011b145a4ad1439545af..4e9e13457a9eb3f9dccdde6a3f70cd8537f9b8c6 100644 (file)
--- a/Modules/_xxsubinterpretersmodule.c
+++ b/Modules/_xxsubinterpretersmodule.c
@@ -547,7 +547,9 @@ interp_create(PyObject *self, PyObject *args, PyObject *kwds)
         return NULL;
     }
 
+    PyThreadState_Swap(tstate);
     PyThreadState_Clear(tstate);
+    PyThreadState_Swap(save_tstate);
     PyThreadState_Delete(tstate);
 
     _PyInterpreterState_RequireIDRef(interp, 1);

diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c
index 7581daa55b5e467fc55ad50ff46c2ba2d95a5e58..d70abbc27606b46fd71a86395e606df1bf170e51 100644 (file)
--- a/Python/ceval_gil.c
+++ b/Python/ceval_gil.c
@@ -447,7 +447,7 @@ init_own_gil(PyInterpreterState *interp, struct _gil_runtime_state *gil)
     interp->ceval.own_gil = 1;
 }
 
-PyStatus
+void
 _PyEval_InitGIL(PyThreadState *tstate, int own_gil)
 {
     assert(tstate->interp->ceval.gil == NULL);
@@ -466,8 +466,6 @@ _PyEval_InitGIL(PyThreadState *tstate, int own_gil)
 
     // Lock the GIL and mark the current thread as attached.
     _PyThreadState_Attach(tstate);
-
-    return _PyStatus_OK();
 }
 
 void

diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index b5c7dc5da596def8eeb99bc5f7cac13c5264705b..0ec29846b0850b2f9a109e78fbd7763efa838c78 100644 (file)
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -576,44 +576,33 @@ init_interp_settings(PyInterpreterState *interp,
         interp->feature_flags |= Py_RTFLAGS_MULTI_INTERP_EXTENSIONS;
     }
 
-    /* We check "gil" in init_interp_create_gil(). */
+    switch (config->gil) {
+    case PyInterpreterConfig_DEFAULT_GIL: break;
+    case PyInterpreterConfig_SHARED_GIL: break;
+    case PyInterpreterConfig_OWN_GIL: break;
+    default:
+        return _PyStatus_ERR("invalid interpreter config 'gil' value");
+    }
 
     return _PyStatus_OK();
 }
 
 
-static PyStatus
+static void
 init_interp_create_gil(PyThreadState *tstate, int gil)
 {
-    PyStatus status;
-
     /* finalize_interp_delete() comment explains why _PyEval_FiniGIL() is
        only called here. */
     // XXX This is broken with a per-interpreter GIL.
     _PyEval_FiniGIL(tstate->interp);
 
     /* Auto-thread-state API */
-    status = _PyGILState_SetTstate(tstate);
-    if (_PyStatus_EXCEPTION(status)) {
-        return status;
-    }
+    _PyGILState_SetTstate(tstate);
 
-    int own_gil;
-    switch (gil) {
-    case PyInterpreterConfig_DEFAULT_GIL: own_gil = 0; break;
-    case PyInterpreterConfig_SHARED_GIL: own_gil = 0; break;
-    case PyInterpreterConfig_OWN_GIL: own_gil = 1; break;
-    default:
-        return _PyStatus_ERR("invalid interpreter config 'gil' value");
-    }
+    int own_gil = (gil == PyInterpreterConfig_OWN_GIL);
 
     /* Create the GIL and take it */
-    status = _PyEval_InitGIL(tstate, own_gil);
-    if (_PyStatus_EXCEPTION(status)) {
-        return status;
-    }
-
-    return _PyStatus_OK();
+    _PyEval_InitGIL(tstate, own_gil);
 }
 
 
@@ -657,10 +646,7 @@ pycore_create_interpreter(_PyRuntimeState *runtime,
     }
     _PyThreadState_Bind(tstate);
 
-    status = init_interp_create_gil(tstate, config.gil);
-    if (_PyStatus_EXCEPTION(status)) {
-        return status;
-    }
+    init_interp_create_gil(tstate, config.gil);
 
     *tstate_p = tstate;
     return _PyStatus_OK();
@@ -2099,28 +2085,21 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config)
         return _PyStatus_OK();
     }
 
-    PyThreadState *tstate = _PyThreadState_New(interp,
-                                               _PyThreadState_WHENCE_INTERP);
-    if (tstate == NULL) {
-        PyInterpreterState_Delete(interp);
-        *tstate_p = NULL;
-        return _PyStatus_OK();
-    }
-    _PyThreadState_Bind(tstate);
-
+    // XXX Might new_interpreter() have been called without the GIL held?
     PyThreadState *save_tstate = _PyThreadState_GET();
-    int has_gil = 0;
+    PyThreadState *tstate = NULL;
 
     /* From this point until the init_interp_create_gil() call,
        we must not do anything that requires that the GIL be held
        (or otherwise exist).  That applies whether or not the new
        interpreter has its own GIL (e.g. the main interpreter). */
+    if (save_tstate != NULL) {
+        _PyThreadState_Detach(save_tstate);
+    }
 
     /* Copy the current interpreter config into the new interpreter */
     const PyConfig *src_config;
     if (save_tstate != NULL) {
-        // XXX Might new_interpreter() have been called without the GIL held?
-        _PyThreadState_Detach(save_tstate);
         src_config = _PyInterpreterState_GetConfig(save_tstate->interp);
     }
     else
@@ -2142,11 +2121,14 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config)
         goto error;
     }
 
-    status = init_interp_create_gil(tstate, config->gil);
-    if (_PyStatus_EXCEPTION(status)) {
+    tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_INTERP);
+    if (tstate == NULL) {
+        status = _PyStatus_NO_MEMORY();
         goto error;
     }
-    has_gil = 1;
+
+    _PyThreadState_Bind(tstate);
+    init_interp_create_gil(tstate, config->gil);
 
     /* No objects have been created yet. */
 
@@ -2165,16 +2147,14 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config)
 
 error:
     *tstate_p = NULL;
-
-    /* Oops, it didn't work.  Undo it all. */
-    if (has_gil) {
+    if (tstate != NULL) {
+        PyThreadState_Clear(tstate);
         _PyThreadState_Detach(tstate);
+        PyThreadState_Delete(tstate);
     }
     if (save_tstate != NULL) {
         _PyThreadState_Attach(save_tstate);
     }
-    PyThreadState_Clear(tstate);
-    PyThreadState_Delete(tstate);
     PyInterpreterState_Delete(interp);
 
     return status;

diff --git a/Python/pystate.c b/Python/pystate.c
index f0c5259967d90795a66e2797b12905c86ec230b3..e18eb0186d0010317d3d0371e40659f44834ac07 100644 (file)
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -1454,6 +1454,7 @@ void
 PyThreadState_Clear(PyThreadState *tstate)
 {
     assert(tstate->_status.initialized && !tstate->_status.cleared);
+    assert(current_fast_get(&_PyRuntime)->interp == tstate->interp);
     // XXX assert(!tstate->_status.bound || tstate->_status.unbound);
     tstate->_status.finalizing = 1;  // just in case
 
@@ -2150,7 +2151,7 @@ _PyGILState_Fini(PyInterpreterState *interp)
 
 
 // XXX Drop this.
-PyStatus
+void
 _PyGILState_SetTstate(PyThreadState *tstate)
 {
     /* must init with valid states */
@@ -2160,7 +2161,7 @@ _PyGILState_SetTstate(PyThreadState *tstate)
     if (!_Py_IsMainInterpreter(tstate->interp)) {
         /* Currently, PyGILState is shared by all interpreters. The main
          * interpreter is responsible to initialize it. */
-        return _PyStatus_OK();
+        return;
     }
 
 #ifndef NDEBUG
@@ -2170,8 +2171,6 @@ _PyGILState_SetTstate(PyThreadState *tstate)
     assert(gilstate_tss_get(runtime) == tstate);
     assert(tstate->gilstate_counter == 1);
 #endif
-
-    return _PyStatus_OK();
 }
 
 PyInterpreterState *