From b4d1e1f7c1af6ae33f0e371576c8bcafedb099db Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 30 Nov 2017 22:05:00 +0100 Subject: [PATCH] bpo-20891: Fix PyGILState_Ensure() (#4650) When PyGILState_Ensure() is called in a non-Python thread before PyEval_InitThreads(), only call PyEval_InitThreads() after calling PyThreadState_New() to fix a crash. Add an unit test in test_embed. --- Doc/c-api/init.rst | 5 +- Lib/test/test_embed.py | 10 ++++ .../2017-11-30-18-13-45.bpo-20891.wBnMdF.rst | 3 ++ Programs/_testembed.c | 49 +++++++++++++++++++ Python/pystate.c | 24 ++++++--- 5 files changed, 83 insertions(+), 8 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2017-11-30-18-13-45.bpo-20891.wBnMdF.rst diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 2f77bb359d24e5..a9927aba5e13a6 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -58,8 +58,9 @@ The following functions can be safely called before Python is initialized: The following functions **should not be called** before :c:func:`Py_Initialize`: :c:func:`Py_EncodeLocale`, :c:func:`Py_GetPath`, - :c:func:`Py_GetPrefix`, :c:func:`Py_GetExecPrefix` and - :c:func:`Py_GetProgramFullPath` and :c:func:`Py_GetPythonHome`. + :c:func:`Py_GetPrefix`, :c:func:`Py_GetExecPrefix`, + :c:func:`Py_GetProgramFullPath`, :c:func:`Py_GetPythonHome` and + :c:func:`PyEval_InitThreads`. .. _global-conf-vars: diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 8d44543ffd67a5..c7f45b59acc8a1 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -198,6 +198,16 @@ def test_pre_initialization_api(self): self.assertEqual(out, '') self.assertEqual(err, '') + def test_bpo20891(self): + """ + bpo-20891: Calling PyGILState_Ensure in a non-Python thread before + calling PyEval_InitThreads() must not crash. PyGILState_Ensure() must + call PyEval_InitThreads() for us in this case. + """ + out, err = self.run_embedded_interpreter("bpo20891") + self.assertEqual(out, '') + self.assertEqual(err, '') + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/C API/2017-11-30-18-13-45.bpo-20891.wBnMdF.rst b/Misc/NEWS.d/next/C API/2017-11-30-18-13-45.bpo-20891.wBnMdF.rst new file mode 100644 index 00000000000000..e89cf1292afa8a --- /dev/null +++ b/Misc/NEWS.d/next/C API/2017-11-30-18-13-45.bpo-20891.wBnMdF.rst @@ -0,0 +1,3 @@ +Fix PyGILState_Ensure(). When PyGILState_Ensure() is called in a non-Python +thread before PyEval_InitThreads(), only call PyEval_InitThreads() after +calling PyThreadState_New() to fix a crash. diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 21aa76e9de38bf..a528f3e3aa0af9 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1,4 +1,5 @@ #include +#include "pythread.h" #include #include @@ -147,6 +148,53 @@ static int test_pre_initialization_api(void) return 0; } +static void bpo20891_thread(void *lockp) +{ + PyThread_type_lock lock = *((PyThread_type_lock*)lockp); + + PyGILState_STATE state = PyGILState_Ensure(); + if (!PyGILState_Check()) { + fprintf(stderr, "PyGILState_Check failed!"); + abort(); + } + + PyGILState_Release(state); + + PyThread_release_lock(lock); + + PyThread_exit_thread(); +} + +static int test_bpo20891(void) +{ + /* bpo-20891: Calling PyGILState_Ensure in a non-Python thread before + calling PyEval_InitThreads() must not crash. PyGILState_Ensure() must + call PyEval_InitThreads() for us in this case. */ + PyThread_type_lock lock = PyThread_allocate_lock(); + if (!lock) { + fprintf(stderr, "PyThread_allocate_lock failed!"); + return 1; + } + + _testembed_Py_Initialize(); + + unsigned long thrd = PyThread_start_new_thread(bpo20891_thread, &lock); + if (thrd == PYTHREAD_INVALID_THREAD_ID) { + fprintf(stderr, "PyThread_start_new_thread failed!"); + return 1; + } + PyThread_acquire_lock(lock, WAIT_LOCK); + + Py_BEGIN_ALLOW_THREADS + /* wait until the thread exit */ + PyThread_acquire_lock(lock, WAIT_LOCK); + Py_END_ALLOW_THREADS + + PyThread_free_lock(lock); + + return 0; +} + /* ********************************************************* * List of test cases and the function that implements it. @@ -170,6 +218,7 @@ static struct TestCase TestCases[] = { { "forced_io_encoding", test_forced_io_encoding }, { "repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters }, { "pre_initialization_api", test_pre_initialization_api }, + { "bpo20891", test_bpo20891 }, { NULL, NULL } }; diff --git a/Python/pystate.c b/Python/pystate.c index 0fb8ed071954a0..500f96768752f4 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -922,6 +922,8 @@ PyGILState_Ensure(void) { int current; PyThreadState *tcur; + int need_init_threads = 0; + /* Note that we do not auto-init Python here - apart from potential races with 2 threads auto-initializing, pep-311 spells out other issues. Embedders are expected to have @@ -929,12 +931,10 @@ PyGILState_Ensure(void) */ /* Py_Initialize() hasn't been called! */ assert(_PyRuntime.gilstate.autoInterpreterState); + tcur = (PyThreadState *)PyThread_tss_get(&_PyRuntime.gilstate.autoTSSkey); if (tcur == NULL) { - /* At startup, Python has no concrete GIL. If PyGILState_Ensure() is - called from a new thread for the first time, we need the create the - GIL. */ - PyEval_InitThreads(); + need_init_threads = 1; /* Create a new thread state for this thread */ tcur = PyThreadState_New(_PyRuntime.gilstate.autoInterpreterState); @@ -945,16 +945,28 @@ PyGILState_Ensure(void) tcur->gilstate_counter = 0; current = 0; /* new thread state is never current */ } - else + else { current = PyThreadState_IsCurrent(tcur); - if (current == 0) + } + + if (current == 0) { PyEval_RestoreThread(tcur); + } + /* Update our counter in the thread-state - no need for locks: - tcur will remain valid as we hold the GIL. - the counter is safe as we are the only thread "allowed" to modify this value */ ++tcur->gilstate_counter; + + if (need_init_threads) { + /* At startup, Python has no concrete GIL. If PyGILState_Ensure() is + called from a new thread for the first time, we need the create the + GIL. */ + PyEval_InitThreads(); + } + return current ? PyGILState_LOCKED : PyGILState_UNLOCKED; }