Todd Lipcon has posted comments on this change.
Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation
......................................................................
Patch Set 5:
(12 comments)
Seems like there is some kind of openssl-related leak as well:
Direct leak of 4848 byte(s) in 6 object(s) allocated from:
#0 0x53ca06 in __interceptor_malloc
/home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
#1 0x7f45da9c5d72 in CRYPTO_malloc
(/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fd72)
Indirect leak of 38284 byte(s) in 578 object(s) allocated from:
#0 0x53ca06 in __interceptor_malloc
/home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
#1 0x7f45da9c5d72 in CRYPTO_malloc
(/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fd72)
Indirect leak of 4608 byte(s) in 12 object(s) allocated from:
#0 0x53cdbd in realloc
/home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:77
#1 0x7f45da9c5e28 in CRYPTO_realloc
(/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fe28)
SUMMARY: AddressSanitizer: 47740 byte(s) leaked in 596 allocation(s).
unfortunately the stack trace is truncated because openssl doesn't have frame
pointers/debuginfo I guess?
http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
PS5, Line 56: sasl_client
rename arg (also below)
PS5, Line 175: InvalidArgument
InvalidArgument doesn't seem quite right, since really it's an error with the
other side, not an argument set by the caller
Line 188: RequestHeader header;
can you add a TRACE here?
Line 202: RETURN_NOT_OK(helper_.CheckSaslCallId(header.call_id()));
and maybe a TRACE here that the negotiate response was received?
Line 227: return socket()->BlockingWrite(buf, buflen, &nsent, deadline_);
does this guarantee it sent the whole buffer? it seems not, given it has the
&nsent parameter. Maybe need something that loops to try to send all?
Line 237: RETURN_NOT_OK(WrapSaslCall(nullptr /* no conn */, [&]() {
I think the old code which saved the status and converted to RuntimeError was
on purpose, to make sure the error type was correct? At least we should keep
prepending 'unable to create new SASL client' so it's clear where the error
came from (SASL error messages are typically inscrutable on their own so the
extra context is helpful even if verbose)
PS5, Line 267: InvalidArgument
same as comment elsewhere
Line 277: if (feature_flag != UNKNOWN) {
I think the intention of having the separate kSupportedClientFeatureFlags here
was that it allows for us to keep a flag in the .proto even if it stops being
supported by either client or server in a future version. i.e we decouple the
set of supported client flags from supported server flags.
http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/client_negotiation.h
File src/kudu/rpc/client_negotiation.h:
Line 47: // Does not take ownership of the socket indicated by the fd.
this line doesn't seem right anymore
Line 51: // Must be called after Init().
I think Init() is gone now?
Line 85: // Must be called before Init(). Required for some mechanisms.
same
http://gerrit.cloudera.org:8080/#/c/5760/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
PS5, Line 238: kSaslAppName
we should check with Impala folks whether this will cause a problem. I'm not
100% sure what the "app name" ends up used for, but maybe they'll need a
programattic way to set this to Impala rather than having to modify the code?
though maybe it doesnt matter
--
To view, visit http://gerrit.cloudera.org:8080/5760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes
