Sailesh Mukil has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: Line 37: #include "kudu/util/status.h" > warning: #includes are not sorted properly [llvm-include-order] Done 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 sk_X509_INFO_pop_free() takes 2 arguments but kFreeFunc is always passed one argument, so this will not work unless we create a wrapper function for sk_X509_INFO_pop_free(), which I think is going beyond what we need since we use it only in a single place. Let me know what you think. 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 It would if we didn't set the pointer to nullptr, but since we set it to nullptr, it will ignore it. Another way of doing this would be to use sk_X509_dup() above instead of sk_X509_push() to copy the X509 items over to the final stack, and not do the following line: stack_item->x509 = nullptr; But I thought this would be better for performance in the case of large chains. PS2, Line 193: if (sk_X509_push(sk, x.release()) == 0) { : return nullptr; : } : return sk; > I think it should be Ah yes, this seems the right way to do it. Done. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
