You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2022/02/01 21:57:56 UTC
[kudu] 03/03: [asan] improve asan success rate, kerberos context init and destroy
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit f4fa4d22f2dc122c2c130fd08142317869729e28
Author: shenxingwuying <sh...@gmail.com>
AuthorDate: Mon Jan 10 19:00:36 2022 +0800
[asan] improve asan success rate, kerberos context init and destroy
kerberos's init context is global, KinitContext* g_kinit_ctx,
it used new operator, but no delete operator.
It release the memory by os when program stopping.
Some asan tests may failed, when MiniCluster restart/stop.
KinitContext should be deleted safely.
Change-Id: I76a639e35fdf951787f14e0603e73e9e19da6691
Reviewed-on: http://gerrit.cloudera.org:8080/18135
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
src/kudu/security/init.cc | 77 +++++++++++++++++++++++++--------------
src/kudu/security/init.h | 3 ++
src/kudu/security/kinit_context.h | 13 +++++++
src/kudu/server/server_base.cc | 2 +
4 files changed, 68 insertions(+), 27 deletions(-)
diff --git a/src/kudu/security/init.cc b/src/kudu/security/init.cc
index 1a512dc..8d59d51 100644
--- a/src/kudu/security/init.cc
+++ b/src/kudu/security/init.cc
@@ -37,9 +37,11 @@
#include <glog/logging.h>
#include "kudu/gutil/macros.h"
+#include "kudu/gutil/ref_counted.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/gutil/strings/util.h"
#include "kudu/security/kinit_context.h"
+#include "kudu/util/countdown_latch.h"
#include "kudu/util/flag_tags.h"
#include "kudu/util/monotime.h"
#include "kudu/util/net/net_util.h"
@@ -94,7 +96,7 @@ namespace kudu {
namespace security {
// Global instance of the context used by the kinit/reacquire thread.
-KinitContext* g_kinit_ctx;
+KinitContext* g_kinit_ctx = nullptr;
namespace {
@@ -160,34 +162,13 @@ Status Krb5UnparseName(krb5_principal princ, string* name) {
return Status::OK();
}
-// Periodically calls DoRenewal().
-void RenewThread() {
- uint32_t failure_retries = 0;
- while (true) {
- // This thread is run immediately after the first Kinit, so sleep first.
- int64_t renew_interval_s = g_kinit_ctx->GetNextRenewInterval(failure_retries);
- if (failure_retries > 0) {
- // Log in the abnormal case where something failed.
- LOG(INFO) << Substitute("Renew thread sleeping after $0 failures for $1s",
- failure_retries, renew_interval_s);
- }
- SleepFor(MonoDelta::FromSeconds(renew_interval_s));
-
- Status s = g_kinit_ctx->DoRenewal();
- WARN_NOT_OK(s, "Kerberos reacquire error: ");
- if (!s.ok()) {
- ++failure_retries;
- } else {
- failure_retries = 0;
- }
- }
-}
} // anonymous namespace
-KinitContext::KinitContext() {}
+KinitContext::KinitContext() : stop_latch_(1) {}
KinitContext::~KinitContext() {
// Free memory associated with these objects.
+ Kdestroy();
if (principal_ != nullptr) krb5_free_principal(g_krb5_ctx, principal_);
if (keytab_ != nullptr) krb5_kt_close(g_krb5_ctx, keytab_);
if (ccache_ != nullptr) krb5_cc_close(g_krb5_ctx, ccache_);
@@ -329,7 +310,19 @@ Status KinitContext::Kinit(const string& keytab_path, const string& principal) {
KRB5_RETURN_NOT_OK_PREPEND(krb5_get_init_creds_opt_alloc(g_krb5_ctx, &opts_),
"unable to allocate get_init_creds_opt struct");
- return KinitInternal();
+ RETURN_NOT_OK(KinitInternal());
+
+ // Start the thread to renew and reacquire Kerberos tickets.
+ RETURN_NOT_OK(Thread::Create("kerberos", "reacquire thread",
+ [this]() { this->RenewThread(); }, &reacquire_thread_));
+ return Status::OK();
+}
+
+void KinitContext::Kdestroy() {
+ stop_latch_.CountDown();
+ if (reacquire_thread_.get() != nullptr) {
+ reacquire_thread_->Join();
+ }
}
Status KinitContext::KinitInternal() {
@@ -372,6 +365,30 @@ Status KinitContext::KinitInternal() {
return Status::OK();
}
+// Periodically calls DoRenewal().
+void KinitContext::RenewThread() {
+ uint32_t failure_retries = 0;
+ int64_t renew_interval_s = GetNextRenewInterval(failure_retries);
+ while (!stop_latch_.WaitFor(MonoDelta::FromSeconds(renew_interval_s))) {
+ Status s = DoRenewal();
+ WARN_NOT_OK(s, "Kerberos reacquire error: ");
+ if (!s.ok()) {
+ ++failure_retries;
+ } else {
+ failure_retries = 0;
+ }
+
+ if (failure_retries > 0) {
+ // Log in the abnormal case where something failed.
+ LOG(INFO) << Substitute("Renew thread sleeping after $0 failures for $1s",
+ failure_retries, renew_interval_s);
+ }
+
+ // This thread is run immediately after the first Kinit, so sleep first.
+ renew_interval_s = GetNextRenewInterval(failure_retries);
+ }
+}
+
RWMutex* KerberosReinitLock() {
return g_kerberos_reinit_lock;
}
@@ -495,8 +512,14 @@ Status InitKerberosForServer(const std::string& raw_principal, const std::string
RETURN_NOT_OK_PREPEND(g_kinit_ctx->Kinit(
keytab_file, configured_principal), "unable to kinit");
- // Start the thread to renew and reacquire Kerberos tickets.
- return Thread::Create("kerberos", "reacquire thread", &RenewThread, nullptr);
+ return Status::OK();
+}
+
+void DestroyKerberosForServer() {
+ if (g_kinit_ctx == nullptr) return;
+
+ delete g_kinit_ctx;
+ g_kinit_ctx = nullptr;
}
string GetKrb5ConfigFile() {
diff --git a/src/kudu/security/init.h b/src/kudu/security/init.h
index 8b76e94..aa8a8b8 100644
--- a/src/kudu/security/init.h
+++ b/src/kudu/security/init.h
@@ -55,6 +55,9 @@ Status InitKerberosForServer(const std::string& raw_principal,
const std::string& krb5ccname = kKrb5CCName,
bool disable_krb5_replay_cache = true);
+// Destroy Kerberos for a server.
+void DestroyKerberosForServer();
+
// Returns the process lock 'kerberos_reinit_lock'
// This lock is taken in write mode while the ticket is being reacquired, and
// taken in read mode before using the SASL library which might require a ticket.
diff --git a/src/kudu/security/kinit_context.h b/src/kudu/security/kinit_context.h
index a6c2b41..9a9e644 100644
--- a/src/kudu/security/kinit_context.h
+++ b/src/kudu/security/kinit_context.h
@@ -21,7 +21,9 @@
#include <krb5/krb5.h>
+#include "kudu/gutil/strings/substitute.h"
#include "kudu/util/status.h"
+#include "kudu/util/thread.h"
namespace kudu {
namespace security {
@@ -63,6 +65,12 @@ class KinitContext {
private:
Status KinitInternal();
+ // Safe stop the renewal thread before destroying KinitContext
+ void Kdestroy();
+
+ // Periodically calls DoRenewal().
+ void RenewThread();
+
// Helper for DoRenewal() that tries to do a renewal. On success, returns OK and sets
// *found_in_cache = true. If there is an error doing the renewal itself, returns an
// error. If the TGT to be renewed was not found in the cache, return OK and set
@@ -79,6 +87,11 @@ class KinitContext {
// This is the time that the current TGT in use expires.
int32_t ticket_end_timestamp_;
+
+ // To stop reacquire_thread_ when process stopping.
+ CountDownLatch stop_latch_;
+ // A thread to renew and reacquire Kerberos credentials.
+ scoped_refptr<Thread> reacquire_thread_;
};
} // namespace security
diff --git a/src/kudu/server/server_base.cc b/src/kudu/server/server_base.cc
index ec44729..6123f77 100644
--- a/src/kudu/server/server_base.cc
+++ b/src/kudu/server/server_base.cc
@@ -869,6 +869,8 @@ void ServerBase::ShutdownImpl() {
tcmalloc_memory_gc_thread_->Join();
}
#endif
+
+ security::DestroyKerberosForServer();
}
#ifdef TCMALLOC_ENABLED