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 <john.spray@redhat.com>

diff --git a/src/mgr/Gil.cc b/src/mgr/Gil.cc
index 53ecf99c37f057f71840ffb4cdfb7a20ed5ea84d..9489a31310868771a113bbba030890f7a2ed422b 100644 (file)
--- 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 28ec8a5f389704566a5e5769e93d03104799eeb5..ef9e76ac108b9b7ad35ae6b6f3485238d991dfbc 100644 (file)
--- a/src/mgr/Gil.h
+++ b/src/mgr/Gil.h
@@ -17,6 +17,35 @@
 struct _ts;
 typedef struct _ts PyThreadState;
 
+#include <pthread.h>
+
+
+/**
+ * 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 49b70d8a24bbd3047ad34afcfac1f889f5350031..50165e71187bdaf8b7abeb992dad9c95741182ec 100644 (file)
--- 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 6eea29eee24e6abbbd2dcc9e32aeae326f337d7a..9f5a5ef53ac483972ee5f92bb94064dfad5acc5c 100644 (file)
--- 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 <vector>
 #include <string>
@@ -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 4b3f5cc608b65743b1fbd26b5964201a40bd6b70..11865eff28edbb364c2f642d6fcd6706268e16ea 100644 (file)
--- 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<std::string> failed_modules;
 

diff --git a/src/mgr/PyState.cc b/src/mgr/PyState.cc
index fe849de107e790a123b775fe2075e310d591fba7..31340f678cc93ba21ffd73f18e8e5b2d80f52f62 100644 (file)
--- 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;