kudu git commit: KUDU-2328. Fix crash at startup with OpenSSL FIPS mode

[to...@apache.org]

Repository: kudu
Updated Branches:
  refs/heads/master 3b2df01c0 -> e9448dac5


KUDU-2328. Fix crash at startup with OpenSSL FIPS mode

The wrapping of libdl functions added by commit
d9e7037138646e3efb331af98c6982de13294c4b has a problem if any other
dynamic initializer calls dlopen or dlclose. It turns out that OpenSSL
in FIPS mode does indeed do that, leading to a crash with a stack like:

  #0  0x0000000000000000 in ?? ()
  #1  0x0000000001b45d23 in dlopen ()
  #2  0x00007f1f444967ba in ?? () from /lib64/libcrypto.so.1.0.0
  #3  0x00007f1f44496857 in ?? () from /lib64/libcrypto.so.1.0.0
  #4  0x00007f1f44496bfe in FIPS_module_mode_set () from /lib64/libcrypto.so.1.0.0
  #5  0x00007f1f4437216c in FIPS_mode_set () from /lib64/libcrypto.so.1.0.0
  #6  0x00007f1f4436eb60 in OPENSSL_init_library () from /lib64/libcrypto.so.1.0.0
  #7  0x00007f1f450a2c0a in call_init.part () from /lib64/ld-linux-x86-64.so.2
  #8  0x00007f1f450a2cf3 in _dl_init () from /lib64/ld-linux-x86-64.so.2
  #9  0x00007f1f4509518a in _dl_start_user () from /lib64/ld-linux-x86-64.so.2

The fix takes the same approach we already used to workaround a similar issue
with the ASAN runtime, but generalizes it to all of our wrapped functions.

Change-Id: I10a04126411f51b4d8e290a6b061aa585aad0769
Reviewed-on: http://gerrit.cloudera.org:8080/9460
Tested-by: Todd Lipcon <to...@apache.org>
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/e9448dac
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e9448dac
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e9448dac

Branch: refs/heads/master
Commit: e9448dac5d4d70a4c6e05c3d8e98c4c5d8c3fc14
Parents: 3b2df01
Author: Todd Lipcon <to...@apache.org>
Authored: Tue Feb 27 23:41:17 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Wed Feb 28 09:08:55 2018 +0000

----------------------------------------------------------------------
 src/kudu/util/debug/unwind_safeness.cc | 38 ++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 11 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/e9448dac/src/kudu/util/debug/unwind_safeness.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/debug/unwind_safeness.cc b/src/kudu/util/debug/unwind_safeness.cc
index 3339189..4e233ef 100644
--- a/src/kudu/util/debug/unwind_safeness.cc
+++ b/src/kudu/util/debug/unwind_safeness.cc
@@ -32,8 +32,6 @@
 
 #include <glog/logging.h>
 
-#include "kudu/gutil/port.h"
-
 #define CALL_ORIG(func_name, ...) \
   ((decltype(&func_name))g_orig_ ## func_name)(__VA_ARGS__)
 
@@ -41,6 +39,10 @@ typedef int (*dl_iterate_phdr_cbtype)(struct dl_phdr_info *, size_t, void *);
 
 namespace {
 
+// Whether InitializeIfNecessary() has been called.
+bool g_initted;
+
+// The original versions of our wrapped functions.
 void* g_orig_dlopen;
 void* g_orig_dlclose;
 #ifndef __APPLE__
@@ -68,13 +70,32 @@ void *dlsym_or_die(const char *sym) {
   return ret;
 }
 
+// Initialize the global variables which store the original references. This is
+// set up as a constructor so that we're guaranteed to call this before main()
+// while we are still single-threaded.
+//
+// NOTE: We _also_ call explicitly this from each of the wrappers, because
+// there are some cases where the constructors of dynamic libraries may call
+// dlopen, and there is no guarantee that our own constructor runs before
+// the constructor of other libraries.
+//
+// A couple examples of the above:
+//
+// 1) In ASAN builds, the sanitizer runtime ends up calling dl_iterate_phdr from its
+//    initialization.
+// 2) OpenSSL in FIPS mode calls dlopen() within its constructor.
 __attribute__((constructor))
-void Init(void) {
+void InitIfNecessary() {
+  // Dynamic library initialization is always single-threaded, so there's no
+  // need for any synchronization here.
+  if (g_initted) return;
+
   g_orig_dlopen = dlsym_or_die("dlopen");
   g_orig_dlclose = dlsym_or_die("dlclose");
 #ifndef __APPLE__ // This function doesn't exist on macOS.
   g_orig_dl_iterate_phdr = dlsym_or_die("dl_iterate_phdr");
 #endif
+  g_initted = true;
 }
 
 } // anonymous namespace
@@ -92,11 +113,13 @@ bool SafeToUnwindStack() {
 extern "C" {
 
 void *dlopen(const char *filename, int flag) { // NOLINT
+  InitIfNecessary();
   ScopedBumpDepth d;
   return CALL_ORIG(dlopen, filename, flag);
 }
 
 int dlclose(void *handle) { // NOLINT
+  InitIfNecessary();
   ScopedBumpDepth d;
   return CALL_ORIG(dlclose, handle);
 }
@@ -105,14 +128,7 @@ int dlclose(void *handle) { // NOLINT
 // function and blocks signals while calling it.
 #if !defined(THREAD_SANITIZER) && !defined(__APPLE__)
 int dl_iterate_phdr(dl_iterate_phdr_cbtype callback, void *data) { // NOLINT
-  // In ASAN builds, the sanitizer runtime ends up calling dl_iterate_phdr from its
-  // initialization, which runs before our own constructor functions. In that case, we
-  // need to call Init() from here rather than via the normal path. There's no need to
-  // worry about thread-safety since this all happens single-threaded during startup.
-  if (PREDICT_FALSE(!g_orig_dl_iterate_phdr)) {
-    Init();
-  }
-
+  InitIfNecessary();
   ScopedBumpDepth d;
   return CALL_ORIG(dl_iterate_phdr, callback, data);
 }