Todd Lipcon has posted comments on this change. Change subject: KUDU-1964. security: avoid calling ERR_clear_error() defensively ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/6552/1/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS1, Line 79: if (!ctx) { : ERR_clear_error(); : return Status::RuntimeError("SSL library appears uninitialized (cannot create SSL_CTX)"); : } > Consider using OPENSSL_RET_IF_NULL() macro instead: it's more idiomatic and I don't think we want to spit back the internal error here (it's pretty ugly) since this gets propagated up to the user library. PS1, Line 110: ERR_clear_error(); > Why is it necessary here? As I understand, DoInitializeOpenSSL() is the on we actually expect the above SSL_CTX_new() call to cause an error (we expect that SSL is _not_ initialized yet at this point). So we want to clear the error that we got. I'll move it into an 'else' http://gerrit.cloudera.org:8080/#/c/6552/1/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: Line 41: CHECK_GT((call), 0) > Perhaps we should assert that the error stack is empty here, before making Done PS1, Line 174: ERR_peek_last_error > Why ERR_peek_last_error() instead of ERR_peek_error()? Please add a commen Done http://gerrit.cloudera.org:8080/#/c/6552/1/src/kudu/security/tls_handshake.cc File src/kudu/security/tls_handshake.cc: PS1, Line 96: ERR_clear_error(); > Consider removing this, otherwise the code below would not be able to extra wouldn't your code end up generating warnings on every WANT_READ/WANT_WRITE return, which are normal? I see your point though that GetSSLErrorDescription itself may need to look at the error queue and the clear_error breaks that. I checked the SSL code and it seems that SSL_get_error() itself guarantees to return either SSL_ERROR_SYSCALL or SSL_ERROR_SSL in the case that the error queue is non-empty, so there's no need to clear it here. PS1, Line 132: ERR_clear_error(); : return Status::NotAuthorized("SSL_get_verify_result()", X509_verify_cert_error_string(rc)); > Consider replacing with: k, did something like that (kept a prefix on the error string) http://gerrit.cloudera.org:8080/#/c/6552/1/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: PS1, Line 57: ERR_clear_error(); > I don't think it's needed. Even if it were needed, it would be better plac yea, agreed it's not needed since GetSSLErrorDescription clears in the case that there could have been some error -- To view, visit http://gerrit.cloudera.org:8080/6552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b4421f4aae4d0e5a2d938881f9eea4e07ff2b10 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
