You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2019/02/22 06:45:33 UTC
[Impala-ASF-CR] IMPALA-8239 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12553
to review the following change.
Change subject: IMPALA-8239 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
IMPALA-8239 / 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
---
M be/src/kudu/security/init.cc
1 file changed, 12 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/12553/1
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Feb 2019 22:50:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8239 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8239 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Removed reviewer Kudu Jenkins.
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Feb 2019 18:40:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Feb 2019 07:23:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Feb 2019 17:56:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Removed reviewer Kudu Jenkins.
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3819/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Feb 2019 18:40:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
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>
---
M be/src/kudu/security/init.cc
1 file changed, 12 insertions(+), 0 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Alexey Serbin, Kudu Jenkins, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12553
to look at the new patch set (#2).
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
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
---
M be/src/kudu/security/init.cc
1 file changed, 12 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/12553/2
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5 parse name()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12553 )
Change subject: IMPALA-8212 / KUDU-2706: Work around the lack of thread safety in krb5_parse_name()
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2210/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12553
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12553
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Feb 2019 07:28:26 +0000
Gerrit-HasComments: No