Repository: kudu Updated Branches: refs/heads/master b9453a258 -> f2aeba6c0
KUDU-1749. Allow callers to disable SASL initialization The Kudu client makes use of SASL, as do other applications such as Impala that use the client. This means that both Kudu and those applications are expected to initialize SASL. The sasl_*_init() functions themselves are safe to call multiple times (they have no effect). However, sasl_set_mutex() is not safe to call multiple times, nor is it safe to call after another library has already begun using the SASL libraries. IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely caused by this issue: Impala has initialized the SASL library with no call to sasl_set_mutex(), and then during Kudu initialization we set the mutex functions to our own. Then when we call sasl_dispose(), SASL tries to acquire an internal mutex which was previously allocated to a garbage value 0x1 by the built-in "fake" mutex implementation. This crashes when we attempt to access it using our real mutex implementation. The fix has a few parts: - we have a new API which allows embedders to disable initialization of SASL. - we try to auto-detect the case in which the client has fully initialized SASL including providing a mutex implementation. In that case, we skip initialization but log a warning that the caller should disable it explicitly. - if we detect that it's initialized but no mutex implementation is provided, we'll log a warning on first usage of SASL. - if they disable initialization but didn't initialize SASL on their own, we'll return an error on first usage. The one sketchy thing about this patch is that the SASL API doesn't give a 100% public way to determine its initialization state. However, it does export a symbol which we can import that provides the necessary information. This symbol is marked as an API, so we expect it to be stable enough for usage, at least across the range of supported operating systems. Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd Reviewed-on: http://gerrit.cloudera.org:8080/5120 Reviewed-by: Adar Dembo <[email protected]> Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/f2aeba6c Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f2aeba6c Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f2aeba6c Branch: refs/heads/master Commit: f2aeba6c059ea61f9cf8984d3e84d6c27b64d463 Parents: b9453a2 Author: Todd Lipcon <[email protected]> Authored: Wed Nov 16 22:12:06 2016 -0800 Committer: Todd Lipcon <[email protected]> Committed: Fri Nov 18 04:48:33 2016 +0000 ---------------------------------------------------------------------- src/kudu/client/client.cc | 5 ++ src/kudu/client/client.h | 13 +++++ src/kudu/rpc/sasl_common.cc | 89 ++++++++++++++++++++++++++---- src/kudu/rpc/sasl_common.h | 8 +++ src/kudu/rpc/sasl_rpc-test.cc | 107 +++++++++++++++++++++++++++++++++++++ 5 files changed, 212 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/f2aeba6c/src/kudu/client/client.cc ---------------------------------------------------------------------- diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc index fb63cb2..e2c1cf6 100644 --- a/src/kudu/client/client.cc +++ b/src/kudu/client/client.cc @@ -58,6 +58,7 @@ #include "kudu/master/master.proxy.h" #include "kudu/rpc/messenger.h" #include "kudu/rpc/request_tracker.h" +#include "kudu/rpc/sasl_common.h" #include "kudu/util/init.h" #include "kudu/util/logging.h" #include "kudu/util/net/dns_resolver.h" @@ -178,6 +179,10 @@ Status SetInternalSignalNumber(int signum) { return SetStackTraceSignal(signum); } +Status DisableSaslInitialization() { + return kudu::rpc::DisableSaslInitialization(); +} + string GetShortVersionString() { return VersionInfo::GetShortVersionString(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/f2aeba6c/src/kudu/client/client.h ---------------------------------------------------------------------- diff --git a/src/kudu/client/client.h b/src/kudu/client/client.h index 728949f..62d631f 100644 --- a/src/kudu/client/client.h +++ b/src/kudu/client/client.h @@ -127,6 +127,19 @@ void KUDU_EXPORT SetVerboseLogLevel(int level); /// @return Operation result status. Status KUDU_EXPORT SetInternalSignalNumber(int signum); +/// Disable initialization of the Cyrus SASL library. Clients should call this +/// method before using the Kudu client if they are manually initializing Cyrus +/// SASL. If this method is not called, Kudu will attempt to auto-detect whether +/// SASL has been externally initialized, but it is recommended to be explicit. +/// +/// If this function is called, it must be called prior to the first construction +/// of a KuduClient object. +/// +/// NOTE: Kudu makes use of SASL from multiple threads. Thus, it's imperative +/// that embedding applications use sasl_set_mutex(3) to provide a mutex +/// implementation if they are choosing to handle SASL initialization manually. +Status KUDU_EXPORT DisableSaslInitialization(); + /// @return Short version info, i.e. a single-line version string /// identifying the Kudu client. std::string KUDU_EXPORT GetShortVersionString(); http://git-wip-us.apache.org/repos/asf/kudu/blob/f2aeba6c/src/kudu/rpc/sasl_common.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_common.cc b/src/kudu/rpc/sasl_common.cc index 7874f20..4e9aed5 100644 --- a/src/kudu/rpc/sasl_common.cc +++ b/src/kudu/rpc/sasl_common.cc @@ -25,6 +25,7 @@ #include <glog/logging.h> #include <regex.h> #include <sasl/sasl.h> +#include <sasl/saslplug.h> #include "kudu/gutil/macros.h" #include "kudu/gutil/once.h" @@ -45,6 +46,16 @@ const char* const kSaslMechGSSAPI = "GSSAPI"; // See WrapSaslCall(). static __thread string* g_auth_failure_capture = nullptr; +// Determine whether initialization was ever called +struct InitializationData { + Status status; + string app_name; +}; +static struct InitializationData* sasl_init_data; + +// If true, then we expect someone else has initialized SASL. +static bool g_disable_sasl_init = false; + // Output Sasl messages. // context: not used. // level: logging level. @@ -156,13 +167,41 @@ static int SaslMutexUnlock(void* m) { return 0; // indicates success. } +namespace internal { +void SaslSetMutex() { + sasl_set_mutex(&SaslMutexAlloc, &SaslMutexLock, &SaslMutexUnlock, &SaslMutexFree); +} +} // namespace internal -// Determine whether initialization was ever called -struct InitializationData { - Status status; - string app_name; -}; -static struct InitializationData* sasl_init_data; +// Sasl initialization detection methods. The OS X SASL library doesn't define +// the sasl_global_utils symbol, so we have to use less robust methods of +// detection. +#if defined(__APPLE__) +static bool SaslIsInitialized() { + return sasl_global_listmech() != nullptr; +} +static bool SaslMutexImplementationProvided() { + return SaslIsInitialized(); +} +#else + +// This symbol is exported by the SASL library but not defined +// in the headers. It's marked as an API in the library source, +// so seems safe to rely on. +extern "C" sasl_utils_t* sasl_global_utils; +static bool SaslIsInitialized() { + return sasl_global_utils != nullptr; +} +static bool SaslMutexImplementationProvided() { + if (!SaslIsInitialized()) return false; + void* m = sasl_global_utils->mutex_alloc(); + sasl_global_utils->mutex_free(m); + // The default implementation of mutex_alloc just returns the constant pointer 0x1. + // This is a bit of an ugly heuristic, but seems unlikely that anyone would ever + // provide a valid implementation that returns an invalid pointer value. + return m != reinterpret_cast<void*>(1); +} +#endif // Actually perform the initialization for the SASL subsystem. // Meant to be called via GoogleOnceInitArg(). @@ -171,13 +210,33 @@ static void DoSaslInit(void* app_name_char_array) { // We were getting Clang 3.4 UBSAN errors when letting GoogleOnce cast. const char* const app_name = reinterpret_cast<const char* const>(app_name_char_array); VLOG(3) << "Initializing SASL library"; - - // Make SASL thread-safe. - sasl_set_mutex(&SaslMutexAlloc, &SaslMutexLock, &SaslMutexUnlock, &SaslMutexFree); - sasl_init_data = new InitializationData(); sasl_init_data->app_name = app_name; + bool sasl_initialized = SaslIsInitialized(); + if (sasl_initialized && !g_disable_sasl_init) { + LOG(WARNING) << "SASL was initialized prior to Kudu's initialization. Skipping " + << "initialization. Call kudu::client::DisableSaslInitialization() " + << "to suppress this message."; + g_disable_sasl_init = true; + } + + if (g_disable_sasl_init) { + if (!sasl_initialized) { + sasl_init_data->status = Status::RuntimeError( + "SASL initialization was disabled, but SASL was not externally initialized."); + return; + } + if (!SaslMutexImplementationProvided()) { + LOG(WARNING) + << "SASL appears to be initialized by code outside of Kudu " + << "but was not provided with a mutex implementation! If " + << "manually initializing SASL, use sasl_set_mutex(3)."; + } + sasl_init_data->status = Status::OK(); + return; + } + internal::SaslSetMutex(); int result = sasl_client_init(&callbacks[0]); if (result != SASL_OK) { sasl_init_data->status = Status::RuntimeError("Could not initialize SASL client", @@ -195,6 +254,16 @@ static void DoSaslInit(void* app_name_char_array) { sasl_init_data->status = Status::OK(); } +Status DisableSaslInitialization() { + if (g_disable_sasl_init) return Status::OK(); + if (sasl_init_data != nullptr) { + return Status::IllegalState("SASL already initialized. Initialization can only be disabled " + "before first usage."); + } + g_disable_sasl_init = true; + return Status::OK(); +} + Status SaslInit(const char* app_name) { // Only execute SASL initialization once static GoogleOnceType once = GOOGLE_ONCE_INIT; http://git-wip-us.apache.org/repos/asf/kudu/blob/f2aeba6c/src/kudu/rpc/sasl_common.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_common.h b/src/kudu/rpc/sasl_common.h index 4492d82..cf1ee27 100644 --- a/src/kudu/rpc/sasl_common.h +++ b/src/kudu/rpc/sasl_common.h @@ -56,6 +56,9 @@ extern const char* const kSaslMechGSSAPI; // This function should NOT be called during static initialization. Status SaslInit(const char* app_name); +// Disable Kudu's initialization of SASL. See equivalent method in client.h. +Status DisableSaslInitialization(); + // Wrap a call into the SASL library. 'call' should be a lambda which // returns a SASL error code. // @@ -107,6 +110,11 @@ struct SaslMechanism { static const char* name_of(Type val); }; +// Internals exposed in the header for test purposes. +namespace internal { +void SaslSetMutex(); +} // namespace internal + } // namespace rpc } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/f2aeba6c/src/kudu/rpc/sasl_rpc-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_rpc-test.cc b/src/kudu/rpc/sasl_rpc-test.cc index bdb15b2..7aecbb7 100644 --- a/src/kudu/rpc/sasl_rpc-test.cc +++ b/src/kudu/rpc/sasl_rpc-test.cc @@ -39,6 +39,7 @@ #include "kudu/util/monotime.h" #include "kudu/util/net/sockaddr.h" #include "kudu/util/net/socket.h" +#include "kudu/util/subprocess.h" using std::string; using std::thread; @@ -54,6 +55,10 @@ using std::thread; #define KRB5_VERSION_LE_1_10 #endif +DEFINE_bool(is_test_child, false, + "Used by tests which require clean processes. " + "See TestDisableInit."); + namespace kudu { namespace rpc { @@ -477,5 +482,107 @@ TEST_F(TestSaslRpc, TestServerTimeout) { //////////////////////////////////////////////////////////////////////////////// +// This suite of tests ensure that applications that embed the Kudu client are +// able to externally handle the initialization of SASL. See KUDU-1749 and +// IMPALA-4497 for context. +// +// The tests are a bit tricky because the initialization of SASL is static state +// that we can't easily clear/reset between test cases. So, each test invokes +// itself as a subprocess with the appropriate --gtest_filter line as well as a +// special flag to indicate that it is the test child running. +class TestDisableInit : public KuduTest { + protected: + // Run the lambda 'f' in a newly-started process, capturing its stderr + // into 'stderr'. + template<class TestFunc> + void DoTest(const TestFunc& f, string* stderr = nullptr) { + if (FLAGS_is_test_child) { + f(); + return; + } + + // Invoke the currently-running test case in a new subprocess. + string filter_flag = strings::Substitute("--gtest_filter=$0.$1", + CURRENT_TEST_CASE_NAME(), CURRENT_TEST_NAME()); + string executable_path; + CHECK_OK(env_->GetExecutablePath(&executable_path)); + string stdout; + Status s = Subprocess::Call({ executable_path, "test", filter_flag, "--is_test_child" }, + "" /* stdin */, + &stdout, + stderr); + ASSERT_TRUE(s.ok()) << "Test failed: " << stdout; + } +}; + +// Test disabling SASL but not actually properly initializing it before usage. +TEST_F(TestDisableInit, TestDisableSasl_NotInitialized) { + DoTest([]() { + CHECK_OK(DisableSaslInitialization()); + Status s = SaslInit("kudu"); + ASSERT_STR_CONTAINS(s.ToString(), "was disabled, but SASL was not externally initialized"); + }); +} + +// Test disabling SASL with proper initialization by some other app. +TEST_F(TestDisableInit, TestDisableSasl_Good) { + DoTest([]() { + rpc::internal::SaslSetMutex(); + sasl_client_init(NULL); + CHECK_OK(DisableSaslInitialization()); + ASSERT_OK(SaslInit("kudu")); + }); +} + +// Test a client which inits SASL itself but doesn't remember to disable Kudu's +// SASL initialization. +TEST_F(TestDisableInit, TestMultipleSaslInit) { + string stderr; + DoTest([]() { + rpc::internal::SaslSetMutex(); + sasl_client_init(NULL); + ASSERT_OK(SaslInit("kudu")); + }, &stderr); + // If we are the parent, we should see the warning from the child that it automatically + // skipped initialization because it detected that it was already initialized. + if (!FLAGS_is_test_child) { + ASSERT_STR_CONTAINS(stderr, "Skipping initialization"); + } +} + +// We are not able to detect mutexes not being set with the macOS version of libsasl. +#ifndef __APPLE__ +// Test disabling SASL but not remembering to initialize the SASL mutex support. This +// should succeed but generate a warning. +TEST_F(TestDisableInit, TestDisableSasl_NoMutexImpl) { + string stderr; + DoTest([]() { + sasl_client_init(NULL); + CHECK_OK(DisableSaslInitialization()); + ASSERT_OK(SaslInit("kudu")); + }, &stderr); + // If we are the parent, we should see the warning from the child. + if (!FLAGS_is_test_child) { + ASSERT_STR_CONTAINS(stderr, "not provided with a mutex implementation"); + } +} + +// Test a client which inits SASL itself but doesn't remember to disable Kudu's +// SASL initialization. +TEST_F(TestDisableInit, TestMultipleSaslInit_NoMutexImpl) { + string stderr; + DoTest([]() { + sasl_client_init(NULL); + ASSERT_OK(SaslInit("kudu")); + }, &stderr); + // If we are the parent, we should see the warning from the child that it automatically + // skipped initialization because it detected that it was already initialized. + if (!FLAGS_is_test_child) { + ASSERT_STR_CONTAINS(stderr, "Skipping initialization"); + ASSERT_STR_CONTAINS(stderr, "not provided with a mutex implementation"); + } +} +#endif + } // namespace rpc } // namespace kudu
