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 <[email protected]> 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 <[email protected]> --- 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
