From: John Spray Date: Tue, 3 Oct 2017 12:16:10 +0000 (-0400) Subject: mgr: safety checks on pyThreadState usage X-Git-Tag: v13.0.1~561^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=625e1b5cfb9b8a5843dfe75e97826f70a57d6ebe;p=ceph.git mgr: safety checks on pyThreadState usage Previously relied on the caller of Gil() to pass new_thread=true if they would be calling from a different thread. Enforce this with an assertion, by wrapping PyThreadState in a SafeThreadState class that remembers which POSIX thread it's meant to be used in. Signed-off-by: John Spray --- diff --git a/src/mgr/Gil.cc b/src/mgr/Gil.cc index 53ecf99c37f0..9489a3131086 100644 --- a/src/mgr/Gil.cc +++ b/src/mgr/Gil.cc @@ -24,13 +24,18 @@ #include "Gil.h" -Gil::Gil(PyThreadState *ts, bool new_thread) : pThreadState(ts) +SafeThreadState::SafeThreadState(PyThreadState *ts_) + : ts(ts_) { - assert(pThreadState != nullptr); + assert(ts != nullptr); + thread = pthread_self(); +} +Gil::Gil(SafeThreadState &ts, bool new_thread) : pThreadState(ts) +{ // Acquire the GIL, set the current thread state - PyEval_RestoreThread(pThreadState); - dout(25) << "GIL acquired for thread state " << pThreadState << dendl; + PyEval_RestoreThread(pThreadState.ts); + dout(25) << "GIL acquired for thread state " << pThreadState.ts << dendl; // // If called from a separate OS thread (i.e. a thread not created @@ -51,9 +56,11 @@ Gil::Gil(PyThreadState *ts, bool new_thread) : pThreadState(ts) // something that's meant to be a black box. // if (new_thread) { - pNewThreadState = PyThreadState_New(pThreadState->interp); + pNewThreadState = PyThreadState_New(pThreadState.ts->interp); PyThreadState_Swap(pNewThreadState); dout(20) << "Switched to new thread state " << pNewThreadState << dendl; + } else { + assert(pthread_self() == pThreadState.thread); } } @@ -61,12 +68,12 @@ Gil::~Gil() { if (pNewThreadState != nullptr) { dout(20) << "Destroying new thread state " << pNewThreadState << dendl; - PyThreadState_Swap(pThreadState); + PyThreadState_Swap(pThreadState.ts); PyThreadState_Clear(pNewThreadState); PyThreadState_Delete(pNewThreadState); } // Release the GIL, reset the thread state to NULL PyEval_SaveThread(); - dout(25) << "GIL released for thread state " << pThreadState << dendl; + dout(25) << "GIL released for thread state " << pThreadState.ts << dendl; } diff --git a/src/mgr/Gil.h b/src/mgr/Gil.h index 28ec8a5f3897..ef9e76ac108b 100644 --- a/src/mgr/Gil.h +++ b/src/mgr/Gil.h @@ -17,6 +17,35 @@ struct _ts; typedef struct _ts PyThreadState; +#include + + +/** + * Wrap PyThreadState to carry a record of which POSIX thread + * the thread state relates to. This allows the Gil class to + * validate that we're being used from the right thread. + */ +class SafeThreadState +{ + public: + SafeThreadState(PyThreadState *ts_); + + SafeThreadState() + : ts(nullptr), thread(0) + { + } + + PyThreadState *ts; + pthread_t thread; + + void set(PyThreadState *ts_) + { + ts = ts_; + thread = pthread_self(); + } +}; + +// // Use one of these in any scope in which you need to hold Python's // Global Interpreter Lock. // @@ -33,11 +62,11 @@ public: Gil(const Gil&) = delete; Gil& operator=(const Gil&) = delete; - Gil(PyThreadState *ts, bool new_thread = false); + Gil(SafeThreadState &ts, bool new_thread = false); ~Gil(); private: - PyThreadState *pThreadState; + SafeThreadState &pThreadState; PyThreadState *pNewThreadState = nullptr; }; diff --git a/src/mgr/MgrPyModule.cc b/src/mgr/MgrPyModule.cc index 49b70d8a24bb..50165e71187b 100644 --- a/src/mgr/MgrPyModule.cc +++ b/src/mgr/MgrPyModule.cc @@ -13,7 +13,6 @@ #include "PyState.h" #include "PyOSDMap.h" -#include "Gil.h" #include "PyFormatter.h" @@ -85,14 +84,14 @@ MgrPyModule::MgrPyModule(const std::string &module_name_, const std::string &sys pClassInstance(nullptr), pMainThreadState(main_ts_) { - assert(pMainThreadState != nullptr); - Gil gil(pMainThreadState); - pMyThreadState = Py_NewInterpreter(); - if (pMyThreadState == nullptr) { + auto thread_state = Py_NewInterpreter(); + if (thread_state == nullptr) { derr << "Failed to create python sub-interpreter for '" << module_name << '"' << dendl; } else { + pMyThreadState.set(thread_state); + // Some python modules do not cope with an unpopulated argv, so lets // fake one. This step also picks up site-packages into sys.path. const char *argv[] = {"ceph-mgr"}; @@ -120,7 +119,7 @@ MgrPyModule::MgrPyModule(const std::string &module_name_, const std::string &sys MgrPyModule::~MgrPyModule() { - if (pMyThreadState != nullptr) { + if (pMyThreadState.ts != nullptr) { Gil gil(pMyThreadState); Py_XDECREF(pClassInstance); @@ -153,7 +152,7 @@ MgrPyModule::~MgrPyModule() int MgrPyModule::load() { - if (pMyThreadState == nullptr) { + if (pMyThreadState.ts == nullptr) { derr << "No python sub-interpreter exists for module '" << module_name << "'" << dendl; return -EINVAL; } diff --git a/src/mgr/MgrPyModule.h b/src/mgr/MgrPyModule.h index 6eea29eee24e..9f5a5ef53ac4 100644 --- a/src/mgr/MgrPyModule.h +++ b/src/mgr/MgrPyModule.h @@ -23,6 +23,7 @@ #include "common/LogEntry.h" #include "common/Mutex.h" #include "mon/health_check.h" +#include "mgr/Gil.h" #include #include @@ -46,8 +47,8 @@ class MgrPyModule private: const std::string module_name; PyObject *pClassInstance; - PyThreadState *pMainThreadState; - PyThreadState *pMyThreadState = nullptr; + SafeThreadState pMainThreadState; + SafeThreadState pMyThreadState; health_check_map_t health_checks; diff --git a/src/mgr/PyModules.cc b/src/mgr/PyModules.cc index 4b3f5cc608b6..11865eff28ed 100644 --- a/src/mgr/PyModules.cc +++ b/src/mgr/PyModules.cc @@ -417,6 +417,7 @@ int PyModules::init() // Drop the GIL and remember the main thread state (current // thread state becomes NULL) pMainThreadState = PyEval_SaveThread(); + assert(pMainThreadState != nullptr); std::list failed_modules; diff --git a/src/mgr/PyState.cc b/src/mgr/PyState.cc index fe849de107e7..31340f678cc9 100644 --- a/src/mgr/PyState.cc +++ b/src/mgr/PyState.cc @@ -35,7 +35,7 @@ class MonCommandCompletion : public Context { PyObject *python_completion; const std::string tag; - PyThreadState *pThreadState; + SafeThreadState pThreadState; public: std::string outs;