From 32466c97c06ee5812923d695195394c736eeb707 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 27 Sep 2023 13:41:06 -0600 Subject: gh-109793: Allow Switching Interpreters During Finalization (gh-109794) Essentially, we should check the thread ID rather than the thread state pointer. --- Include/cpython/pyatomic.h | 17 +++++++++++++++++ Include/internal/pycore_interp.h | 16 ++++++++++++++++ Include/internal/pycore_pystate.h | 8 ++++++-- Include/internal/pycore_runtime.h | 16 ++++++++++++++++ Lib/test/test_interpreters.py | 21 +++++++++++++++++++++ .../2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst | 4 ++++ Python/pystate.c | 17 ++++++++++++++++- 7 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst diff --git a/Include/cpython/pyatomic.h b/Include/cpython/pyatomic.h index ab18238..ce23e13 100644 --- a/Include/cpython/pyatomic.h +++ b/Include/cpython/pyatomic.h @@ -501,3 +501,20 @@ static inline void _Py_atomic_fence_release(void); #else # error "no available pyatomic implementation for this platform/compiler" #endif + + +// --- aliases --------------------------------------------------------------- + +#if SIZEOF_LONG == 8 +# define _Py_atomic_load_ulong _Py_atomic_load_uint64 +# define _Py_atomic_load_ulong_relaxed _Py_atomic_load_uint64_relaxed +# define _Py_atomic_store_ulong _Py_atomic_store_uint64 +# define _Py_atomic_store_ulong_relaxed _Py_atomic_store_uint64_relaxed +#elif SIZEOF_LONG == 4 +# define _Py_atomic_load_ulong _Py_atomic_load_uint32 +# define _Py_atomic_load_ulong_relaxed _Py_atomic_load_uint32_relaxed +# define _Py_atomic_store_ulong _Py_atomic_store_uint32 +# define _Py_atomic_store_ulong_relaxed _Py_atomic_store_uint32_relaxed +#else +# error "long must be 4 or 8 bytes in size" +#endif // SIZEOF_LONG diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index ba5764e..0912bd1 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -93,6 +93,8 @@ struct _is { and _PyInterpreterState_SetFinalizing() to access it, don't access it directly. */ _Py_atomic_address _finalizing; + /* The ID of the OS thread in which we are finalizing. */ + unsigned long _finalizing_id; struct _gc_runtime_state gc; @@ -215,9 +217,23 @@ _PyInterpreterState_GetFinalizing(PyInterpreterState *interp) { return (PyThreadState*)_Py_atomic_load_relaxed(&interp->_finalizing); } +static inline unsigned long +_PyInterpreterState_GetFinalizingID(PyInterpreterState *interp) { + return _Py_atomic_load_ulong_relaxed(&interp->_finalizing_id); +} + static inline void _PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tstate) { _Py_atomic_store_relaxed(&interp->_finalizing, (uintptr_t)tstate); + if (tstate == NULL) { + _Py_atomic_store_ulong_relaxed(&interp->_finalizing_id, 0); + } + else { + // XXX Re-enable this assert once gh-109860 is fixed. + //assert(tstate->thread_id == PyThread_get_thread_ident()); + _Py_atomic_store_ulong_relaxed(&interp->_finalizing_id, + tstate->thread_id); + } } diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 9fc8ae9..2e568f8 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -36,8 +36,12 @@ _Py_IsMainInterpreter(PyInterpreterState *interp) static inline int _Py_IsMainInterpreterFinalizing(PyInterpreterState *interp) { - return (_PyRuntimeState_GetFinalizing(interp->runtime) != NULL && - interp == &interp->runtime->_main_interpreter); + /* 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. */ + return (_PyRuntimeState_GetFinalizing(&_PyRuntime) != NULL && + interp == &_PyRuntime._main_interpreter); } diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 0ddc405..cc3a342 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -171,6 +171,8 @@ typedef struct pyruntimestate { Use _PyRuntimeState_GetFinalizing() and _PyRuntimeState_SetFinalizing() to access it, don't access it directly. */ _Py_atomic_address _finalizing; + /* The ID of the OS thread in which we are finalizing. */ + unsigned long _finalizing_id; struct pyinterpreters { PyThread_type_lock mutex; @@ -303,9 +305,23 @@ _PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) { return (PyThreadState*)_Py_atomic_load_relaxed(&runtime->_finalizing); } +static inline unsigned long +_PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) { + return _Py_atomic_load_ulong_relaxed(&runtime->_finalizing_id); +} + static inline void _PyRuntimeState_SetFinalizing(_PyRuntimeState *runtime, PyThreadState *tstate) { _Py_atomic_store_relaxed(&runtime->_finalizing, (uintptr_t)tstate); + if (tstate == NULL) { + _Py_atomic_store_ulong_relaxed(&runtime->_finalizing_id, 0); + } + else { + // XXX Re-enable this assert once gh-109860 is fixed. + //assert(tstate->thread_id == PyThread_get_thread_ident()); + _Py_atomic_store_ulong_relaxed(&runtime->_finalizing_id, + tstate->thread_id); + } } #ifdef __cplusplus diff --git a/Lib/test/test_interpreters.py b/Lib/test/test_interpreters.py index 90932c0..9c0dac7 100644 --- a/Lib/test/test_interpreters.py +++ b/Lib/test/test_interpreters.py @@ -1,5 +1,6 @@ import contextlib import os +import sys import threading from textwrap import dedent import unittest @@ -487,6 +488,26 @@ class StressTests(TestBase): pass +class FinalizationTests(TestBase): + + def test_gh_109793(self): + import subprocess + argv = [sys.executable, '-c', '''if True: + import _xxsubinterpreters as _interpreters + interpid = _interpreters.create() + raise Exception + '''] + proc = subprocess.run(argv, capture_output=True, text=True) + self.assertIn('Traceback', proc.stderr) + if proc.returncode == 0 and support.verbose: + print() + print("--- cmd unexpected succeeded ---") + print(f"stdout:\n{proc.stdout}") + print(f"stderr:\n{proc.stderr}") + print("------") + self.assertEqual(proc.returncode, 1) + + class TestIsShareable(TestBase): def test_default_shareables(self): diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst new file mode 100644 index 0000000..d2dc4c8 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst @@ -0,0 +1,4 @@ +The main thread no longer exits prematurely when a subinterpreter +is cleaned up during runtime finalization. The bug was a problem +particularly because, when triggered, the Python process would +always return with a 0 exitcode, even if it failed. diff --git a/Python/pystate.c b/Python/pystate.c index dcc6c11..570b524 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2964,11 +2964,26 @@ _PyThreadState_MustExit(PyThreadState *tstate) 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. */ + unsigned long finalizing_id = _PyRuntimeState_GetFinalizingID(&_PyRuntime); PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime); if (finalizing == NULL) { + // XXX This isn't completely safe from daemon thraeds, + // since tstate might be a dangling pointer. finalizing = _PyInterpreterState_GetFinalizing(tstate->interp); + finalizing_id = _PyInterpreterState_GetFinalizingID(tstate->interp); } - return (finalizing != NULL && finalizing != tstate); + // XXX else check &_PyRuntime._main_interpreter._initial_thread + if (finalizing == NULL) { + return 0; + } + else if (finalizing == tstate) { + return 0; + } + else if (finalizing_id == PyThread_get_thread_ident()) { + /* gh-109793: we must have switched interpreters. */ + return 0; + } + return 1; } -- cgit v0.12