kudu git commit: Work around another OpenSSL thread safety bug

[al...@apache.org]

Repository: kudu
Updated Branches:
  refs/heads/master 1b89ec813 -> d0270172e


Work around another OpenSSL thread safety bug

In the course of debugging some CHECK failures and TSAN errors, I found
that older versions of OpenSSL have non-threadsafe OBJ_create and even
ERR_peek_error methods. This commit fixes an instance where we were
calling OBJ_create concurrently by wrapping it in a std::call_once. I
don't have a fix for ERR_peek_err unsafety, since that's used
pervasively in most methods touching OpenSSL.

Side note: for debugging issues like this, I find it helpful to run ASAN
with the following options:

ASAN_OPTIONS="fast_unwind_on_malloc=0"

That option typically makes races more reproducible, and produces better
stack traces as well.

Change-Id: I9a9fe1a32f77bf24a5c7e692a55b8ad96488d409
Reviewed-on: http://gerrit.cloudera.org:8080/6997
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/d0270172
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/d0270172
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/d0270172

Branch: refs/heads/master
Commit: d0270172e91bf6bcb045cb63a731609472fa6be4
Parents: 1b89ec8
Author: Dan Burkert <da...@apache.org>
Authored: Thu May 25 16:24:17 2017 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Sat May 27 00:45:13 2017 +0000

----------------------------------------------------------------------
 src/kudu/security/cert-test.cc | 24 ++++++++++++++++++++++++
 src/kudu/security/cert.cc      | 13 +++++++------
 2 files changed, 31 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/d0270172/src/kudu/security/cert-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/cert-test.cc b/src/kudu/security/cert-test.cc
index 275a60b..acd0f74 100644
--- a/src/kudu/security/cert-test.cc
+++ b/src/kudu/security/cert-test.cc
@@ -15,7 +15,9 @@
 // specific language governing permissions and limitations
 // under the License.
 
+#include <thread>
 #include <utility>
+#include <vector>
 
 #include <boost/optional.hpp>
 #include <boost/optional/optional_io.hpp>
@@ -25,11 +27,14 @@
 #include "kudu/security/crypto.h"
 #include "kudu/security/openssl_util.h"
 #include "kudu/security/test/test_certs.h"
+#include "kudu/util/barrier.h"
 #include "kudu/util/status.h"
 #include "kudu/util/test_macros.h"
 #include "kudu/util/test_util.h"
 
 using std::pair;
+using std::thread;
+using std::vector;
 
 namespace kudu {
 namespace security {
@@ -60,6 +65,25 @@ class CertTest : public KuduTest {
   PrivateKey ca_exp_private_key_;
 };
 
+// Regression test to make sure that GetKuduKerberosPrincipalOidNid is thread
+// safe. OpenSSL 1.0.0's OBJ_create method is not thread safe.
+TEST_F(CertTest, GetKuduKerberosPrincipalOidNidConcurrent) {
+  int kConcurrency = 16;
+  Barrier barrier(kConcurrency);
+
+  vector<thread> threads;
+  for (int i = 0; i < kConcurrency; i++) {
+    threads.emplace_back([&] () {
+        barrier.Wait();
+        CHECK_NE(NID_undef, GetKuduKerberosPrincipalOidNid());
+    });
+  }
+
+  for (auto& thread : threads) {
+    thread.join();
+  }
+}
+
 // Check input/output of the X509 certificates in PEM format.
 TEST_F(CertTest, CertInputOutputPEM) {
   const Cert& cert = ca_cert_;

http://git-wip-us.apache.org/repos/asf/kudu/blob/d0270172/src/kudu/security/cert.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/cert.cc b/src/kudu/security/cert.cc
index cde4e47..fa4c753 100644
--- a/src/kudu/security/cert.cc
+++ b/src/kudu/security/cert.cc
@@ -17,6 +17,7 @@
 
 #include "kudu/security/cert.h"
 
+#include <mutex>
 #include <string>
 
 #include <openssl/evp.h>
@@ -57,12 +58,12 @@ string X509NameToString(X509_NAME* name) {
 
 int GetKuduKerberosPrincipalOidNid() {
   InitializeOpenSSL();
-  SCOPED_OPENSSL_NO_PENDING_ERRORS;
-
-  int nid = OBJ_txt2nid(kKuduKerberosPrincipalOidStr);
-  if (nid != NID_undef) return nid;
-  nid = OBJ_create(kKuduKerberosPrincipalOidStr, "kuduPrinc", "kuduKerberosPrincipal");
-  CHECK_NE(nid, NID_undef) << "could not create kuduPrinc oid";
+  static std::once_flag flag;
+  static int nid;
+  std::call_once(flag, [&] () {
+      nid = OBJ_create(kKuduKerberosPrincipalOidStr, "kuduPrinc", "kuduKerberosPrincipal");
+      CHECK_NE(nid, NID_undef) << "failed to create kuduPrinc oid: " << GetOpenSSLErrors();
+  });
   return nid;
 }