Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15394 )
Change subject: KUDU-3050: recover from corrupt kerberos ccache ...................................................................... Patch Set 4: (5 comments) > > Patch Set 4: > > > > Might be https://krbdev.mit.edu/rt/Ticket/Display.html?id=8437 or > similar. I don't see any evidence that Kudu is misusing the APIs. > It looks like the memory is allocated in either krb5_cc_resolve() > or krb5_cc_dup(), by tracing back from fcc_resolve. > > Try running the test with ASAN_OPTIONS=fast_unwind_on_malloc=0 in > the environment. That should give you a more complete stack trace. Should we call krb5_cc_end_seq_get() even if prior call to krb5_cc_start_seq_get() returned an error? Maybe, that's the reason? I.e., maybe add SCOPED_CLEANUP({krb5_cc_end_seq_get(g_krb5_ctx, ccache_, &cursor); }); before call to krb5_cc_start_seq_get(). http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@403 PS4, Line 403: That nit: Test that ? http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@420 PS4, Line 420: LOG(INFO) << "Truncating ccache at '" << cc_path << "' to " << trunc_len; nit: since the test is already working as expected, to reduce chatter, consider using 'SCOPED_TRACE(message)' instead of 'LOG(INFO) << message' to output the message only when any assertion below fails. http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@421 PS4, Line 421: RWFileOptions opts; : opts.mode = Env::MUST_EXIST; : unique_ptr<RWFile> cc_file; : ASSERT_OK(env_->NewRWFile(opts, cc_path, &cc_file)); : ASSERT_OK(cc_file->Truncate(trunc_len)); : cc_file->Close(); nit: move these lines into a sub-scope and remove the cc_file->Close()? http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/integration-tests/security-itest.cc@431 PS4, Line 431: LOG(INFO) << s.ToString(); nit: our tests are super-chatty; maybe it makes sense to drop this once the test is worked as expected given there is ASSERT_STR_CONTAINS() next line? http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/15394/4/src/kudu/security/init.cc@244 PS4, Line 244: LOG(WARNING) << "Error " << code << " while opening credential cache: " > I suppose I should also switch this to Substitute() +1 -- To view, visit http://gerrit.cloudera.org:8080/15394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d6e06c3ea65708896a6bf0134cc84838b3f1b58 Gerrit-Change-Number: 15394 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 11 Mar 2020 00:25:52 +0000 Gerrit-HasComments: Yes
