From 5e2224fb1f72e7133cd522d931f1dd34f7b7ba43 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 23 Sep 2023 13:53:01 -0600 Subject: [PATCH 1/9] Add a test. --- Lib/test/test_interpreters.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/test/test_interpreters.py b/Lib/test/test_interpreters.py index 90932c0f66f38f..1bde4cd62a8692 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,20 @@ def task(): 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) + self.assertEqual(proc.returncode, 1) + + class TestIsShareable(TestBase): def test_default_shareables(self): From a1014a924ce126707d6b525eef5794a45eb19deb Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 23 Sep 2023 13:54:22 -0600 Subject: [PATCH 2/9] Also check the thread ID in _PyThreadState_MustExit(). --- Python/pystate.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index dcc6c112215b30..de28d193848114 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2968,7 +2968,17 @@ _PyThreadState_MustExit(PyThreadState *tstate) if (finalizing == NULL) { finalizing = _PyInterpreterState_GetFinalizing(tstate->interp); } - return (finalizing != NULL && finalizing != tstate); + if (finalizing == NULL) { + return 0; + } + else if (finalizing == tstate) { + return 0; + } + else if (finalizing->thread_id == tstate->thread_id) { + /* gh-109793: we must have switched interpreters. */ + return 0; + } + return 1; } From d46103d487c7e7cac977ca4732b7f8e009d96439 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 23 Sep 2023 14:10:16 -0600 Subject: [PATCH 3/9] Use _PyRuntime directly in _Py_IsMainInterpreterFinalizing(). --- Include/internal/pycore_pystate.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 9fc8ae903b2ac0..2e568f8aeeb152 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); } From 2e13a75928057758f650ad22eaeb7d56ec728120 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Sat, 23 Sep 2023 18:12:33 -0600 Subject: [PATCH 4/9] Add _PyRuntimeState._finalizing_id and _PyInterpreterState._finalizing_id. --- Include/internal/pycore_atomic.h | 24 ++++++++++++++++++++++++ Include/internal/pycore_interp.h | 14 ++++++++++++++ Include/internal/pycore_runtime.h | 14 ++++++++++++++ Python/pystate.c | 7 ++++++- 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_atomic.h b/Include/internal/pycore_atomic.h index 22ce971a64f3df..c7d2ae4cc0d3ed 100644 --- a/Include/internal/pycore_atomic.h +++ b/Include/internal/pycore_atomic.h @@ -46,6 +46,10 @@ typedef struct _Py_atomic_address { atomic_uintptr_t _value; } _Py_atomic_address; +typedef struct _Py_atomic_ulong { + atomic_ulong _value; +} _Py_atomic_ulong; + typedef struct _Py_atomic_int { atomic_int _value; } _Py_atomic_int; @@ -77,6 +81,10 @@ typedef struct _Py_atomic_address { uintptr_t _value; } _Py_atomic_address; +typedef struct _Py_atomic_ulong { + unsigned long _value; +} _Py_atomic_ulong; + typedef struct _Py_atomic_int { int _value; } _Py_atomic_int; @@ -115,6 +123,10 @@ typedef struct _Py_atomic_address { uintptr_t _value; } _Py_atomic_address; +typedef struct _Py_atomic_ulong { + unsigned long _value; +} _Py_atomic_ulong; + typedef struct _Py_atomic_int { int _value; } _Py_atomic_int; @@ -251,6 +263,10 @@ typedef struct _Py_atomic_address { volatile uintptr_t _value; } _Py_atomic_address; +typedef struct _Py_atomic_ulong { + volatile unsigned long _value; +} _Py_atomic_ulong; + typedef struct _Py_atomic_int { volatile int _value; } _Py_atomic_int; @@ -387,6 +403,10 @@ typedef struct _Py_atomic_address { volatile uintptr_t _value; } _Py_atomic_address; +typedef struct _Py_atomic_ulong { + volatile unsigned long _value; +} _Py_atomic_ulong; + typedef struct _Py_atomic_int { volatile int _value; } _Py_atomic_int; @@ -524,6 +544,10 @@ typedef struct _Py_atomic_address { uintptr_t _value; } _Py_atomic_address; +typedef struct _Py_atomic_ulong { + unsigned long _value; +} _Py_atomic_ulong; + typedef struct _Py_atomic_int { int _value; } _Py_atomic_int; diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index ba5764e943e676..7a7839dda37398 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. */ + _Py_atomic_ulong _finalizing_id; struct _gc_runtime_state gc; @@ -215,9 +217,21 @@ _PyInterpreterState_GetFinalizing(PyInterpreterState *interp) { return (PyThreadState*)_Py_atomic_load_relaxed(&interp->_finalizing); } +static inline unsigned long +_PyInterpreterState_GetFinalizingID(PyInterpreterState *interp) { + return _Py_atomic_load_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_relaxed(&interp->_finalizing_id, 0); + } + else { + assert(tstate->thread_id == PyThread_get_thread_ident()); + _Py_atomic_store_relaxed(&interp->_finalizing_id, tstate->thread_id); + } } diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 0ddc405f221a1c..0fbcdfac4cb655 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. */ + _Py_atomic_ulong _finalizing_id; struct pyinterpreters { PyThread_type_lock mutex; @@ -303,9 +305,21 @@ _PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) { return (PyThreadState*)_Py_atomic_load_relaxed(&runtime->_finalizing); } +static inline unsigned long +_PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) { + return _Py_atomic_load_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_relaxed(&runtime->_finalizing_id, 0); + } + else { + assert(tstate->thread_id == PyThread_get_thread_ident()); + _Py_atomic_store_relaxed(&runtime->_finalizing_id, tstate->thread_id); + } } #ifdef __cplusplus diff --git a/Python/pystate.c b/Python/pystate.c index de28d193848114..570b5242600c0c 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2964,17 +2964,22 @@ _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); } + // XXX else check &_PyRuntime._main_interpreter._initial_thread if (finalizing == NULL) { return 0; } else if (finalizing == tstate) { return 0; } - else if (finalizing->thread_id == tstate->thread_id) { + else if (finalizing_id == PyThread_get_thread_ident()) { /* gh-109793: we must have switched interpreters. */ return 0; } From 756cc3b16e71cdf10f42deff1aeb35164ddb2db2 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 25 Sep 2023 09:10:07 -0600 Subject: [PATCH 5/9] Add verbose output to the test. --- Lib/test/test_interpreters.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_interpreters.py b/Lib/test/test_interpreters.py index 1bde4cd62a8692..9c0dac7d6c61fb 100644 --- a/Lib/test/test_interpreters.py +++ b/Lib/test/test_interpreters.py @@ -499,6 +499,12 @@ def test_gh_109793(self): '''] 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) From ad312b989a7a51be03528f057e71ad4f96875d38 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 25 Sep 2023 09:24:21 -0600 Subject: [PATCH 6/9] Add a NEWS entry. --- .../2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst 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 00000000000000..398077dd3d5bda --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst @@ -0,0 +1 @@ +``sys.path[0]`` is now set properly for subinterpreters. From 8feeb4c54d5b3318f758b38e0e7c1a5523c24ba1 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 25 Sep 2023 11:15:57 -0600 Subject: [PATCH 7/9] Skip the assert for now. --- Include/internal/pycore_interp.h | 3 ++- Include/internal/pycore_runtime.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 7a7839dda37398..a6ce824014cb77 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -229,7 +229,8 @@ _PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tst _Py_atomic_store_relaxed(&interp->_finalizing_id, 0); } else { - assert(tstate->thread_id == PyThread_get_thread_ident()); + // XXX Re-enable this assert once gh-109860 is fixed. + //assert(tstate->thread_id == PyThread_get_thread_ident()); _Py_atomic_store_relaxed(&interp->_finalizing_id, tstate->thread_id); } } diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 0fbcdfac4cb655..2fa77f439e9567 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -317,7 +317,8 @@ _PyRuntimeState_SetFinalizing(_PyRuntimeState *runtime, PyThreadState *tstate) { _Py_atomic_store_relaxed(&runtime->_finalizing_id, 0); } else { - assert(tstate->thread_id == PyThread_get_thread_ident()); + // XXX Re-enable this assert once gh-109860 is fixed. + //assert(tstate->thread_id == PyThread_get_thread_ident()); _Py_atomic_store_relaxed(&runtime->_finalizing_id, tstate->thread_id); } } From 9e589a789ab49e78d764a669c88906b10ece441d Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 27 Sep 2023 10:04:09 -0600 Subject: [PATCH 8/9] Use the new _Py_atomic API. --- Include/cpython/pyatomic.h | 17 +++++++++++++++++ Include/internal/pycore_atomic.h | 24 ------------------------ Include/internal/pycore_interp.h | 9 +++++---- Include/internal/pycore_runtime.h | 9 +++++---- 4 files changed, 27 insertions(+), 32 deletions(-) diff --git a/Include/cpython/pyatomic.h b/Include/cpython/pyatomic.h index ab182381b39f00..ce23e13bf3838c 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_atomic.h b/Include/internal/pycore_atomic.h index c7d2ae4cc0d3ed..22ce971a64f3df 100644 --- a/Include/internal/pycore_atomic.h +++ b/Include/internal/pycore_atomic.h @@ -46,10 +46,6 @@ typedef struct _Py_atomic_address { atomic_uintptr_t _value; } _Py_atomic_address; -typedef struct _Py_atomic_ulong { - atomic_ulong _value; -} _Py_atomic_ulong; - typedef struct _Py_atomic_int { atomic_int _value; } _Py_atomic_int; @@ -81,10 +77,6 @@ typedef struct _Py_atomic_address { uintptr_t _value; } _Py_atomic_address; -typedef struct _Py_atomic_ulong { - unsigned long _value; -} _Py_atomic_ulong; - typedef struct _Py_atomic_int { int _value; } _Py_atomic_int; @@ -123,10 +115,6 @@ typedef struct _Py_atomic_address { uintptr_t _value; } _Py_atomic_address; -typedef struct _Py_atomic_ulong { - unsigned long _value; -} _Py_atomic_ulong; - typedef struct _Py_atomic_int { int _value; } _Py_atomic_int; @@ -263,10 +251,6 @@ typedef struct _Py_atomic_address { volatile uintptr_t _value; } _Py_atomic_address; -typedef struct _Py_atomic_ulong { - volatile unsigned long _value; -} _Py_atomic_ulong; - typedef struct _Py_atomic_int { volatile int _value; } _Py_atomic_int; @@ -403,10 +387,6 @@ typedef struct _Py_atomic_address { volatile uintptr_t _value; } _Py_atomic_address; -typedef struct _Py_atomic_ulong { - volatile unsigned long _value; -} _Py_atomic_ulong; - typedef struct _Py_atomic_int { volatile int _value; } _Py_atomic_int; @@ -544,10 +524,6 @@ typedef struct _Py_atomic_address { uintptr_t _value; } _Py_atomic_address; -typedef struct _Py_atomic_ulong { - unsigned long _value; -} _Py_atomic_ulong; - typedef struct _Py_atomic_int { int _value; } _Py_atomic_int; diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index a6ce824014cb77..0912bd175fe4f7 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -94,7 +94,7 @@ struct _is { to access it, don't access it directly. */ _Py_atomic_address _finalizing; /* The ID of the OS thread in which we are finalizing. */ - _Py_atomic_ulong _finalizing_id; + unsigned long _finalizing_id; struct _gc_runtime_state gc; @@ -219,19 +219,20 @@ _PyInterpreterState_GetFinalizing(PyInterpreterState *interp) { static inline unsigned long _PyInterpreterState_GetFinalizingID(PyInterpreterState *interp) { - return _Py_atomic_load_relaxed(&interp->_finalizing_id); + 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_relaxed(&interp->_finalizing_id, 0); + _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_relaxed(&interp->_finalizing_id, tstate->thread_id); + _Py_atomic_store_ulong_relaxed(&interp->_finalizing_id, + tstate->thread_id); } } diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 2fa77f439e9567..cc3a3420befa3d 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -172,7 +172,7 @@ typedef struct pyruntimestate { to access it, don't access it directly. */ _Py_atomic_address _finalizing; /* The ID of the OS thread in which we are finalizing. */ - _Py_atomic_ulong _finalizing_id; + unsigned long _finalizing_id; struct pyinterpreters { PyThread_type_lock mutex; @@ -307,19 +307,20 @@ _PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) { static inline unsigned long _PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) { - return _Py_atomic_load_relaxed(&runtime->_finalizing_id); + 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_relaxed(&runtime->_finalizing_id, 0); + _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_relaxed(&runtime->_finalizing_id, tstate->thread_id); + _Py_atomic_store_ulong_relaxed(&runtime->_finalizing_id, + tstate->thread_id); } } From 9829315813b4593fd1b56a6df72cc26b742616f6 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 27 Sep 2023 13:00:53 -0600 Subject: [PATCH 9/9] Fix the NEWS entry. --- .../2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 index 398077dd3d5bda..d2dc4c830a9031 100644 --- 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 @@ -1 +1,4 @@ -``sys.path[0]`` is now set properly for subinterpreters. +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.