Repository: kudu Updated Branches: refs/heads/master 7138468a5 -> 1096d663f
Free SASL connection objects when negotiation completes The SASL connections are only currently used during negotiation, and then can be dropped following that. So, it makes sense to dispose of the objects as soon as possible from a memory consumption standpoint, since they probably hold some buffers. More importantly, though, this also works around a bug I saw occasionally when running a Kerberized Kudu CLI: - the CLI main would make some RPC call and get an "unauthorized" status. - it would then drop its reference to the KuduClient and exit. - this would call Messenger::AllExternalReferencesDropped() which initiates an asynchronous shutdown of the reactor threads. - the main thread would get to 'exit()' and start unloading dynamic libraries, including libkrb5. - the reactor thread would call sasl_dispose on a sasl_connection_t during its shutdown sequence, which would crash with an assertion failure in k5_mutex.h if this happened after krb5 was unloaded. Rather than futz with the shutdown sequence of the reactor, it was much simpler to just dispose the connections earlier as done in this patch. Change-Id: Ib7aada1e44a80af94c5c069e9f583aedcd78a68b Reviewed-on: http://gerrit.cloudera.org:8080/4761 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/1096d663 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/1096d663 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/1096d663 Branch: refs/heads/master Commit: 1096d663f36da19b211dd4fdf76fca10f842b8b8 Parents: 7138468 Author: Todd Lipcon <[email protected]> Authored: Wed Oct 19 11:02:31 2016 -0700 Committer: Todd Lipcon <[email protected]> Committed: Thu Oct 20 22:02:24 2016 +0000 ---------------------------------------------------------------------- src/kudu/rpc/sasl_client.cc | 10 ++++++++++ src/kudu/rpc/sasl_client.h | 2 ++ src/kudu/rpc/sasl_server.cc | 8 +++++++- src/kudu/rpc/sasl_server.h | 2 ++ 4 files changed, 21 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/1096d663/src/kudu/rpc/sasl_client.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_client.cc b/src/kudu/rpc/sasl_client.cc index 3ac47f8..cc252b2 100644 --- a/src/kudu/rpc/sasl_client.cc +++ b/src/kudu/rpc/sasl_client.cc @@ -38,6 +38,7 @@ #include "kudu/util/faststring.h" #include "kudu/util/net/sockaddr.h" #include "kudu/util/net/socket.h" +#include "kudu/util/scoped_cleanup.h" #include "kudu/util/trace.h" namespace kudu { @@ -171,6 +172,15 @@ Status SaslClient::Init(const string& service_type) { } Status SaslClient::Negotiate() { + // After negotiation, we no longer need the SASL library object, so + // may as well free its memory since the connection may be long-lived. + // Additionally, this works around a SEGV seen at process shutdown time: + // if we still have SASL objects retained by Reactor when the process + // is exiting, the SASL libraries may start destructing global state + // and cause a crash when we sasl_dispose the connection. + auto cleanup = MakeScopedCleanup([&]() { + sasl_conn_.reset(); + }); TRACE("Called SaslClient::Negotiate()"); // Ensure we called exactly once, and in the right order. http://git-wip-us.apache.org/repos/asf/kudu/blob/1096d663/src/kudu/rpc/sasl_client.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_client.h b/src/kudu/rpc/sasl_client.h index a01865e..d98942a 100644 --- a/src/kudu/rpc/sasl_client.h +++ b/src/kudu/rpc/sasl_client.h @@ -146,6 +146,8 @@ class SaslClient { string app_name_; Socket sock_; std::vector<sasl_callback_t> callbacks_; + // The SASL connection object. This is initialized in Init() and + // freed after Negotiate() completes (regardless whether it was successful). gscoped_ptr<sasl_conn_t, SaslDeleter> sasl_conn_; SaslHelper helper_; http://git-wip-us.apache.org/repos/asf/kudu/blob/1096d663/src/kudu/rpc/sasl_server.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_server.cc b/src/kudu/rpc/sasl_server.cc index 5cb5699..bdacc83 100644 --- a/src/kudu/rpc/sasl_server.cc +++ b/src/kudu/rpc/sasl_server.cc @@ -28,12 +28,13 @@ #include "kudu/gutil/map-util.h" #include "kudu/gutil/stringprintf.h" #include "kudu/gutil/strings/split.h" -#include "kudu/rpc/blocking_ops.h" #include "kudu/rpc/auth_store.h" +#include "kudu/rpc/blocking_ops.h" #include "kudu/rpc/constants.h" #include "kudu/rpc/serialization.h" #include "kudu/util/net/sockaddr.h" #include "kudu/util/net/socket.h" +#include "kudu/util/scoped_cleanup.h" #include "kudu/util/trace.h" namespace kudu { @@ -146,6 +147,11 @@ Status SaslServer::Init(const string& service_type) { } Status SaslServer::Negotiate() { + // After negotiation, we no longer need the SASL library object, so + // may as well free its memory since the connection may be long-lived. + auto cleanup = MakeScopedCleanup([&]() { + sasl_conn_.reset(); + }); DVLOG(4) << "Called SaslServer::Negotiate()"; // Ensure we are called exactly once, and in the right order. http://git-wip-us.apache.org/repos/asf/kudu/blob/1096d663/src/kudu/rpc/sasl_server.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/sasl_server.h b/src/kudu/rpc/sasl_server.h index 2f9c194..0614139 100644 --- a/src/kudu/rpc/sasl_server.h +++ b/src/kudu/rpc/sasl_server.h @@ -144,6 +144,8 @@ class SaslServer { string app_name_; Socket sock_; std::vector<sasl_callback_t> callbacks_; + // The SASL connection object. This is initialized in Init() and + // freed after Negotiate() completes (regardless whether it was successful). gscoped_ptr<sasl_conn_t, SaslDeleter> sasl_conn_; SaslHelper helper_;
