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]>

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.

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_;
 

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.

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_;