From 30c8135a12b94fc3ddf42f2df41ea1278e9bccf7 Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Wed, 14 Jun 2023 16:43:34 -0700 Subject: [PATCH 01/15] gh-87135: Hang non-main threads that attempt to acquire the GIL during finalization --- Doc/c-api/init.rst | 68 ++++++++++++++----- Include/internal/pycore_pythread.h | 8 +++ Include/pythread.h | 21 +++++- Lib/test/test_threading.py | 64 +++++++++++++++++ ...2-08-05-19-41-20.gh-issue-87135.SCNBYj.rst | 15 ++++ .../2021-09-22-14-19-02.bpo-42969.8Z2mth.rst | 3 + Modules/_testcapimodule.c | 28 ++++++++ Python/ceval_gil.c | 12 ++-- Python/thread_nt.h | 8 +++ Python/thread_pthread.h | 8 +++ 10 files changed, 213 insertions(+), 22 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst create mode 100644 Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 1dab0af2659b4e..c2a62764ae4ac6 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -401,7 +401,11 @@ Initializing and finalizing the interpreter freed. Some memory allocated by extension modules may not be freed. Some extensions may not work properly if their initialization routine is called more than once; this can happen if an application calls :c:func:`Py_Initialize` and - :c:func:`Py_FinalizeEx` more than once. + :c:func:`Py_FinalizeEx` more than once. :c:func:`Py_FinalizeEx` must not be + called recursively from within itself. Therefore, it must not be called by any + code that may be run as part of the interpreter shutdown process, such as + :py:mod:`atexit` handlers, object finalizers, or any code that may be run while + flushing the stdout and stderr files. .. audit-event:: cpython._PySys_ClearAuditHooks "" c.Py_FinalizeEx @@ -804,6 +808,29 @@ thread, where the CPython global runtime was originally initialized. The only exception is if :c:func:`exec` will be called immediately after. +.. _cautions-regarding-runtime-finalization: + +Cautions regarding runtime finalization +--------------------------------------- + +In the late stage of :term:`interpreter shutdown`, after attempting to wait for +non-daemon threads to exit (though this can be interrupted by +:class:`KeyboardInterrupt`) and running the :mod:`atexit` functions, the runtime +is marked as *finalizing*: :c:func:`_Py_IsFinalizing` and +:func:`sys.is_finalizing` return true. At this point, only the *finalization +thread* that initiated finalization (typically the main thread) is allowed to +acquire the :term:`GIL`. + +If any thread, other than the finalization thread, attempts to acquire the GIL +during finalization, either explicitly via :c:func:`PyGILState_Ensure`, +:c:macro:`Py_END_ALLOW_THREADS`, :c:func:`PyEval_AcquireThread`, or +:c:func:`PyEval_AcquireLock`, or implicitly when the interpreter attempts to +reacquire it after having yielded it, the thread enters a permanently blocked +state where it remains until the program exits. In most cases this is harmless, +but this can result in deadlock if a later stage of finalization attempts to +acquire a lock owned by the blocked thread, or otherwise waits on the blocked +thread. + High-level API -------------- @@ -847,11 +874,14 @@ code, or when embedding the Python interpreter: ensues. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. + + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. .. c:function:: PyThreadState* PyThreadState_Get() @@ -893,11 +923,14 @@ with sub-interpreters: to call arbitrary Python code. Failure is a fatal error. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. + + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. .. c:function:: void PyGILState_Release(PyGILState_STATE) @@ -1175,17 +1208,20 @@ All of the following functions must be called after :c:func:`Py_Initialize`. If this thread already has the lock, deadlock ensues. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. .. versionchanged:: 3.8 Updated to be consistent with :c:func:`PyEval_RestoreThread`, :c:func:`Py_END_ALLOW_THREADS`, and :c:func:`PyGILState_Ensure`, and terminate the current thread if called while the interpreter is finalizing. + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. + :c:func:`PyEval_RestoreThread` is a higher-level function which is always available (even when threads have not been initialized). diff --git a/Include/internal/pycore_pythread.h b/Include/internal/pycore_pythread.h index f53921494c158f..3e297c4a1a05aa 100644 --- a/Include/internal/pycore_pythread.h +++ b/Include/internal/pycore_pythread.h @@ -74,6 +74,14 @@ struct _pythread_runtime_state { #endif }; +/* Hangs the thread indefinitely without exiting it. + * + * bpo-42969: There is no safe way to exit a thread other than returning + * normally from its start function. This is used during finalization in lieu + * of actually exiting the thread. Since the program is expected to terminate + * soon anyway, it does not matter if the thread stack stays around until then. + */ +void _Py_NO_RETURN PyThread_hang_thread(void); #ifdef __cplusplus } diff --git a/Include/pythread.h b/Include/pythread.h index 63714437c496b7..c88302dd1041b6 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -17,7 +17,26 @@ typedef enum PyLockStatus { PyAPI_FUNC(void) PyThread_init_thread(void); PyAPI_FUNC(unsigned long) PyThread_start_new_thread(void (*)(void *), void *); -PyAPI_FUNC(void) _Py_NO_RETURN PyThread_exit_thread(void); +/* Terminates the current thread. + * + * WARNING: This function is only safe to call if all functions in the full call + * stack are written to safely allow it. Additionally, the behavior is + * platform-dependent. This function should be avoided, and is no longer called + * by Python itself. It is retained only for compatibility with existing C + * extension code. + * + * With pthreads, calls `pthread_exit` which attempts to unwind the stack and + * call C++ destructors. If a `noexcept` function is reached, the program is + * terminated. + * + * On Windows, calls `_endthreadex` which kills the thread without calling C++ + * destructors. + * + * In either case there is a risk of invalid references remaining to data on the + * thread stack. + */ +Py_DEPRECATED(3.12) PyAPI_FUNC(void) _Py_NO_RETURN PyThread_exit_thread(void); + PyAPI_FUNC(unsigned long) PyThread_get_thread_ident(void); #if (defined(__APPLE__) || defined(__linux__) || defined(_WIN32) \ diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 9e4972ecb640df..68622707046f47 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1037,6 +1037,70 @@ def exit_handler(): self.assertEqual(out, b'') self.assertIn(b"can't create new thread at interpreter shutdown", err) + @cpython_only + def test_finalize_daemon_thread_hang(self): + # bpo-42969: tests that daemon threads hang during finalization + script = textwrap.dedent(''' + import os + import sys + import threading + import time + import _testcapi + + lock = threading.Lock() + lock.acquire() + thread_started_event = threading.Event() + def thread_func(): + try: + thread_started_event.set() + _testcapi.finalize_thread_hang(lock.acquire) + finally: + # Control must not reach here. + os._exit(2) + + t = threading.Thread(target=thread_func) + t.daemon = True + t.start() + thread_started_event.wait() + # Sleep to ensure daemon thread is blocked on `lock.acquire` + # + # Note: This test is designed so that in the unlikely case that + # `0.1` seconds is not sufficient time for the thread to become + # blocked on `lock.acquire`, the test will still pass, it just + # won't be properly testing the thread behavior during + # finalization. + time.sleep(0.1) + + def run_during_finalization(): + # Wake up daemon thread + lock.release() + # Sleep to give the daemon thread time to crash if it is going + # to. + # + # Note: If due to an exceptionally slow execution this delay is + # insufficient, the test will still pass but will simply be + # ineffective as a test. + time.sleep(0.1) + # If control reaches here, the test succeeded. + os._exit(0) + + # Replace sys.stderr.flush as a way to run code during finalization + orig_flush = sys.stderr.flush + def do_flush(*args, **kwargs): + orig_flush(*args, **kwargs) + if not sys.is_finalizing: + return + sys.stderr.flush = orig_flush + run_during_finalization() + + sys.stderr.flush = do_flush + + # If the follow exit code is retained, `run_during_finalization` + # did not run. + sys.exit(1) + ''') + assert_python_ok("-c", script) + class ThreadJoinOnShutdown(BaseTestCase): def _run_and_join(self, script): diff --git a/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst b/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst new file mode 100644 index 00000000000000..6387d69bc267c6 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst @@ -0,0 +1,15 @@ +Attempting to acquire the GIL after runtime finalization has begun in a +different thread now causes the thread to hang rather than terminate, which +avoids potential crashes or memory corruption caused by attempting to +terminate a thread that is running code not specifically designed to support +termination. In most cases this hanging is harmless since the process will +soon exit anyway. + +The ``PyThread_exit_thread`` function is now deprecated. Its behavior is +inconsistent across platforms, and it can only be used safely in the +unlikely case that every function in the entire call stack has been designed +to support the platform-dependent termination mechanism. It is recommended +that users of this function change their design to not require thread +termination. In the unlikely case that thread termination is needed and can +be done safely, users may migrate to calling platform-specific APIs such as +``pthread_exit`` (POSIX) or ``_endthreadex`` (Windows) directly. diff --git a/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst b/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst new file mode 100644 index 00000000000000..e5e03baa6ac0df --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst @@ -0,0 +1,3 @@ +Daemon threads and other threads not created by Python are now paused rather +than unsafely terminated if they attempt to acquire the GIL during Python +finalization. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 35687b49f24a0d..fb21bd157c964c 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3324,6 +3324,33 @@ test_atexit(PyObject *self, PyObject *Py_UNUSED(args)) Py_RETURN_NONE; } +// Used by `finalize_thread_hang`. +#ifdef _POSIX_THREADS +static void finalize_thread_hang_cleanup_callback(void *Py_UNUSED(arg)) { + // Should not reach here. + assert(0 && "pthread thread termination was triggered unexpectedly"); +} +#endif + +// Tests that finalization does not trigger pthread cleanup. +// +// Must be called with a single nullary callable function that should block +// (with GIL released) until finalization is in progress. +static PyObject * +finalize_thread_hang(PyObject *self, PyObject *arg) +{ +#ifdef _POSIX_THREADS + pthread_cleanup_push(finalize_thread_hang_cleanup_callback, NULL); +#endif + PyObject_CallNoArgs(arg); + // Should not reach here. + Py_FatalError("thread unexpectedly did not hang"); +#ifdef _POSIX_THREADS + pthread_cleanup_pop(0); +#endif + Py_RETURN_NONE; +} + static PyMethodDef TestMethods[] = { {"set_errno", set_errno, METH_VARARGS}, @@ -3468,6 +3495,7 @@ static PyMethodDef TestMethods[] = { {"function_get_kw_defaults", function_get_kw_defaults, METH_O, NULL}, {"function_set_kw_defaults", function_set_kw_defaults, METH_VARARGS, NULL}, {"test_atexit", test_atexit, METH_NOARGS}, + {"finalize_thread_hang", finalize_thread_hang, METH_O, NULL}, {NULL, NULL} /* sentinel */ }; diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index bb1279f46cf9f7..15710305a653f5 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -7,6 +7,7 @@ #include "pycore_initconfig.h" // _PyStatus_OK() #include "pycore_interp.h" // _Py_RunGC() #include "pycore_pymem.h" // _PyMem_IsPtrFreed() +#include "pycore_pythread.h" // PyThread_hang_thread() /* Notes about the implementation: @@ -382,7 +383,7 @@ take_gil(PyThreadState *tstate) This code path can be reached by a daemon thread after Py_Finalize() completes. In this case, tstate is a dangling pointer: points to PyThreadState freed memory. */ - PyThread_exit_thread(); + PyThread_hang_thread(); } assert(is_tstate_valid(tstate)); @@ -424,7 +425,9 @@ take_gil(PyThreadState *tstate) if (drop_requested) { RESET_GIL_DROP_REQUEST(interp); } - PyThread_exit_thread(); + // gh-87135: hang the thread as *thread_exit() is not a safe + // API. It lacks stack unwind and local variable destruction. + PyThread_hang_thread(); } assert(is_tstate_valid(tstate)); @@ -455,7 +458,7 @@ take_gil(PyThreadState *tstate) if (tstate_must_exit(tstate)) { /* bpo-36475: If Py_Finalize() has been called and tstate is not - the thread which called Py_Finalize(), exit immediately the + the thread which called Py_Finalize(), bpo-42969: hang the thread. This code path can be reached by a daemon thread which was waiting @@ -463,7 +466,7 @@ take_gil(PyThreadState *tstate) wait_for_thread_shutdown() from Py_Finalize(). */ MUTEX_UNLOCK(gil->mutex); drop_gil(ceval, tstate); - PyThread_exit_thread(); + PyThread_hang_thread(); } assert(is_tstate_valid(tstate)); @@ -1123,4 +1126,3 @@ _Py_HandlePending(PyThreadState *tstate) return 0; } - diff --git a/Python/thread_nt.h b/Python/thread_nt.h index 26f441bd6d3c56..846f79a6ab7e5b 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -257,6 +257,14 @@ PyThread_exit_thread(void) _endthreadex(0); } +void _Py_NO_RETURN +PyThread_hang_thread(void) +{ + while (1) { + SleepEx(INFINITE, TRUE); + } +} + /* * Lock support. It has to be implemented as semaphores. * I [Dag] tried to implement it with mutex but I could find a way to diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 76d6f3bcdf9c40..6e8a9c2bbab0fd 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -359,6 +359,14 @@ PyThread_exit_thread(void) pthread_exit(0); } +void _Py_NO_RETURN +PyThread_hang_thread(void) +{ + while (1) { + pause(); + } +} + #ifdef USE_SEMAPHORES /* From da37473a4ae870eae2b04e9a220163ffd8da7d3b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 06:20:30 +0000 Subject: [PATCH 02/15] docs fixups --- .../Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst b/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst deleted file mode 100644 index e5e03baa6ac0df..00000000000000 --- a/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst +++ /dev/null @@ -1,3 +0,0 @@ -Daemon threads and other threads not created by Python are now paused rather -than unsafely terminated if they attempt to acquire the GIL during Python -finalization. From 3ec84f6fa3c6c698d95937d94f0d6a63a5329ce7 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 06:21:44 +0000 Subject: [PATCH 03/15] doc fixup, use the new next versionchanged docs feature. --- Doc/c-api/init.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 913cb38559951f..cd31ebb1b9d4aa 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -1417,7 +1417,7 @@ All of the following functions must be called after :c:func:`Py_Initialize`. :c:func:`Py_END_ALLOW_THREADS`, and :c:func:`PyGILState_Ensure`, and terminate the current thread if called while the interpreter is finalizing. - .. versionchanged:: 3.12 + .. versionchanged:: next Hangs the current thread, rather than terminating it, if called while the interpreter is finalizing. From 2bc54ca5adfe6905c31a1157e05721720e3ab817 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 06:23:39 +0000 Subject: [PATCH 04/15] deprecated in 3.14 --- Include/pythread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/pythread.h b/Include/pythread.h index 2ed28750914971..b3d3b665688aa4 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -35,7 +35,7 @@ PyAPI_FUNC(unsigned long) PyThread_start_new_thread(void (*)(void *), void *); * In either case there is a risk of invalid references remaining to data on the * thread stack. */ -Py_DEPRECATED(3.12) PyAPI_FUNC(void) _Py_NO_RETURN PyThread_exit_thread(void); +Py_DEPRECATED(3.14) PyAPI_FUNC(void) _Py_NO_RETURN PyThread_exit_thread(void); PyAPI_FUNC(unsigned long) PyThread_get_thread_ident(void); From 80d2c49b41ca28888f746cc34d677de8c504de5b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 06:31:00 +0000 Subject: [PATCH 05/15] use the gh- ref instead of bpo- everywhere. --- Lib/test/test_threading.py | 2 +- Python/ceval_gil.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index ab19eb093bc29d..a13db4984ef850 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1173,7 +1173,7 @@ def __del__(self): @cpython_only def test_finalize_daemon_thread_hang(self): - # bpo-42969: tests that daemon threads hang during finalization + # gh-87135: tests that daemon threads hang during finalization script = textwrap.dedent(''' import os import sys diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 6ddd5be0ea355f..7e1d26dd86bdfc 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -386,7 +386,7 @@ take_gil(PyThreadState *tstate) if (_PyThreadState_MustExit(tstate)) { /* bpo-36475: If Py_Finalize() has been called and tstate is not - the thread which called Py_Finalize(), bpo-42969: hang the + the thread which called Py_Finalize(), gh-87135: hang the thread. This code path can be reached by a daemon thread which was waiting From 9b1eaf679003f3a21c72eaae192bc9d6f07575c1 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 18:10:58 +0000 Subject: [PATCH 06/15] Remove wrong comments, improve others. --- Python/ceval_gil.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index 7e1d26dd86bdfc..c4067e8dff6657 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -279,9 +279,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int final_release) The function saves errno at entry and restores its value at exit. - tstate must be non-NULL. - - Returns 1 if the GIL was acquired, or 0 if not. */ + tstate must be non-NULL. */ static void take_gil(PyThreadState *tstate) { @@ -294,11 +292,17 @@ take_gil(PyThreadState *tstate) if (_PyThreadState_MustExit(tstate)) { /* bpo-39877: If Py_Finalize() has been called and tstate is not the - thread which called Py_Finalize(), exit immediately the thread. + thread which called Py_Finalize(), this thread cannot continue. This code path can be reached by a daemon thread after Py_Finalize() completes. In this case, tstate is a dangling pointer: points to - PyThreadState freed memory. */ + PyThreadState freed memory. + + This used to call a *thread_exit API, but that was not safe as it + lacks stack unwinding and local variable destruction important to + C++. gh-87135: The best that can be done is to hang the thread as + the public APIs calling this have no error reporting mechanism (!). + */ PyThread_hang_thread(); } From cf76777314cb3bc2a85ef51342e9fe52a9aefd93 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 18:16:42 +0000 Subject: [PATCH 07/15] expand the comment. --- Include/internal/pycore_pythread.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Include/internal/pycore_pythread.h b/Include/internal/pycore_pythread.h index dc7eeef38f3f2a..a1e084cf67d58d 100644 --- a/Include/internal/pycore_pythread.h +++ b/Include/internal/pycore_pythread.h @@ -159,6 +159,10 @@ PyAPI_FUNC(int) PyThread_detach_thread(PyThread_handle_t); * normally from its start function. This is used during finalization in lieu * of actually exiting the thread. Since the program is expected to terminate * soon anyway, it does not matter if the thread stack stays around until then. + * + * This is unfortunate for embedders who may not be terminating their process + * when they're done with the interpreter, but our C API design does not allow + * for safely exiting threads attempting to re-enter Python post finalization. */ void _Py_NO_RETURN PyThread_hang_thread(void); From 2dae315aad7fd599b87cb84fe7c1f9ff82de0f60 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 26 Sep 2024 21:08:04 +0000 Subject: [PATCH 08/15] more docs --- Doc/c-api/init.rst | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index cd31ebb1b9d4aa..3225f508495983 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -981,11 +981,19 @@ If any thread, other than the finalization thread, attempts to acquire the GIL during finalization, either explicitly via :c:func:`PyGILState_Ensure`, :c:macro:`Py_END_ALLOW_THREADS`, :c:func:`PyEval_AcquireThread`, or :c:func:`PyEval_AcquireLock`, or implicitly when the interpreter attempts to -reacquire it after having yielded it, the thread enters a permanently blocked -state where it remains until the program exits. In most cases this is harmless, -but this can result in deadlock if a later stage of finalization attempts to -acquire a lock owned by the blocked thread, or otherwise waits on the blocked -thread. +reacquire it after having yielded it, the thread enters **a permanently blocked +state** where it remains until the program exits. In most cases this is +harmless, but this can result in deadlock if a later stage of finalization +attempts to acquire a lock owned by the blocked thread, or otherwise waits on +the blocked thread. + +Gross? Yes. This prevents random crashes and/or unexpectedly skipped C++ +finalizations further up the call stack when such threads were forcably exited +here in CPython 3.13 and earlier. The CPython runtime GIL acquiring C APIs +have never had any error reporting or handling expectations at GIL acquisition +time that would've allowed for graceful exit from this situation. Changing that +would require new stable C APIs and rewriting the majority of C code in the +CPython ecosystem to use those with error handling. High-level API From 28118a0ab1f7f928face78b24692c0a053faf760 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Fri, 27 Sep 2024 00:03:57 +0000 Subject: [PATCH 09/15] update a comment --- Python/pylifecycle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 27faf723745c21..bc6eb1ff80cd63 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2020,7 +2020,7 @@ _Py_Finalize(_PyRuntimeState *runtime) /* Ensure that remaining threads are detached */ _PyEval_StopTheWorldAll(runtime); - /* Remaining daemon threads will automatically exit + /* Remaining daemon threads will be trapped in PyThread_hang_thread when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(tstate->interp, tstate); _PyRuntimeState_SetFinalizing(runtime, tstate); From 87249ab96357556a266cf7346e2af56c92aa0efa Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 1 Oct 2024 22:30:05 +0000 Subject: [PATCH 10/15] address review comments. --- Doc/c-api/init.rst | 2 +- Include/pythread.h | 8 ++++---- Modules/_testcapimodule.c | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 3225f508495983..0ed3f77d84be97 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -988,7 +988,7 @@ attempts to acquire a lock owned by the blocked thread, or otherwise waits on the blocked thread. Gross? Yes. This prevents random crashes and/or unexpectedly skipped C++ -finalizations further up the call stack when such threads were forcably exited +finalizations further up the call stack when such threads were forcibly exited here in CPython 3.13 and earlier. The CPython runtime GIL acquiring C APIs have never had any error reporting or handling expectations at GIL acquisition time that would've allowed for graceful exit from this situation. Changing that diff --git a/Include/pythread.h b/Include/pythread.h index b3d3b665688aa4..82247daf8e0aa0 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -17,7 +17,7 @@ typedef enum PyLockStatus { PyAPI_FUNC(void) PyThread_init_thread(void); PyAPI_FUNC(unsigned long) PyThread_start_new_thread(void (*)(void *), void *); -/* Terminates the current thread. +/* Terminates the current thread. Considered unsafe. * * WARNING: This function is only safe to call if all functions in the full call * stack are written to safely allow it. Additionally, the behavior is @@ -25,9 +25,9 @@ PyAPI_FUNC(unsigned long) PyThread_start_new_thread(void (*)(void *), void *); * by Python itself. It is retained only for compatibility with existing C * extension code. * - * With pthreads, calls `pthread_exit` which attempts to unwind the stack and - * call C++ destructors. If a `noexcept` function is reached, the program is - * terminated. + * With pthreads, calls `pthread_exit` causes some libcs (glibc?) to attempt to + * unwind the stack and call C++ destructors; if a `noexcept` function is + * reached, they may terminate the process. Others (macOS) do unwinding. * * On Windows, calls `_endthreadex` which kills the thread without calling C++ * destructors. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 36637e5b925dca..0abe30cfdccc78 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3333,7 +3333,7 @@ test_critical_sections(PyObject *module, PyObject *Py_UNUSED(args)) #ifdef _POSIX_THREADS static void finalize_thread_hang_cleanup_callback(void *Py_UNUSED(arg)) { // Should not reach here. - assert(0 && "pthread thread termination was triggered unexpectedly"); + Py_FatalError("pthread thread termination was triggered unexpectedly"); } #endif @@ -3342,12 +3342,12 @@ static void finalize_thread_hang_cleanup_callback(void *Py_UNUSED(arg)) { // Must be called with a single nullary callable function that should block // (with GIL released) until finalization is in progress. static PyObject * -finalize_thread_hang(PyObject *self, PyObject *arg) +finalize_thread_hang(PyObject *self, PyObject *callback) { #ifdef _POSIX_THREADS pthread_cleanup_push(finalize_thread_hang_cleanup_callback, NULL); #endif - PyObject_CallNoArgs(arg); + PyObject_CallNoArgs(callback); // Should not reach here. Py_FatalError("thread unexpectedly did not hang"); #ifdef _POSIX_THREADS From afe55073bfcd421ce062910040188f3382c52898 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 2 Oct 2024 00:28:55 +0000 Subject: [PATCH 11/15] include what you use for pause() --- Python/thread_pthread.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 8b153e14f97363..8b4c25815e07f7 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -16,6 +16,7 @@ #undef destructor #endif #include +#include /* pause(), also getthrid() on OpenBSD */ #if defined(__linux__) # include /* syscall(SYS_gettid) */ @@ -23,8 +24,6 @@ # include /* pthread_getthreadid_np() */ #elif defined(__FreeBSD_kernel__) # include /* syscall(SYS_thr_self) */ -#elif defined(__OpenBSD__) -# include /* getthrid() */ #elif defined(_AIX) # include /* thread_self() */ #elif defined(__NetBSD__) From e05a1275b640b67cd6d59704f4eaf80d63f2bce5 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 2 Oct 2024 00:29:10 +0000 Subject: [PATCH 12/15] improve comments. --- Python/ceval_gil.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index c4067e8dff6657..e52370e4d4a979 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -278,6 +278,7 @@ drop_gil(PyInterpreterState *interp, PyThreadState *tstate, int final_release) /* Take the GIL. The function saves errno at entry and restores its value at exit. + It may hang rather than return if the interpreter has been finalized. tstate must be non-NULL. */ static void @@ -298,11 +299,11 @@ take_gil(PyThreadState *tstate) completes. In this case, tstate is a dangling pointer: points to PyThreadState freed memory. - This used to call a *thread_exit API, but that was not safe as it - lacks stack unwinding and local variable destruction important to - C++. gh-87135: The best that can be done is to hang the thread as - the public APIs calling this have no error reporting mechanism (!). - */ + This used to call a *thread_exit API, but that was not safe as it + lacks stack unwinding and local variable destruction important to + C++. gh-87135: The best that can be done is to hang the thread as + the public APIs calling this have no error reporting mechanism (!). + */ PyThread_hang_thread(); } From 1569700bec261c87a695f62c7e4cfcc264307ded Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 2 Oct 2024 01:03:14 +0000 Subject: [PATCH 13/15] wasi doesn't have pause()?!? try sleep(9999999) --- Python/thread_pthread.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 8b4c25815e07f7..c010b3a827757f 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -422,7 +422,11 @@ void _Py_NO_RETURN PyThread_hang_thread(void) { while (1) { +#if defined(__wasi__) + sleep(9999999); // WASI doesn't have pause() ?! +#else pause(); +#endif } } From ac3b2ceaf7442581d90b4cced60d6924e5944c19 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 2 Oct 2024 03:42:28 +0000 Subject: [PATCH 14/15] more wasi workarounds. if any more, i'll just skip the test on wasi. --- Modules/_testcapimodule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index ac25f649ae7b69..ea26295cca49d4 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3325,13 +3325,14 @@ static void finalize_thread_hang_cleanup_callback(void *Py_UNUSED(arg)) { static PyObject * finalize_thread_hang(PyObject *self, PyObject *callback) { -#ifdef _POSIX_THREADS + // WASI builds some pthread stuff but doesn't have these APIs today? +#if defined(_POSIX_THREADS) && !defined(__wasi__) pthread_cleanup_push(finalize_thread_hang_cleanup_callback, NULL); #endif PyObject_CallNoArgs(callback); // Should not reach here. Py_FatalError("thread unexpectedly did not hang"); -#ifdef _POSIX_THREADS +#if defined(_POSIX_THREADS) && !defined(__wasi__) pthread_cleanup_pop(0); #endif Py_RETURN_NONE; From ac3d2be346ea5f7ec3127d73e3bc1d3719bffd01 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 2 Oct 2024 04:17:49 +0000 Subject: [PATCH 15/15] Skip the new test under TSAN (and MSAN). Linking to the filed pre-existing known issue. --- Lib/test/test_threading.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index a13db4984ef850..3ca5f5ce1b7068 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1173,6 +1173,12 @@ def __del__(self): @cpython_only def test_finalize_daemon_thread_hang(self): + if support.check_sanitizer(thread=True, memory=True): + # the thread running `time.sleep(100)` below will still be alive + # at process exit + self.skipTest( + "https://github.com/python/cpython/issues/124878 - Known" + " race condition that TSAN identifies.") # gh-87135: tests that daemon threads hang during finalization script = textwrap.dedent(''' import os