From 6729b294103f169ca653386dd763aaaa55bf4409 Mon Sep 17 00:00:00 2001 From: Jeffrey Walton Date: Mon, 3 Dec 2018 06:33:15 -0500 Subject: [PATCH] Move DEFAULT_CHANNEL and AAD_CHANNEL defs into cryptlib.cpp (GH #751) The library used to provide DEFAULT_CHANNEL and AAD_CHANNEL this way. We experienced Static Initialization Order Fiasco crashes on occassion, so we moved them into cryptlib.h with internal linkage. The cost was, each translation unit got a copy of the strings which contributed to bloat. Issue 751 shows Clang compiles the global constructors for DEFAULT_CHANNEL and AAD_CHANNEL above the base ISA so we caught crashes on OS X with down-level hardware. We are now at a "pick your poison" point. We selected Static Initialization Order Fiasco because it seems to be less prevalent. Hat tip to the C++ Committee for allowing this problem to fester for three decades. --- cryptlib.cpp | 36 ++++++++++++++++++++++++++++++++++++ cryptlib.h | 38 ++++---------------------------------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/cryptlib.cpp b/cryptlib.cpp index 81ff4b16..6399ec53 100644 --- a/cryptlib.cpp +++ b/cryptlib.cpp @@ -986,6 +986,42 @@ int LibraryVersion(CRYPTOPP_NOINLINE_DOTDOTDOT) return CRYPTOPP_BUILD_VERSION; } +class NullNameValuePairs : public NameValuePairs +{ +public: + NullNameValuePairs() {} // Clang complains a default ctor must be avilable + bool GetVoidValue(const char *name, const std::type_info &valueType, void *pValue) const + {CRYPTOPP_UNUSED(name); CRYPTOPP_UNUSED(valueType); CRYPTOPP_UNUSED(pValue); return false;} +}; + +#if HAVE_GCC_INIT_PRIORITY +CRYPTOPP_COMPILE_ASSERT(CRYPTOPP_INIT_PRIORITY >= 101); +const std::string DEFAULT_CHANNEL __attribute__ ((init_priority (CRYPTOPP_INIT_PRIORITY + 25))) = ""; +const std::string AAD_CHANNEL __attribute__ ((init_priority (CRYPTOPP_INIT_PRIORITY + 26))) = "AAD"; +const NullNameValuePairs s_nullNameValuePairs __attribute__ ((init_priority (CRYPTOPP_INIT_PRIORITY + 27))); +const NameValuePairs& g_nullNameValuePairs = s_nullNameValuePairs; +#elif HAVE_MSC_INIT_PRIORITY +#pragma warning(disable: 4073) +#pragma init_seg(lib) +const std::string DEFAULT_CHANNEL = ""; +const std::string AAD_CHANNEL = "AAD"; +const NullNameValuePairs s_nullNameValuePairs; +const NameValuePairs& g_nullNameValuePairs = s_nullNameValuePairs; +#pragma warning(default: 4073) +#elif HAVE_XLC_INIT_PRIORITY +#pragma priority(260) +const std::string DEFAULT_CHANNEL = ""; +const std::string AAD_CHANNEL = "AAD"; +const NullNameValuePairs s_nullNameValuePairs; +const NameValuePairs& g_nullNameValuePairs = s_nullNameValuePairs; +#else +static const std::string s1(""), s2("AAD"); +const std::string DEFAULT_CHANNEL = s1; +const std::string AAD_CHANNEL = s2; +const simple_ptr s_pNullNameValuePairs(new NullNameValuePairs); +const NameValuePairs &g_nullNameValuePairs = *s_pNullNameValuePairs.m_p; +#endif + NAMESPACE_END // CryptoPP #endif // CRYPTOPP_IMPORTS diff --git a/cryptlib.h b/cryptlib.h index ebe14c4c..2f36c14a 100644 --- a/cryptlib.h +++ b/cryptlib.h @@ -468,28 +468,6 @@ public: CRYPTOPP_DLL virtual bool GetVoidValue(const char *name, const std::type_info &valueType, void *pValue) const =0; }; -/// \brief Interface for retrieving values given their names -/// \details This class is used when no names or values are present. Typically a program uses -/// g_nullNameValuePairs rather than creating its own NullNameValuePairs object. -/// \details NullNameValuePairs always existed in cryptlib.cpp. Crypto++ 6.0 moved NullNameValuePairs -/// into the header. This allowed the library to define g_nullNameValuePairs in the header rather -/// than declaring it as extern and placing the definition in the source file. As an external definition -/// the string g_nullNameValuePairs was subject to static initialization order fiasco problems. -/// \sa NameValuePairs, g_nullNameValuePairs, -/// NameValuePairs on the Crypto++ wiki -class NullNameValuePairs : public NameValuePairs -{ -public: - NullNameValuePairs() {} // Clang complains a default ctor must be avilable - bool GetVoidValue(const char *name, const std::type_info &valueType, void *pValue) const - {CRYPTOPP_UNUSED(name); CRYPTOPP_UNUSED(valueType); CRYPTOPP_UNUSED(pValue); return false;} -}; - -// More static initialization order fiasco workarounds. These definitions cannot be extern and -// cannot be static class members because they require a single definition in a source file. -// User programs should use g_nullNameValuePairs rather than s_nullNameValuePairs. -static const NullNameValuePairs s_nullNameValuePairs; - // Doxygen cannot handle initialization #if CRYPTOPP_DOXYGEN_PROCESSING /// \brief Default channel for BufferedTransformation @@ -510,21 +488,13 @@ const std::string AAD_CHANNEL; /// \details Crypto++ 6.0 placed g_nullNameValuePairs in the header, rather than declaring it as extern /// and placing the definition in the source file. As an external definition the g_nullNameValuePairs /// was subject to static initialization order fiasco problems. -const NameValuePairs g_nullNameValuePairs; +const NameValuePairs& g_nullNameValuePairs; // Sun Studio 12.3 and earlier can't handle NameValuePairs initialization -#elif defined(__SUNPRO_CC) && (__SUNPRO_CC < 0x5130) -static const std::string DEFAULT_CHANNEL; -static const std::string AAD_CHANNEL = "AAD"; -static const NameValuePairs& g_nullNameValuePairs = s_nullNameValuePairs; - -// We don't really want static here since it detracts from public symbol visibility, but the Windows -// DLL fails to compile when the symbols are only const. Apparently Microsoft compilers don't treat -// const the same as static in a translation unit for visibility under C++. #else -static const std::string DEFAULT_CHANNEL; -static const std::string AAD_CHANNEL("AAD"); -static const NameValuePairs& g_nullNameValuePairs(s_nullNameValuePairs); +extern const std::string DEFAULT_CHANNEL; +extern const std::string AAD_CHANNEL; +extern const NameValuePairs& g_nullNameValuePairs; #endif // Document additional name spaces which show up elsewhere in the sources.