Never destroy module-wide singletons via static teardown (#897)

SoftHSM::instance, MutexFactory::instance, SecureMemoryRegistry::instance,
and OSSLCryptoFactory::instance/BotanCryptoFactory::instance were each a
class-level static std::unique_ptr<T> (or std::auto_ptr<T> pre-C++11).
Their destructors ran as part of the C++ runtime's ordinary static-object
teardown at process exit - an indeterminate time relative to unrelated
code's own cleanup.

Concretely: a PKCS#11 caller (e.g. an OpenSSL provider) that still holds
open sessions at process exit calls C_CloseSession() on each before
calling C_Finalize(), per the PKCS#11 spec. But SoftHSM::instance's own
destructor could fire first, via ordinary static teardown, independently
of and unordered with respect to that caller's explicit cleanup sequence.
Whichever teardown path lost the race operated on an already-destroyed
(or transparently reconstructed-blank, since SoftHSM::i() rebuilds a
fresh instance on next access) singleton, and its stale session-close
calls then dereferenced a NULL member on it - a real, reproducible
SIGSEGV when a pkcs11-provider-based caller runs openssl req against a
PKCS#11 key and exits normally.

This is the same category of bug already fixed once for
OSSLCryptoFactory's own OpenSSL ENGINE cleanup (issue #548, commit
c2cc065), just never applied to these singletons' own top-level
lifecycle - and the gap resurfaces one level down too: OSSLCryptoFactory
can be lazily constructed for the first time from deep inside
SoftHSM::C_Finalize()'s own teardown chain, touching ENGINE_by_id() after
OpenSSL's own OPENSSL_cleanup() has already run.

Each singleton's std::unique_ptr<T>/std::auto_ptr<T> is now heap-allocated
once and deliberately never freed, so nothing destroys the T it owns
except that class's own explicit reset()/C_Finalize() path - never
ordinary static teardown. The smart pointer itself is retained as the
owning type (matching the "Static and Global Variables" guidance in the
Google C++ Style Guide, and the same "leaky singleton" idiom used by
e.g. Drake's never_destroyed<T>), so reset()'s exception-safety and
ownership semantics for the owned object are unchanged.

Author: rikard-soderstrom

src/bin/util/softhsm2-util.cpp

@@ -74,22 +74,22 @@
 
 #ifdef HAVE_CXX11
 
-std::unique_ptr<MutexFactory> MutexFactory::instance(nullptr);
-std::unique_ptr<SecureMemoryRegistry> SecureMemoryRegistry::instance(nullptr);
+std::unique_ptr<MutexFactory>* MutexFactory::instance = new std::unique_ptr<MutexFactory>();
+std::unique_ptr<SecureMemoryRegistry>* SecureMemoryRegistry::instance = new std::unique_ptr<SecureMemoryRegistry>();
 #if defined(WITH_OPENSSL)
-std::unique_ptr<OSSLCryptoFactory> OSSLCryptoFactory::instance(nullptr);
+std::unique_ptr<OSSLCryptoFactory>* OSSLCryptoFactory::instance = new std::unique_ptr<OSSLCryptoFactory>();
 #else
-std::unique_ptr<BotanCryptoFactory> BotanCryptoFactory::instance(nullptr);
+std::unique_ptr<BotanCryptoFactory>* BotanCryptoFactory::instance = new std::unique_ptr<BotanCryptoFactory>();
 #endif
 
 #else
 
-std::auto_ptr<MutexFactory> MutexFactory::instance(NULL);
-std::auto_ptr<SecureMemoryRegistry> SecureMemoryRegistry::instance(NULL);
+std::auto_ptr<MutexFactory>* MutexFactory::instance = new std::auto_ptr<MutexFactory>();
+std::auto_ptr<SecureMemoryRegistry>* SecureMemoryRegistry::instance = new std::auto_ptr<SecureMemoryRegistry>();
 #if defined(WITH_OPENSSL)
-std::auto_ptr<OSSLCryptoFactory> OSSLCryptoFactory::instance(NULL);
+std::auto_ptr<OSSLCryptoFactory>* OSSLCryptoFactory::instance = new std::auto_ptr<OSSLCryptoFactory>();
 #else
-std::auto_ptr<BotanCryptoFactory> BotanCryptoFactory::instance(NULL);
+std::auto_ptr<BotanCryptoFactory>* BotanCryptoFactory::instance = new std::auto_ptr<BotanCryptoFactory>();
 #endif
 
 #endif

src/lib/SoftHSM.cpp

@@ -105,25 +105,25 @@
 
 #ifdef HAVE_CXX11
 
-std::unique_ptr<MutexFactory> MutexFactory::instance(nullptr);
-std::unique_ptr<SecureMemoryRegistry> SecureMemoryRegistry::instance(nullptr);
+std::unique_ptr<MutexFactory>* MutexFactory::instance = new std::unique_ptr<MutexFactory>();
+std::unique_ptr<SecureMemoryRegistry>* SecureMemoryRegistry::instance = new std::unique_ptr<SecureMemoryRegistry>();
 #if defined(WITH_OPENSSL)
-std::unique_ptr<OSSLCryptoFactory> OSSLCryptoFactory::instance(nullptr);
+std::unique_ptr<OSSLCryptoFactory>* OSSLCryptoFactory::instance = new std::unique_ptr<OSSLCryptoFactory>();
 #else
-std::unique_ptr<BotanCryptoFactory> BotanCryptoFactory::instance(nullptr);
+std::unique_ptr<BotanCryptoFactory>* BotanCryptoFactory::instance = new std::unique_ptr<BotanCryptoFactory>();
 #endif
-std::unique_ptr<SoftHSM> SoftHSM::instance(nullptr);
+std::unique_ptr<SoftHSM>* SoftHSM::instance = new std::unique_ptr<SoftHSM>();
 
 #else
 
-std::auto_ptr<MutexFactory> MutexFactory::instance(NULL);
-std::auto_ptr<SecureMemoryRegistry> SecureMemoryRegistry::instance(NULL);
+std::auto_ptr<MutexFactory>* MutexFactory::instance = new std::auto_ptr<MutexFactory>();
+std::auto_ptr<SecureMemoryRegistry>* SecureMemoryRegistry::instance = new std::auto_ptr<SecureMemoryRegistry>();
 #if defined(WITH_OPENSSL)
-std::auto_ptr<OSSLCryptoFactory> OSSLCryptoFactory::instance(NULL);
+std::auto_ptr<OSSLCryptoFactory>* OSSLCryptoFactory::instance = new std::auto_ptr<OSSLCryptoFactory>();
 #else
-std::auto_ptr<BotanCryptoFactory> BotanCryptoFactory::instance(NULL);
+std::auto_ptr<BotanCryptoFactory>* BotanCryptoFactory::instance = new std::auto_ptr<BotanCryptoFactory>();
 #endif
-std::auto_ptr<SoftHSM> SoftHSM::instance(NULL);
+std::auto_ptr<SoftHSM>* SoftHSM::instance = new std::auto_ptr<SoftHSM>();
 
 #endif
 
@@ -415,11 +415,11 @@ static void resetMutexFactoryCallbacks()
 // Return the one-and-only instance
 SoftHSM* SoftHSM::i()
 {
-	if (!instance.get())
+	if (!instance->get())
 	{
-		instance.reset(new SoftHSM());
+		instance->reset(new SoftHSM());
 	}
-	else if(instance->detectFork())
+	else if((*instance)->detectFork())
 	{
 		if (Configuration::i()->getBool("library.reset_on_fork", false))
 		{
@@ -428,18 +428,18 @@ SoftHSM* SoftHSM::i()
 			 * the old instance is first destroyed as some
 			 * static structures are erased in the destructor.
 			 */
-			instance.reset(NULL);
-			instance.reset(new SoftHSM());
+			instance->reset(NULL);
+			instance->reset(new SoftHSM());
 		}
 	}
 
-	return instance.get();
+	return instance->get();
 }
 
 void SoftHSM::reset()
 {
-	if (instance.get())
-		instance.reset();
+	if (instance->get())
+		instance->reset();
 }
 
 // Constructor

src/lib/SoftHSM.h

@@ -201,11 +201,30 @@ class SoftHSM
 	// Constructor
 	SoftHSM();
 
-	// The one-and-only instance
+	// The one-and-only instance.
+	//
+	// Smart pointers must not be used directly as class-level
+	// static storage: their destructor runs as part of the C++
+	// runtime's ordinary static-object teardown at process exit, an
+	// indeterminate time relative to unrelated code's own cleanup
+	// (see the "Static and Global Variables" section of the Google
+	// C++ Style Guide, which explicitly recommends "a pointer that
+	// is never freed" for exactly this situation). Concretely here:
+	// a PKCS#11 caller (e.g. a provider) may still be closing its
+	// own open sessions via C_CloseSession() when this destructor
+	// fires, and would then be operating on an already-destroyed
+	// singleton.
+	//
+	// The smart pointer itself is therefore heap-allocated once and
+	// deliberately never freed - the same "leaky singleton" idiom
+	// used by e.g. Drake's never_destroyed<T> - while still using it
+	// (not a raw pointer) to own and manage the SoftHSM object's own
+	// lifetime, which is destroyed exactly once, only via the
+	// explicit C_Finalize()/reset() path below.
 #ifdef HAVE_CXX11
-	static std::unique_ptr<SoftHSM> instance;
+	static std::unique_ptr<SoftHSM>* instance;
 #else
-	static std::auto_ptr<SoftHSM> instance;
+	static std::auto_ptr<SoftHSM>* instance;
 #endif
 
 	// Is the SoftHSM PKCS #11 library initialised?

src/lib/common/MutexFactory.cpp

@@ -111,12 +111,12 @@ MutexFactory::~MutexFactory()
 // Return the one-and-only instance
 MutexFactory* MutexFactory::i()
 {
-	if (!instance.get())
+	if (!instance->get())
 	{
-		instance.reset(new MutexFactory());
+		instance->reset(new MutexFactory());
 	}
 
-	return instance.get();
+	return instance->get();
 }
 
 // Get a mutex instance

src/lib/common/MutexFactory.h

@@ -112,11 +112,15 @@ class MutexFactory
 	CK_RV LockMutex(CK_VOID_PTR mutex);
 	CK_RV UnlockMutex(CK_VOID_PTR mutex);
 
-	// The one-and-only instance
+	// The one-and-only instance.
+	//
+	// A pointer to a heap-allocated, deliberately never-freed smart
+	// pointer, not a smart pointer directly: see the rationale
+	// beside SoftHSM::instance in SoftHSM.h.
 #ifdef HAVE_CXX11
-	static std::unique_ptr<MutexFactory> instance;
+	static std::unique_ptr<MutexFactory>* instance;
 #else
-	static std::auto_ptr<MutexFactory> instance;
+	static std::auto_ptr<MutexFactory>* instance;
 #endif
 
 	// The function pointers

src/lib/crypto/BotanCryptoFactory.cpp

@@ -95,18 +95,18 @@ BotanCryptoFactory::~BotanCryptoFactory()
 // Return the one-and-only instance
 BotanCryptoFactory* BotanCryptoFactory::i()
 {
-	if (!instance.get())
+	if (!instance->get())
 	{
-		instance.reset(new BotanCryptoFactory());
+		instance->reset(new BotanCryptoFactory());
 	}
 
-	return instance.get();
+	return instance->get();
 }
 
 // This will destroy the one-and-only instance.
 void BotanCryptoFactory::reset()
 {
-	instance.reset();
+	instance->reset();
 }
 
 // Create a concrete instance of a symmetric algorithm

src/lib/crypto/BotanCryptoFactory.h

@@ -81,11 +81,15 @@ class BotanCryptoFactory : public CryptoFactory
 	// Constructor
 	BotanCryptoFactory();
 
-	// The one-and-only instance
+	// The one-and-only instance.
+	//
+	// A pointer to a heap-allocated, deliberately never-freed smart
+	// pointer, not a smart pointer directly: see the rationale
+	// beside SoftHSM::instance in SoftHSM.h.
 #ifdef HAVE_CXX11
-	static std::unique_ptr<BotanCryptoFactory> instance;
+	static std::unique_ptr<BotanCryptoFactory>* instance;
 #else
-	static std::auto_ptr<BotanCryptoFactory> instance;
+	static std::auto_ptr<BotanCryptoFactory>* instance;
 #endif
 
 	// Thread specific RNG

src/lib/crypto/OSSLCryptoFactory.cpp

@@ -300,18 +300,18 @@ OSSLCryptoFactory::~OSSLCryptoFactory()
 // Return the one-and-only instance
 OSSLCryptoFactory* OSSLCryptoFactory::i()
 {
-	if (!instance.get())
+	if (!instance->get())
 	{
-		instance.reset(new OSSLCryptoFactory());
+		instance->reset(new OSSLCryptoFactory());
 	}
 
-	return instance.get();
+	return instance->get();
 }
 
 // This will destroy the one-and-only instance.
 void OSSLCryptoFactory::reset()
 {
-	instance.reset();
+	instance->reset();
 }
 
 #ifdef WITH_FIPS

src/lib/crypto/OSSLCryptoFactory.h

@@ -89,11 +89,15 @@ class OSSLCryptoFactory : public CryptoFactory
 	// Constructor
 	OSSLCryptoFactory();
 
-	// The one-and-only instance
+	// The one-and-only instance.
+	//
+	// A pointer to a heap-allocated, deliberately never-freed smart
+	// pointer, not a smart pointer directly: see the rationale
+	// beside SoftHSM::instance in SoftHSM.h.
 #ifdef HAVE_CXX11
-	static std::unique_ptr<OSSLCryptoFactory> instance;
+	static std::unique_ptr<OSSLCryptoFactory>* instance;
 #else
-	static std::auto_ptr<OSSLCryptoFactory> instance;
+	static std::auto_ptr<OSSLCryptoFactory>* instance;
 #endif
 
 #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)

src/lib/crypto/test/cryptotest.cpp

@@ -56,22 +56,22 @@
 // Initialise the one-and-only instance
 #ifdef HAVE_CXX11
 
-std::unique_ptr<MutexFactory> MutexFactory::instance(nullptr);
-std::unique_ptr<SecureMemoryRegistry> SecureMemoryRegistry::instance(nullptr);
+std::unique_ptr<MutexFactory>* MutexFactory::instance = new std::unique_ptr<MutexFactory>();
+std::unique_ptr<SecureMemoryRegistry>* SecureMemoryRegistry::instance = new std::unique_ptr<SecureMemoryRegistry>();
 #if defined(WITH_OPENSSL)
-std::unique_ptr<OSSLCryptoFactory> OSSLCryptoFactory::instance(nullptr);
+std::unique_ptr<OSSLCryptoFactory>* OSSLCryptoFactory::instance = new std::unique_ptr<OSSLCryptoFactory>();
 #else
-std::unique_ptr<BotanCryptoFactory> BotanCryptoFactory::instance(nullptr);
+std::unique_ptr<BotanCryptoFactory>* BotanCryptoFactory::instance = new std::unique_ptr<BotanCryptoFactory>();
 #endif
 
 #else
 
-std::auto_ptr<MutexFactory> MutexFactory::instance(NULL);
-std::auto_ptr<SecureMemoryRegistry> SecureMemoryRegistry::instance(NULL);
+std::auto_ptr<MutexFactory>* MutexFactory::instance = new std::auto_ptr<MutexFactory>();
+std::auto_ptr<SecureMemoryRegistry>* SecureMemoryRegistry::instance = new std::auto_ptr<SecureMemoryRegistry>();
 #if defined(WITH_OPENSSL)
-std::auto_ptr<OSSLCryptoFactory> OSSLCryptoFactory::instance(NULL);
+std::auto_ptr<OSSLCryptoFactory>* OSSLCryptoFactory::instance = new std::auto_ptr<OSSLCryptoFactory>();
 #else
-std::auto_ptr<BotanCryptoFactory> BotanCryptoFactory::instance(NULL);
+std::auto_ptr<BotanCryptoFactory>* BotanCryptoFactory::instance = new std::auto_ptr<BotanCryptoFactory>();
 #endif
 
 #endif