Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16659 )
Change subject: KUDU-3210 Add lock before verifying signature ...................................................................... Patch Set 13: Code-Review+1 (4 comments) Overall looks good to me, just a few nits. http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.h@202 PS13, Line 202: OpensslMutex naming nit: it seems we predominantly use OpenSSL in other code around (e.g., InitializeOpenSSL(), GetOpenSSLErrors(), etc.), so I guess OpenSSLMutex might be a closer alternative to match the rest of the code? http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.cc@52 PS13, Line 52: in certain OpenSSL versions nit: does it make sense to be more specific on the versions affected by the extra locking? Or the idea is to be more flexible here and expand the usage of the newly introduced locks to OpenSSL 1.1.x and beyond as well? http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/openssl_util.cc@372 PS13, Line 372: FLAGS_openssl_defensive_locking Since the --openssl_defensive_locking is marked as a run-time flag, there is a chance that the flag's value might be flipped in between, so it could lead to non-unlocked mutex or an attempt to unlock a mutex which hasn't been locked. I guess it might be safer to capture the value of the --openssl_defensive_locking flag in the constructor or OpensslMutex, and then use that captured value in this and other similar 'if' clauses for this class. http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/tls_handshake.cc File src/kudu/security/tls_handshake.cc: http://gerrit.cloudera.org:8080/#/c/16659/13/src/kudu/security/tls_handshake.cc@50 PS13, Line 50: namespace nit for here an other comments for closing braces for an anonymous namespace: in Kudu code it's usually commented as 'anonymous namespace' -- To view, visit http://gerrit.cloudera.org:8080/16659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifafc7dcf91db910123276b657515e410bb7f6fcd Gerrit-Change-Number: 16659 Gerrit-PatchSet: 13 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 03 Nov 2020 18:19:02 +0000 Gerrit-HasComments: Yes
