Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16659 )
Change subject: Add lock before verifying signature ...................................................................... Patch Set 5: (5 comments) > 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. Added some additional context, but unfortunately, I don't have access to the source code of CryptoComply for Servers, so I don't think I can dig deeper. 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: somewhere > Could you be more specific: maybe, you did get an insight of a function of Done 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? Done 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? Done 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 First I had #else here too, but TSAN didn't like that: http://jenkins.kudu.apache.org/job/kudu-gerrit/22645/BUILD_TYPE=TSAN/testReport/junit/(root)/RemoteKsckTest/TestFilterOnNoTableCluster/ 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: DCHECK(n == recv.size() || (n == -1 && recv.empty())); : DCHECK_EQ(BIO_ctrl_pending(rbio), recv.size()); : > What if SSL/TLS handshake take a very long time due to some network issues? I don't think so, this method uses strings as input and output, and doesn't seem to do anything with the network. By the time the lock is acquired the data should be fully received, and the lock is released before the response is sent back. -- 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: 5 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: Wed, 28 Oct 2020 14:03:07 +0000 Gerrit-HasComments: Yes
