Alexey Serbin has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: PS1, Line 178: ASSERT_OK(DoTestSyncCall(p, Ge > I think it should be fine, but I copied the above TestCall() test. I guess Just was curious whether it was necessary to have it here, but that was not crucial and you removed it already, that's fine. http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc File src/kudu/security/cert.cc: Line 214: DCHECK(cert); > Unfortunately, X509_chain_up_ref() is only available from OpenSSL 1.1.0. Sh I think you can leave it as is -- I was just thinking that the code might be simplified. If not and it requires additional ifdefs, then it's better to keep it as is. http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() { : sk_X509_INFO_pop_free(info, X509_INFO_free); : }); nit: if you introduced corresponding trait functions for STACK_OK(X509_INFO), that could be handled via ssl_make_unique(). PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will : // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr. : // We will take the responsibility of freeing it when we are done with the STACK_OF(X509). : stack_item->x509 = nullptr; Would the scoped cleanup about try to free the data just put into the stack upon exiting from the scope? PS2, Line 193: if (sk_X509_push(sk, x.release()) == 0) { : return nullptr; : } : return sk; I think it should be if (sk_X509_push(sk, x.get()) == 0) { return nullptr; } x.release(); return sk; The idea here is to avoid memory leak if sk_X509_push() fails. I looked into the sk_X509_push() and it can fail if it cannot grow the stack to accommodate the new element or the passed stack pointer is nullptr. If so happens, the caller retains the ownership of the passed data/new element. -- To view, visit http://gerrit.cloudera.org:8080/7662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
