Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16659 )
Change subject: Add lock before verifying signature ...................................................................... Patch Set 3: (5 comments) It would be nice to understand where those races are actually happening. I guess rebuilding OpenSSL with TSAN and running those tests could help to pin point those. http://gerrit.cloudera.org:8080/#/c/16659/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16659/3//COMMIT_MSG@9 PS3, Line 9: It seems there is a race condition How did the race condition manifest itself? http://gerrit.cloudera.org:8080/#/c/16659/3//COMMIT_MSG@9 PS3, Line 9: somewhere Could you be more specific: maybe, you did get an insight of a function of method where the race happened? Was that something related to non-thread-safe allocation of memory or that was about accessing some internal state in the library? BTW, it's possible to build OpenSSL with TSAN support and get more details on those races. http://gerrit.cloudera.org:8080/#/c/16659/3//COMMIT_MSG@10 PS3, Line 10: as a : certificate can get corrupted How did this looked like? http://gerrit.cloudera.org:8080/#/c/16659/3/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/16659/3/src/kudu/security/tls_context.cc@470 PS3, Line 470: #if OPENSSL_VERSION_NUMBER < 0x10100000L : unique_lock<std::mutex> lock_global(mutex); : #endif : unique_lock<RWMutex> lock(lock_); I'm not sure it makes much sense to take a global lock and the per-instance lock as well. http://gerrit.cloudera.org:8080/#/c/16659/3/src/kudu/security/tls_handshake.cc File src/kudu/security/tls_handshake.cc: http://gerrit.cloudera.org:8080/#/c/16659/3/src/kudu/security/tls_handshake.cc@101 PS3, Line 101: #if OPENSSL_VERSION_NUMBER < 0x10100000L : std::unique_lock<std::mutex> lock(mutex); : #endif What if SSL/TLS handshake take a very long time due to some network issues? Does this mean _all_ other connection negotiations for entire kudu-master/kudu-tserver or Kudu application are frozen? -- 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: 3 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, 27 Oct 2020 18:11:29 +0000 Gerrit-HasComments: Yes
