From 0b8f47dc531d3cbaf172a5e17f27524a40833dba Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 1 Oct 2013 17:23:17 +0700 Subject: [PATCH] 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. --- src/allocators.cpp | 3 +++ src/allocators.h | 43 ++++++++++++++++++++++++++++++++++++++----- src/crypter.h | 8 ++++---- src/util.cpp | 2 -- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/allocators.cpp b/src/allocators.cpp index b239b623d..15f34aa2c 100644 --- a/src/allocators.cpp +++ b/src/allocators.cpp @@ -24,6 +24,9 @@ #include // 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 index fd6f51b27..b199f2dd4 100644 --- a/src/allocators.h +++ b/src/allocators.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include // for OPENSSL_cleanse() @@ -34,6 +35,12 @@ public: 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,13 +124,39 @@ public: /** * 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 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 { 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; }; // @@ -131,12 +164,12 @@ private: // Intended for non-dynamically allocated structures. // template void LockObject(const T &t) { - LockedPageManager::instance.LockRange((void*)(&t), sizeof(T)); + LockedPageManager::Instance().LockRange((void*)(&t), sizeof(T)); } template 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 *p; p = std::allocator::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 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::deallocate(p, n); } diff --git a/src/crypter.h b/src/crypter.h index 4134c1b49..9826d63b7 100644 --- a/src/crypter.h +++ b/src/crypter.h @@ -88,16 +88,16 @@ public: // 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 index cfaf5bdf8..ab288f63f 100644 --- 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 {