kudu git commit: KUDU-2427: relax dlsym call to dl_iterate_phdr

[ad...@apache.org]

Repository: kudu
Updated Branches:
  refs/heads/master 8489fa816 -> b621f9c1a


KUDU-2427: relax dlsym call to dl_iterate_phdr

When gcc passes --as-needed to ld by default, dynamically linked test
binaries that don't directly reference symbols in kudu_util may end up
loading libc before loading kudu_util. This causes dlsym(RTLD_NEXT, ...) on
libc symbols to fail because libc falls earlier in the symbol search order.

At first I thought this was a new problem in Ubuntu 18.04, but I was able to
reproduce it in Ubuntu 16.04 too; the only difference is that in Ubuntu
16.04 the failure isn't reported via dlerror() so we never noticed it. Given
that, I figured it'd be okay to make it non-fatal across the board.

Change-Id: I1795847740b1201c46331b6a64a96631ecbcea36
Reviewed-on: http://gerrit.cloudera.org:8080/10434
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>


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

Branch: refs/heads/master
Commit: b621f9c1a3949dc31ca4836b0767b2840fa73f29
Parents: 8489fa8
Author: Adar Dembo <ad...@cloudera.com>
Authored: Sun May 13 20:40:36 2018 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Thu May 17 18:08:13 2018 +0000

----------------------------------------------------------------------
 src/kudu/util/debug/unwind_safeness.cc | 39 ++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/b621f9c1/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 4e233ef..c8e0adf 100644
--- a/src/kudu/util/debug/unwind_safeness.cc
+++ b/src/kudu/util/debug/unwind_safeness.cc
@@ -35,6 +35,13 @@
 #define CALL_ORIG(func_name, ...) \
   ((decltype(&func_name))g_orig_ ## func_name)(__VA_ARGS__)
 
+// Don't hook dl_iterate_phdr in TSAN builds since TSAN already instruments this
+// function and blocks signals while calling it. And skip it for macOS; it
+// doesn't exist there.
+#if !defined(THREAD_SANITIZER) && !defined(__APPLE__)
+#define HOOK_DL_ITERATE_PHDR 1
+#endif
+
 typedef int (*dl_iterate_phdr_cbtype)(struct dl_phdr_info *, size_t, void *);
 
 namespace {
@@ -45,7 +52,7 @@ bool g_initted;
 // The original versions of our wrapped functions.
 void* g_orig_dlopen;
 void* g_orig_dlclose;
-#ifndef __APPLE__
+#ifdef HOOK_DL_ITERATE_PHDR
 void* g_orig_dl_iterate_phdr;
 #endif
 
@@ -92,8 +99,30 @@ void InitIfNecessary() {
 
   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");
+#ifdef HOOK_DL_ITERATE_PHDR
+  // Failing to hook dl_iterate_phdr is non-fatal.
+  //
+  // In toolchains where the linker is passed --as-needed by default, a
+  // dynamically linked binary that doesn't directly reference any kudu_util
+  // symbols will omit a DT_NEEDED entry for kudu_util. Such a binary will
+  // no doubt emit a DT_NEEDED entry for libc, which means libc will wind up
+  // _before_ kudu_util in dlsym's search order. The net effect: the dlsym()
+  // call below will fail.
+  //
+  // All Ubuntu releases since Natty[1] behave in this way, except that many
+  // of them are also vulnerable to a glibc bug[2] that'll cause such a
+  // failure to go unreported by dlerror(). In newer releases, the failure
+  // is reported and dlsym_or_die() crashes the process.
+  //
+  // Given that the subset of affected binaries is small, and given that
+  // dynamic linkage isn't used in production anyway, we'll just treat the
+  // hook attempt as a best effort. Affected binaries that actually attempt
+  // to invoke dl_iterate_phdr will dereference a null pointer and crash, so
+  // if this is ever becomes a problem, we'll know right away.a
+  //
+  // 1. https://wiki.ubuntu.com/NattyNarwhal/ToolchainTransition
+  // 2. https://sourceware.org/bugzilla/show_bug.cgi?id=19509
+  g_orig_dl_iterate_phdr = dlsym(RTLD_NEXT, "dl_iterate_phdr");
 #endif
   g_initted = true;
 }
@@ -124,9 +153,7 @@ int dlclose(void *handle) { // NOLINT
   return CALL_ORIG(dlclose, handle);
 }
 
-// Don't hook dl_iterate_phdr in TSAN builds since TSAN already instruments this
-// function and blocks signals while calling it.
-#if !defined(THREAD_SANITIZER) && !defined(__APPLE__)
+#ifdef HOOK_DL_ITERATE_PHDR
 int dl_iterate_phdr(dl_iterate_phdr_cbtype callback, void *data) { // NOLINT
   InitIfNecessary();
   ScopedBumpDepth d;