Changing LockedPageManager to use a managed instance

This ensures the allocator is ready no matter when it's needed (as some STL implementations allocate in constructors -- i.e., MSVC's STL in debug builds). Using boost::call_once to guarantee thread-safe static initialization. Adding some comments describing why the change was made. Addressing deinitialization of the LockedPageManager object by initializing it in a local static initializer and adding an assert in the base's destructor.
TrezarCoin · Oct 20, 2013 · 0b8f47d · 0b8f47d
1 parent 896853a
commit 0b8f47d
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 11 deletions.
diff --git a/src/allocators.cpp b/src/allocators.cpp
@@ -24,6 +24,9 @@
 #include <unistd.h> // for sysconf
 #endif
 
+LockedPageManager* LockedPageManager::_instance = NULL;
+boost::once_flag LockedPageManager::init_flag = BOOST_ONCE_INIT;
+
 /** Determine system page size in bytes */
 static inline size_t GetSystemPageSize()
 {

diff --git a/src/allocators.h b/src/allocators.h
@@ -8,6 +8,7 @@
 #include <string.h>
 #include <string>
 #include <boost/thread/mutex.hpp>
+#include <boost/thread/once.hpp>
 #include <map>
 #include <openssl/crypto.h> // for OPENSSL_cleanse()
 
@@ -34,6 +35,12 @@ template <class Locker> class LockedPageManagerBase
         page_mask = ~(page_size - 1);
     }
 
+    ~LockedPageManagerBase()
+    {
+        assert(this->GetLockedPageCount() == 0);
+    }
+
+
     // For all pages in affected range, increase lock count
     void LockRange(void *p, size_t size)
     {
@@ -117,26 +124,52 @@ class MemoryPageLocker
 /**
  * Singleton class to keep track of locked (ie, non-swappable) memory pages, for use in
  * std::allocator templates.
+ *
+ * Some implementations of the STL allocate memory in some constructors (i.e., see
+ * MSVC's vector<T> implementation where it allocates 1 byte of memory in the allocator.)
+ * Due to the unpredictable order of static initializers, we have to make sure the
+ * LockedPageManager instance exists before any other STL-based objects that use
+ * secure_allocator are created. So instead of having LockedPageManager also be
+ * static-intialized, it is created on demand.
  */
 class LockedPageManager: public LockedPageManagerBase<MemoryPageLocker>
 {
 public:
-    static LockedPageManager instance; // instantiated in util.cpp
+    static LockedPageManager& Instance() 
+    {
+        boost::call_once(LockedPageManager::CreateInstance, LockedPageManager::init_flag);
+        return *LockedPageManager::_instance;
+    }
+
 private:
     LockedPageManager();
+
+    static void CreateInstance()
+    {
+        // Using a local static instance guarantees that the object is initialized
+        // when it's first needed and also deinitialized after all objects that use
+        // it are done with it.  I can think of one unlikely scenario where we may
+        // have a static deinitialization order/problem, but the check in
+        // LockedPageManagerBase's destructor helps us detect if that ever happens.
+        static LockedPageManager instance;
+        LockedPageManager::_instance = &instance;
+    }
+
+    static LockedPageManager* _instance;
+    static boost::once_flag init_flag;
 };
 
 //
 // Functions for directly locking/unlocking memory objects.
 // Intended for non-dynamically allocated structures.
 //
 template<typename T> void LockObject(const T &t) {
-    LockedPageManager::instance.LockRange((void*)(&t), sizeof(T));
+    LockedPageManager::Instance().LockRange((void*)(&t), sizeof(T));
 }
 
 template<typename T> void UnlockObject(const T &t) {
     OPENSSL_cleanse((void*)(&t), sizeof(T));
-    LockedPageManager::instance.UnlockRange((void*)(&t), sizeof(T));
+    LockedPageManager::Instance().UnlockRange((void*)(&t), sizeof(T));
 }
 
 //
@@ -168,7 +201,7 @@ struct secure_allocator : public std::allocator<T>
         T *p;
         p = std::allocator<T>::allocate(n, hint);
         if (p != NULL)
-            LockedPageManager::instance.LockRange(p, sizeof(T) * n);
+            LockedPageManager::Instance().LockRange(p, sizeof(T) * n);
         return p;
     }
 
@@ -177,7 +210,7 @@ struct secure_allocator : public std::allocator<T>
         if (p != NULL)
         {
             OPENSSL_cleanse(p, sizeof(T) * n);
-            LockedPageManager::instance.UnlockRange(p, sizeof(T) * n);
+            LockedPageManager::Instance().UnlockRange(p, sizeof(T) * n);
         }
         std::allocator<T>::deallocate(p, n);
     }

diff --git a/src/crypter.h b/src/crypter.h
@@ -88,16 +88,16 @@ class CCrypter
         // Try to keep the key data out of swap (and be a bit over-careful to keep the IV that we don't even use out of swap)
         // Note that this does nothing about suspend-to-disk (which will put all our key data on disk)
         // Note as well that at no point in this program is any attempt made to prevent stealing of keys by reading the memory of the running process.
-        LockedPageManager::instance.LockRange(&chKey[0], sizeof chKey);
-        LockedPageManager::instance.LockRange(&chIV[0], sizeof chIV);
+        LockedPageManager::Instance().LockRange(&chKey[0], sizeof chKey);
+        LockedPageManager::Instance().LockRange(&chIV[0], sizeof chIV);
     }
 
     ~CCrypter()
     {
         CleanKey();
 
-        LockedPageManager::instance.UnlockRange(&chKey[0], sizeof chKey);
-        LockedPageManager::instance.UnlockRange(&chIV[0], sizeof chIV);
+        LockedPageManager::Instance().UnlockRange(&chKey[0], sizeof chKey);
+        LockedPageManager::Instance().UnlockRange(&chIV[0], sizeof chIV);
     }
 };
 

diff --git a/src/util.cpp b/src/util.cpp
@@ -95,8 +95,6 @@ void locking_callback(int mode, int i, const char* file, int line)
     }
 }
 
-LockedPageManager LockedPageManager::instance;
-
 // Init
 class CInit
 {
-Original file line number
+Diff line change
@@ Expand Up @@
         }
     }
-    LockedPageManager LockedPageManager::instance;
     // Init
     class CInit
     {
@@ Expand Down @@