[impala] 01/02: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()

[kw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

kwho pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 681b256627e5e569778d4996bbdf530e30c528eb
Author: Michael Ho <kw...@cloudera.com>
AuthorDate: Wed Feb 20 16:51:26 2019 -0800

    IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
    
    krb5_init_context() sets the field 'default_realm' in a krb5_context
    object to 0. Upon first call to krb5_parse_name() with a principal
    without realm specified (e.g. foo/bar), 'default_realm' in the
    krb5_context object is lazily initialized.
    
    When more than one negotiation threads are configured, it's possible
    for multiple threads to call CanonicalizeKrb5Principal() in parallel.
    CanonicalizeKrb5Principal() in turn calls krb5_parse_name(g_krb5_ctx, ...)
    with no lock held. In addition, krb5_parse_name() is not thread safe as
    it lazily initializes 'context->default_realm' without holding lock.
    Consequently, 'g_krb5_ctx' which is shared and not supposed to be modified
    after initialization may be inadvertently modified concurrently by multiple
    threads, leading to crashes (e.g. double free) or errors.
    
    This change works around the problem by initializing 'g_krb5_ctx->default_realm'
    once in InitKrb5Ctx() by calling krb5_get_default_realm().
    
    Impala internally calls InitAuth() which calls InitKerberosForServer()
    which ends up calling InitKrb5Ctx() in a single threaded environment.
    
    Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
    Reviewed-on: http://gerrit.cloudera.org:8080/12545
    Reviewed-by: Alexey Serbin <as...@cloudera.com>
    Tested-by: Kudu Jenkins
    Reviewed-on: http://gerrit.cloudera.org:8080/12553
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/kudu/security/init.cc | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/be/src/kudu/security/init.cc b/be/src/kudu/security/init.cc
index ee754eb..64ee1a5 100644
--- a/be/src/kudu/security/init.cc
+++ b/be/src/kudu/security/init.cc
@@ -151,6 +151,18 @@ void InitKrb5Ctx() {
   static std::once_flag once;
   std::call_once(once, [&]() {
       CHECK_EQ(krb5_init_context(&g_krb5_ctx), 0);
+      // Work around the lack of thread safety in krb5_parse_name() by implicitly
+      // initializing g_krb5_ctx->default_realm once. The assumption is that this
+      // function is called once in a single thread environment during initialization.
+      //
+      // TODO(KUDU-2706): Fix unsafe sharing of 'g_krb5_ctx'.
+      // According to Kerberos documentation
+      // (https://github.com/krb5/krb5/blob/master/doc/threads.txt), any use of
+      // krb5_context must be confined to one thread at a time by the application code.
+      // The current way of sharing of 'g_krb5_ctx' between threads is actually unsafe.
+      char* unused_realm;
+      CHECK_EQ(krb5_get_default_realm(g_krb5_ctx, &unused_realm), 0);
+      krb5_free_default_realm(g_krb5_ctx, unused_realm);
     });
 }