From eb4e2ae2b8486e8ee4249218b95d94a9f0cc513e Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Sun, 8 Mar 2020 11:57:45 +0100 Subject: [PATCH] bpo-39877: Fix PyEval_RestoreThread() for daemon threads (GH-18811) * exit_thread_if_finalizing() does now access directly _PyRuntime variable, rather than using tstate->interp->runtime since tstate can be a dangling pointer after Py_Finalize() has been called. * exit_thread_if_finalizing() is now called *before* calling take_gil(). _PyRuntime.finalizing is an atomic variable, we don't need to hold the GIL to access it. * Add ensure_tstate_not_null() function to check that tstate is not NULL at runtime. Check tstate earlier. take_gil() does not longer check if tstate is NULL. Cleanup: * PyEval_RestoreThread() no longer saves/restores errno: it's already done inside take_gil(). * PyEval_AcquireLock(), PyEval_AcquireThread(), PyEval_RestoreThread() and _PyEval_EvalFrameDefault() now check if tstate is valid with the new is_tstate_valid() function which uses _PyMem_IsPtrFreed(). --- .../2020-03-06-18-30-00.bpo-39877.bzd1y0.rst | 5 ++ Python/ceval.c | 80 ++++++++++++++----- Python/ceval_gil.h | 11 ++- Python/pylifecycle.c | 4 +- 4 files changed, 74 insertions(+), 26 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-03-06-18-30-00.bpo-39877.bzd1y0.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-03-06-18-30-00.bpo-39877.bzd1y0.rst b/Misc/NEWS.d/next/Core and Builtins/2020-03-06-18-30-00.bpo-39877.bzd1y0.rst new file mode 100644 index 00000000000..d545813c107 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-03-06-18-30-00.bpo-39877.bzd1y0.rst @@ -0,0 +1,5 @@ +Fix :c:func:`PyEval_RestoreThread` random crash at exit with daemon threads. +It now accesses the ``_PyRuntime`` variable directly instead of using +``tstate->interp->runtime``, since ``tstate`` can be a dangling pointer after +:c:func:`Py_Finalize` has been called. Moreover, the daemon thread now exits +before trying to take the GIL. diff --git a/Python/ceval.c b/Python/ceval.c index 04e0824727e..42f08c4534c 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -188,6 +188,25 @@ static size_t opcache_global_misses = 0; #include "pythread.h" #include "ceval_gil.h" +static void +ensure_tstate_not_null(const char *func, PyThreadState *tstate) +{ + if (tstate == NULL) { + _Py_FatalErrorFunc(func, "current thread state is NULL"); + } +} + + +#ifndef NDEBUG +static int is_tstate_valid(PyThreadState *tstate) +{ + assert(!_PyMem_IsPtrFreed(tstate)); + assert(!_PyMem_IsPtrFreed(tstate->interp)); + return 1; +} +#endif + + int PyEval_ThreadsInitialized(void) { @@ -208,6 +227,7 @@ PyEval_InitThreads(void) PyThread_init_thread(); create_gil(gil); PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime); + ensure_tstate_not_null(__func__, tstate); take_gil(ceval, tstate); struct _pending_calls *pending = &ceval->pending; @@ -235,14 +255,26 @@ _PyEval_FiniThreads(struct _ceval_runtime_state *ceval) } } +/* This function is designed to exit daemon threads immediately rather than + taking the GIL if Py_Finalize() has been called. + + The caller must *not* hold the GIL, since this function does not release + the GIL before exiting the thread. + + When this function is called by a daemon thread after Py_Finalize() has been + called, the GIL does no longer exist. + + tstate must be non-NULL. */ static inline void exit_thread_if_finalizing(PyThreadState *tstate) { - _PyRuntimeState *runtime = tstate->interp->runtime; - /* _Py_Finalizing is protected by the GIL */ + /* bpo-39877: Access _PyRuntime directly rather than using + tstate->interp->runtime to support calls from Python daemon threads. + After Py_Finalize() has been called, tstate can be a dangling pointer: + point to PyThreadState freed memory. */ + _PyRuntimeState *runtime = &_PyRuntime; PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(runtime); if (finalizing != NULL && finalizing != tstate) { - drop_gil(&runtime->ceval, tstate); PyThread_exit_thread(); } } @@ -280,13 +312,14 @@ void PyEval_AcquireLock(void) { _PyRuntimeState *runtime = &_PyRuntime; - struct _ceval_runtime_state *ceval = &runtime->ceval; PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime); - if (tstate == NULL) { - Py_FatalError("current thread state is NULL"); - } - take_gil(ceval, tstate); + ensure_tstate_not_null(__func__, tstate); + exit_thread_if_finalizing(tstate); + assert(is_tstate_valid(tstate)); + + struct _ceval_runtime_state *ceval = &runtime->ceval; + take_gil(ceval, tstate); } void @@ -304,15 +337,18 @@ PyEval_ReleaseLock(void) void PyEval_AcquireThread(PyThreadState *tstate) { - assert(tstate != NULL); + ensure_tstate_not_null(__func__, tstate); + + exit_thread_if_finalizing(tstate); + assert(is_tstate_valid(tstate)); _PyRuntimeState *runtime = tstate->interp->runtime; struct _ceval_runtime_state *ceval = &runtime->ceval; /* Check someone has called PyEval_InitThreads() to create the lock */ assert(gil_created(&ceval->gil)); + take_gil(ceval, tstate); - exit_thread_if_finalizing(tstate); if (_PyThreadState_Swap(&runtime->gilstate, tstate) != NULL) { Py_FatalError("non-NULL old thread state"); } @@ -344,8 +380,9 @@ _PyEval_ReInitThreads(_PyRuntimeState *runtime) return; } recreate_gil(&ceval->gil); - PyThreadState *current_tstate = _PyRuntimeState_GetThreadState(runtime); - take_gil(ceval, current_tstate); + PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime); + ensure_tstate_not_null(__func__, tstate); + take_gil(ceval, tstate); struct _pending_calls *pending = &ceval->pending; pending->lock = PyThread_allocate_lock(); @@ -354,7 +391,7 @@ _PyEval_ReInitThreads(_PyRuntimeState *runtime) } /* Destroy all threads except the current one */ - _PyThreadState_DeleteExcept(runtime, current_tstate); + _PyThreadState_DeleteExcept(runtime, tstate); } /* This function is used to signal that async exceptions are waiting to be @@ -383,16 +420,16 @@ PyEval_SaveThread(void) void PyEval_RestoreThread(PyThreadState *tstate) { - assert(tstate != NULL); + ensure_tstate_not_null(__func__, tstate); + + exit_thread_if_finalizing(tstate); + assert(is_tstate_valid(tstate)); _PyRuntimeState *runtime = tstate->interp->runtime; struct _ceval_runtime_state *ceval = &runtime->ceval; assert(gil_created(&ceval->gil)); - int err = errno; take_gil(ceval, tstate); - exit_thread_if_finalizing(tstate); - errno = err; _PyThreadState_Swap(&runtime->gilstate, tstate); } @@ -750,11 +787,14 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag) PyObject **fastlocals, **freevars; PyObject *retval = NULL; /* Return value */ _PyRuntimeState * const runtime = &_PyRuntime; - PyThreadState * const tstate = _PyRuntimeState_GetThreadState(runtime); struct _ceval_runtime_state * const ceval = &runtime->ceval; _Py_atomic_int * const eval_breaker = &ceval->eval_breaker; PyCodeObject *co; + PyThreadState * const tstate = _PyRuntimeState_GetThreadState(runtime); + ensure_tstate_not_null(__func__, tstate); + assert(is_tstate_valid(tstate)); + /* when tracing we set things up so that not (instr_lb <= current_bytecode_offset < instr_ub) @@ -1242,11 +1282,11 @@ main_loop: /* Other threads may run now */ - take_gil(ceval, tstate); - /* Check if we should make a quick exit. */ exit_thread_if_finalizing(tstate); + take_gil(ceval, tstate); + if (_PyThreadState_Swap(&runtime->gilstate, tstate) != NULL) { Py_FatalError("orphan tstate"); } diff --git a/Python/ceval_gil.h b/Python/ceval_gil.h index 34d48c990c4..99d576d5615 100644 --- a/Python/ceval_gil.h +++ b/Python/ceval_gil.h @@ -180,15 +180,17 @@ drop_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate) #endif } +/* Take the GIL. + + The function saves errno at entry and restores its value at exit. + + tstate must be non-NULL. */ static void take_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate) { - if (tstate == NULL) { - Py_FatalError("take_gil: NULL tstate"); - } + int err = errno; struct _gil_runtime_state *gil = &ceval->gil; - int err = errno; MUTEX_LOCK(gil->mutex); if (!_Py_atomic_load_relaxed(&gil->locked)) { @@ -240,6 +242,7 @@ _ready: } MUTEX_UNLOCK(gil->mutex); + errno = err; } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 9e3b2572794..eaae5fdbb66 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1364,8 +1364,8 @@ Py_FinalizeEx(void) int malloc_stats = interp->config.malloc_stats; #endif - /* Remaining threads (e.g. daemon threads) will automatically exit - after taking the GIL (in PyEval_RestoreThread()). */ + /* Remaining daemon threads will automatically exit + when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyRuntimeState_SetFinalizing(runtime, tstate); runtime->initialized = 0; runtime->core_initialized = 0;