fix possible race condition in Singleton::Ref()

tolerate double destruction of Singleton and g_nullNameValuePairs
fix #include of standard headers

7 files changed, 45 insertions, 57 deletions

diff --git a/cryptlib.cpp b/cryptlib.cpp
index 0e5bd248..dadd9cec 100644
--- a/cryptlib.cpp
+++ b/cryptlib.cpp
@@ -30,7 +30,14 @@ const std::string DEFAULT_CHANNEL;
 const std::string AAD_CHANNEL = "AAD";
 const std::string &BufferedTransformation::NULL_CHANNEL = DEFAULT_CHANNEL;
 
-const NullNameValuePairs g_nullNameValuePairs;
+class NullNameValuePairs : public NameValuePairs
+{
+public:
+	bool GetVoidValue(const char *name, const std::type_info &valueType, void *pValue) const {return false;}
+};
+
+simple_ptr<NullNameValuePairs> s_pNullNameValuePairs(new NullNameValuePairs);
+const NameValuePairs &g_nullNameValuePairs = *s_pNullNameValuePairs.m_p;
 
 BufferedTransformation & TheBitBucket()
 {
diff --git a/cryptlib.h b/cryptlib.h
index 15cd6dad..b5faa58a 100644
--- a/cryptlib.h
+++ b/cryptlib.h
@@ -318,14 +318,7 @@ DOCUMENTED_NAMESPACE_BEGIN(Name)
 DOCUMENTED_NAMESPACE_END
 
 //! empty set of name-value pairs
-class CRYPTOPP_DLL NullNameValuePairs : public NameValuePairs
-{
-public:
-	bool GetVoidValue(const char *name, const std::type_info &valueType, void *pValue) const {return false;}
-};
-
-//! _
-extern CRYPTOPP_DLL const NullNameValuePairs g_nullNameValuePairs;
+extern CRYPTOPP_DLL const NameValuePairs &g_nullNameValuePairs;
 
 // ********************************************************
 
diff --git a/misc.h b/misc.h
index ac1cbda0..540c0e87 100644
--- a/misc.h
+++ b/misc.h
@@ -6,7 +6,6 @@
 #include <string.h>		// for memcpy and memmove
 
 #ifdef _MSC_VER
-	#include <stdlib.h>
 	#if _MSC_VER >= 1400
 		// VC2005 workaround: disable declarations that conflict with winnt.h
 		#define _interlockedbittestandset CRYPTOPP_DISABLED_INTRINSIC_1
@@ -101,9 +100,9 @@ struct NewObject
 	T* operator()() const {return new T;}
 };
 
-/*! This function safely initializes a static object in a multithreaded environment without using locks.
-	It may leak memory when two threads try to initialize the static object at the same time
-	but this should be acceptable since each static object is only initialized once per session.
+/*! This function safely initializes a static object in a multithreaded environment without using locks (for portability).
+	Note that if two threads call Ref() at the same time, they may get back different references, and one object 
+	may end up being memory leaked. This is by design.
 */
 template <class T, class F = NewObject<T>, int instance=0>
 class Singleton
@@ -121,31 +120,23 @@ private:
 template <class T, class F, int instance>
 const T & Singleton<T, F, instance>::Ref(CRYPTOPP_NOINLINE_DOTDOTDOT) const
 {
-	static simple_ptr<T> s_pObject;
-	static volatile char s_objectState = 0;
+	static volatile simple_ptr<T> s_pObject;
+	T *p = s_pObject.m_p;
 
-retry:
-	switch (s_objectState)
+	if (p)
+		return *p;
+
+	T *newObject = m_objectFactory();
+	p = s_pObject.m_p;
+
+	if (p)
 	{
-	case 0:
-		s_objectState = 1;
-		try
-		{
-			s_pObject.m_p = m_objectFactory();
-		}
-		catch(...)
-		{
-			s_objectState = 0;
-			throw;
-		}
-		s_objectState = 2;
-		break;
-	case 1:
-		goto retry;
-	default:
-		break;
+		delete newObject;
+		return *p;
 	}
-	return *s_pObject.m_p;
+
+	s_pObject.m_p = newObject;
+	return *newObject;
 }
 
 // ************** misc functions ***************
@@ -555,7 +546,7 @@ static std::string StringNarrow(const wchar_t *str, bool throwOnError = true)
 #pragma warning(disable: 4996)	//  'wcstombs': This function or variable may be unsafe.
 #endif
 	size_t size = wcstombs(NULL, str, 0);
-	if (size == -1)
+	if (size == size_t(0)-1)
 	{
 		if (throwOnError)
 			throw InvalidArgument("StringNarrow: wcstombs() call failed");
diff --git a/rijndael.cpp b/rijndael.cpp
index b3d986b8..a39b65d0 100644
--- a/rijndael.cpp
+++ b/rijndael.cpp
@@ -69,14 +69,6 @@ being unloaded from L1 cache, until that round is finished.
 #include "misc.h"
 #include "cpu.h"
 
-#ifdef __sun
-#include <alloca.h>
-#endif
-
-#ifdef __MINGW32__
-#include <malloc.h>
-#endif
-
 NAMESPACE_BEGIN(CryptoPP)
 
 #ifdef CRYPTOPP_ALLOW_UNALIGNED_DATA_ACCESS
diff --git a/secblock.h b/secblock.h
index 6f3aa63b..760633e6 100644
--- a/secblock.h
+++ b/secblock.h
@@ -9,8 +9,6 @@
 
 #if defined(CRYPTOPP_MEMALIGN_AVAILABLE) || defined(CRYPTOPP_MM_MALLOC_AVAILABLE) || defined(QNX)
 	#include <malloc.h>
-#else
-	#include <stdlib.h>
 #endif
 
 NAMESPACE_BEGIN(CryptoPP)
@@ -352,8 +350,11 @@ public:
 	//! copy contents and size from another SecBlock
 	void Assign(const SecBlock<T, A> &t)
 	{
-		New(t.m_size);
-		memcpy_s(m_ptr, m_size*sizeof(T), t.m_ptr, m_size*sizeof(T));
+		if (this != &t)
+		{
+			New(t.m_size);
+			memcpy_s(m_ptr, m_size*sizeof(T), t.m_ptr, m_size*sizeof(T));
+		}
 	}
 
 	SecBlock<T, A>& operator=(const SecBlock<T, A> &t)
diff --git a/smartptr.h b/smartptr.h
index 6b4040e9..a0a727ed 100644
--- a/smartptr.h
+++ b/smartptr.h
@@ -9,8 +9,8 @@ NAMESPACE_BEGIN(CryptoPP)
 template <class T> class simple_ptr
 {
 public:
-	simple_ptr() : m_p(NULL) {}
-	~simple_ptr() {delete m_p;}
+	simple_ptr(T *p = NULL) : m_p(p) {}
+	~simple_ptr() {delete m_p; m_p = NULL;}		// set m_p to NULL so double destruction (which might occur in Singleton) will be harmless
 	T *m_p;
 };
 
diff --git a/stdcpp.h b/stdcpp.h
index 9a468ab6..234bf549 100644
--- a/stdcpp.h
+++ b/stdcpp.h
@@ -4,24 +4,28 @@
 #include <stddef.h>
 #include <assert.h>
 #include <limits.h>
+#include <stdlib.h>
+#include <string.h>
 #include <memory>
 #include <string>
 #include <exception>
 #include <typeinfo>
-
-
-#ifdef _MSC_VER
-#include <string.h>	// CodeWarrior doesn't have memory.h
 #include <algorithm>
 #include <map>
 #include <vector>
 
-// re-disable this
-#pragma warning(disable: 4231)
+// for alloca
+#ifdef __sun
+#include <alloca.h>
+#elif defined(__MINGW32__)
+#include <malloc.h>
 #endif
 
-#if defined(_MSC_VER) && defined(_CRTAPI1)
+#ifdef _MSC_VER
+#pragma warning(disable: 4231)	// re-disable this
+#ifdef _CRTAPI1
 #define CRYPTOPP_MSVCRT6
 #endif
+#endif
 
 #endif