android: crazy-linker: Try to fix WriteLinkMapField() crashes.

This is an attempt to fix the random crashes that happen in
the crazy::RDebug::WriteLinkMapField() function on some devices
(for more context, see related bug below).

It is very hard to understand why these crashes happen, the
best candidate hypothesis so far is that both the crazy linker
and the system linker try to modify the same area of memory
at the same time (the global _r_debug link-map list, see
technical note in crazy_linker_rdebug.h for all details).

This CL tries to reduce the occurence of such events by
better synchronizing modifications performed by the system
linker, and the crazy one.

This is done by the following steps:

- Create a new global mutex, and associated convenience class,
  used to synchronize modifications to the global _r_debug link
  map list. See Globals::ScopedLinkMapLocker.

  Before the CL, only *explicit* modifications to the list were
  synchronized through the global crazy lock, but this did not
  protect against other threads from native code that call the
  wrapped dlopen(), which end up calling the system ::dlopen()
  which may also modify the list.

  These explicit modifications would typically happen after
  the library was loaded, due to the fact that they are
  applied by a delayed callback running on the UI thread,
  not the thread that loaded the library.

  Ironically, this delayed callback scheme was introduced
  to try to reduce these kind of random crashes. We may
  consider removing it entirely if this patch doesn't
  fix the situation.

- To solve this, wrap all calls to the system linker through
  a new convenience class: crazy::SystemLinker. And ensure
  that each call that may change the global list will
  properly lock/unlock the link map mutex.

- Incidentally, this allows making the global crazy mutex
  non-recursive.

+ Initialize the RDebug instance properly when its
  GetAddress() method is called. Otherwise, it could return
  a nullptr value in certain cases.

  This was not an issue in practice, since it was only used
  to update the DT_DEBUG entry of a crazy-loaded library,
  which technically isn't used by anything at runtime
  (e.g. GDB would find the value from the executable
  image instead, not from one of the shared libraries
  already in the list).

BUG=831403
R=pasko@chromium.org, rmcilroy@chromium.org, agrieve@chromium.org, lizeb@chromium.org

Change-Id: I6aa5f622908fc56deb37825ffed4a816ec6e4cc2
Reviewed-on: https://chromium-review.googlesource.com/1059516
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558805}

third_party/android_crazy_linker/BUILD.gn

@@ -90,6 +90,8 @@ if (is_android) {
         "src/src/crazy_linker_shared_library.h",
         "src/src/crazy_linker_system.cpp",
         "src/src/crazy_linker_system.h",
+        "src/src/crazy_linker_system_linker.cpp",
+        "src/src/crazy_linker_system_linker.h",
         "src/src/crazy_linker_thread.cpp",
         "src/src/crazy_linker_thread.h",
         "src/src/crazy_linker_util.cpp",

third_party/android_crazy_linker/src/src/crazy_linker_globals.cpp

@@ -34,13 +34,6 @@ void InitGlobals() {
 }  // namespace
 
 Globals::Globals() {
-  // TODO(digit): Remove the need for a recursive mutex (which is often the
-  // symptom of something wrong in threaded code). This needs refactoring
-  // the deferred task management though.
-  pthread_mutexattr_t attr;
-  pthread_mutexattr_init(&attr);
-  pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
-  pthread_mutex_init(&lock_, &attr);
   search_paths_.ResetFromEnv("LD_LIBRARY_PATH");
 }
 
@@ -65,4 +58,7 @@ Globals* Globals::Get() {
 // static
 int Globals::sdk_build_version = 0;
 
+// static
+pthread_mutex_t ScopedLinkMapLocker::s_lock_ = PTHREAD_MUTEX_INITIALIZER;
+
 }  // namespace crazy

third_party/android_crazy_linker/src/src/crazy_linker_globals.h

@@ -55,7 +55,7 @@ class Globals {
   static RDebug* GetRDebug() { return Get()->rdebug(); }
 
  private:
-  pthread_mutex_t lock_;
+  pthread_mutex_t lock_ = PTHREAD_MUTEX_INITIALIZER;
   LibraryList libraries_;
   SearchPathList search_paths_;
   RDebug rdebug_;
@@ -84,6 +84,17 @@ class ScopedLockedGlobals {
   Globals* globals_;
 };
 
+// Convenience class used to operate on a mutex used to synchronize access to
+// the global _r_debug link map, at least from threads using the crazy linker.
+class ScopedLinkMapLocker {
+ public:
+  ScopedLinkMapLocker() { pthread_mutex_lock(&s_lock_); }
+  ~ScopedLinkMapLocker() { pthread_mutex_unlock(&s_lock_); }
+
+ private:
+  static pthread_mutex_t s_lock_;
+};
+
 }  // namespace crazy
 
 #endif  // CRAZY_LINKER_GLOBALS_H

third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp

@@ -6,7 +6,6 @@
 
 #include <assert.h>
 #include <crazy_linker.h>
-#include <dlfcn.h>
 
 #include "crazy_linker_debug.h"
 #include "crazy_linker_globals.h"
@@ -15,6 +14,7 @@
 #include "crazy_linker_rdebug.h"
 #include "crazy_linker_shared_library.h"
 #include "crazy_linker_system.h"
+#include "crazy_linker_system_linker.h"
 #include "crazy_linker_util.h"
 #include "crazy_linker_zip.h"
 
@@ -349,8 +349,7 @@ LibraryView* LibraryList::LoadLibrary(const char* lib_name,
   // crazily.
   if (is_dependency_or_preload) {
     LOG("Loading system library '%s'", lib_name);
-    ::dlerror();
-    void* system_lib = dlopen(lib_name, dlopen_mode);
+    void* system_lib = SystemLinker::Open(lib_name, dlopen_mode);
     if (!system_lib) {
       error->Format("Can't load system library %s: %s", lib_name, ::dlerror());
       return NULL;

third_party/android_crazy_linker/src/src/crazy_linker_library_view.cpp

@@ -4,10 +4,10 @@
 
 #include "crazy_linker_library_view.h"
 
-#include <dlfcn.h>
 #include "crazy_linker_debug.h"
 #include "crazy_linker_globals.h"
 #include "crazy_linker_shared_library.h"
+#include "crazy_linker_system_linker.h"
 
 namespace crazy {
 
@@ -17,7 +17,7 @@ LibraryView::LibraryView(SharedLibrary* crazy_lib)
 LibraryView::~LibraryView() {
   LOG("Destroying %s", name_.c_str());
   if (type_ == TYPE_SYSTEM) {
-    ::dlclose(system_);
+    SystemLinker::Close(system_);
     system_ = NULL;
   }
   if (type_ == TYPE_CRAZY) {
@@ -28,8 +28,9 @@ LibraryView::~LibraryView() {
 }
 
 void* LibraryView::LookupSymbol(const char* symbol_name) {
-  if (type_ == TYPE_SYSTEM)
-    return ::dlsym(system_, symbol_name);
+  if (type_ == TYPE_SYSTEM) {
+    return SystemLinker::Resolve(system_, symbol_name);
+  }
 
   if (type_ == TYPE_CRAZY) {
     LibraryList* lib_list = Globals::Get()->libraries();

third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp

@@ -196,6 +196,13 @@ void ScopedPageReadWriteRemapper::Release() {
 
 }  // namespace
 
+r_debug* RDebug::GetAddress() {
+  if (!init_) {
+    Init();
+  }
+  return r_debug_;
+}
+
 bool RDebug::Init() {
   // The address of '_r_debug' is in the DT_DEBUG entry of the current
   // executable.
@@ -410,7 +417,6 @@ void RDebug::WriteLinkMapField(link_map_t** link_pointer, link_map_t* entry) {
 }
 
 void RDebug::AddEntryImpl(link_map_t* entry) {
-  ScopedLockedGlobals globals;  // TODO(digit): Remove this lock.
   LOG("Adding: %s", entry->l_name);
   if (!init_)
     Init();
@@ -420,6 +426,9 @@ void RDebug::AddEntryImpl(link_map_t* entry) {
     return;
   }
 
+  // Ensure modifications to the global link map are synchronized.
+  ScopedLinkMapLocker locker;
+
   // IMPORTANT: GDB expects the first entry in the list to correspond
   // to the executable. So add our new entry just after it. This is ok
   // because by default, the linker is always the second entry, as in:
@@ -468,11 +477,14 @@ void RDebug::AddEntryImpl(link_map_t* entry) {
 }
 
 void RDebug::DelEntryImpl(link_map_t* entry) {
-  ScopedLockedGlobals globals;  // TODO(digit): Remove this lock.
-  LOG("Deleting: %s", entry->l_name);
   if (!r_debug_)
     return;
 
+  LOG("Deleting: %s", entry->l_name);
+
+  // Ensure modifications to the global link map are synchronized.
+  ScopedLinkMapLocker locker;
+
   // Tell GDB the list is going to be modified.
   CallRBrk(RT_DELETE);
 

third_party/android_crazy_linker/src/src/crazy_linker_rdebug.h

@@ -202,7 +202,9 @@ class RDebug {
     post_for_later_execution_context_ = context;
   }
 
-  r_debug* GetAddress() { return r_debug_; }
+  // Return address of current global _r_debug variable, or nullptr if not
+  // available.
+  r_debug* GetAddress();
 
  private:
   // Try to find the address of the global _r_debug variable, even

third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp

@@ -4,7 +4,6 @@
 
 #include "crazy_linker_shared_library.h"
 
-#include <dlfcn.h>
 #include <stdlib.h>
 #include <sys/mman.h>
 #include <elf.h>
@@ -13,10 +12,11 @@
 #include "crazy_linker_debug.h"
 #include "crazy_linker_elf_loader.h"
 #include "crazy_linker_elf_relocations.h"
+#include "crazy_linker_globals.h"
 #include "crazy_linker_library_list.h"
 #include "crazy_linker_library_view.h"
-#include "crazy_linker_globals.h"
 #include "crazy_linker_memory_mapping.h"
+#include "crazy_linker_system_linker.h"
 #include "crazy_linker_thread.h"
 #include "crazy_linker_util.h"
 #include "crazy_linker_wrappers.h"
@@ -96,8 +96,10 @@ class SharedLibraryResolver : public ElfRelocations::SymbolResolver {
                         LibraryList* lib_list,
                         Vector<LibraryView*>* preloads,
                         Vector<LibraryView*>* dependencies)
-      : main_program_handle_(::dlopen(NULL, RTLD_NOW)),
-        lib_(lib), preloads_(preloads), dependencies_(dependencies) {}
+      : main_program_handle_(SystemLinker::Open(NULL, RTLD_NOW)),
+        lib_(lib),
+        preloads_(preloads),
+        dependencies_(dependencies) {}
 
   virtual void* Lookup(const char* symbol_name) {
     // First, look inside the current library.
@@ -132,7 +134,7 @@ class SharedLibraryResolver : public ElfRelocations::SymbolResolver {
     }
 
     // Then lookup inside the main executable.
-    address = ::dlsym(main_program_handle_, symbol_name);
+    address = SystemLinker::Resolve(main_program_handle_, symbol_name);
     if (address)
       return address;
 
@@ -153,7 +155,7 @@ class SharedLibraryResolver : public ElfRelocations::SymbolResolver {
  private:
   virtual void* LookupInWrap(const char* symbol_name, LibraryView* wrap) {
     if (wrap->IsSystem()) {
-      void* address = ::dlsym(wrap->GetSystem(), symbol_name);
+      void* address = SystemLinker::Resolve(wrap->GetSystem(), symbol_name);
       // Android libm.so defines isnanf as weak. This means that its
       // address cannot be found by dlsym(), which returns NULL for weak
       // symbols prior to Android 5.0. However, libm.so contains the real
@@ -167,7 +169,7 @@ class SharedLibraryResolver : public ElfRelocations::SymbolResolver {
       // http://code.google.com/p/chromium/issues/detail?id=376828
       if (!address && !strcmp(symbol_name, "isnanf") &&
           !strcmp(wrap->GetName(), "libm.so")) {
-        address = ::dlsym(wrap->GetSystem(), "__isnanf");
+        address = SystemLinker::Resolve(wrap->GetSystem(), "__isnanf");
         if (!address) {
           // __isnanf only exists on Android 21+, so use a local fallback
           // if that doesn't exist either.

third_party/android_crazy_linker/src/src/crazy_linker_system_linker.cpp

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "crazy_linker_system_linker.h"
+
+#include "crazy_linker_globals.h"
+
+#include <dlfcn.h>
+
+namespace crazy {
+
+// static
+void* SystemLinker::Open(const char* path, int mode) {
+  // NOTE: The system linker will likely modify the global _r_debug link map
+  // so ensure this doesn't conflict with other threads performing delayed
+  // updates on it.
+  ScopedLinkMapLocker locker;
+  return ::dlopen(path, mode);
+}
+
+// static
+int SystemLinker::Close(void* handle) {
+  // Similarly, though unlikely, this operation may modify the global link map.
+  ScopedLinkMapLocker locker;
+  return ::dlclose(handle);
+}
+
+// static
+void* SystemLinker::Resolve(void* handle, const char* symbol) {
+  // Just in case the system linker performs lazy symbol resolution
+  // that would modify the global link map.
+  ScopedLinkMapLocker locker;
+  return ::dlsym(handle, symbol);
+}
+
+// static
+const char* SystemLinker::Error() {
+  return ::dlerror();
+}
+
+int SystemLinker::AddressInfo(void* address, Dl_info* info) {
+  ::dlerror();
+  return ::dladdr(address, info);
+}
+
+}  // namespace crazy

third_party/android_crazy_linker/src/src/crazy_linker_system_linker.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CRAZY_LINKER_SYSTEM_LINKER_H
+#define CRAZY_LINKER_SYSTEM_LINKER_H
+
+#include <dlfcn.h>
+
+namespace crazy {
+
+// Convenience wrapper for the system linker functions.
+// Also helps synchronize access to the global link map list.
+//
+// TODO(digit): Use this in the future to mock different versions/behaviours
+// of the Android system linker for unit-testing purposes.
+struct SystemLinker {
+  // Wrapper for dlopen().
+  static void* Open(const char* path, int flags);
+
+  // Wrapper for dlclose().
+  static int Close(void* handle);
+
+  // Wrapper for dlsym().
+  static void* Resolve(void* handle, const char* symbol);
+
+  // Wrapper for dlerror().
+  static const char* Error();
+
+  // Wrapper for dladdr();
+  static int AddressInfo(void* addr, Dl_info* info);
+};
+
+}  // namespace crazy
+
+#endif  // CRAZY_LINKER_SYSTEM_LINKER_H

third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp

@@ -4,7 +4,6 @@
 
 #include "crazy_linker_wrappers.h"
 
-#include <dlfcn.h>
 #include <link.h>
 
 #include "crazy_linker_debug.h"
@@ -12,6 +11,7 @@
 #include "crazy_linker_library_list.h"
 #include "crazy_linker_library_view.h"
 #include "crazy_linker_shared_library.h"
+#include "crazy_linker_system_linker.h"
 #include "crazy_linker_thread.h"
 #include "crazy_linker_util.h"
 
@@ -75,7 +75,7 @@ int __aeabi_atexit(void* object, void (*destructor)(void*), void* dso_handle) {
 // Used to save the system dlerror() into our thread-specific data.
 void SaveSystemError() {
   ThreadData* data = GetThreadData();
-  data->SetError(::dlerror());
+  data->SetError(SystemLinker::Error());
 }
 
 char* WrapDlerror() {
@@ -105,11 +105,10 @@ void* WrapDlopen(const char* path, int mode) {
   }
 
   // Try to load the executable with the system dlopen() instead.
-  ::dlerror();
-  void* system_lib = ::dlopen(path, mode);
+  void* system_lib = SystemLinker::Open(path, mode);
   if (system_lib == NULL) {
     SaveSystemError();
-    return NULL;
+    return nullptr;
   }
 
   auto* wrap_lib = new LibraryView(system_lib, path ? path : "<executable>");
@@ -147,7 +146,7 @@ void* WrapDlsym(void* lib_handle, const char* symbol_name) {
   if (!globals->valid_handles()->Has(lib_handle)) {
     // Note: the handle was not opened with the crazy linker, so fall back
     // to the system linker. That can happen in rare cases.
-    void* result = ::dlsym(lib_handle, symbol_name);
+    void* result = SystemLinker::Resolve(lib_handle, symbol_name);
     if (!result) {
       SaveSystemError();
       LOG("dlsym: could not find symbol '%s' from foreign library\n",
@@ -158,9 +157,7 @@ void* WrapDlsym(void* lib_handle, const char* symbol_name) {
 
   auto* wrap_lib = reinterpret_cast<LibraryView*>(lib_handle);
   if (wrap_lib->IsSystem()) {
-    // Note: the system dlsym() only looks into the target library,
-    // while the GNU linker performs a breadth-first search.
-    void* result = ::dlsym(wrap_lib->GetSystem(), symbol_name);
+    void* result = SystemLinker::Resolve(wrap_lib->GetSystem(), symbol_name);
     if (!result) {
       SaveSystemError();
       LOG("dlsym:%s: could not find symbol '%s' from system library\n%s",
@@ -208,8 +205,7 @@ int WrapDladdr(void* address, Dl_info* info) {
     }
   }
   // Otherwise, use system version.
-  ::dlerror();
-  int ret = ::dladdr(address, info);
+  int ret = SystemLinker::AddressInfo(address, info);
   if (ret != 0)
     SaveSystemError();
   return ret;
@@ -225,7 +221,7 @@ int WrapDlclose(void* lib_handle) {
   if (!globals->valid_handles()->Remove(lib_handle)) {
     // This is a foreign handle that was not created by the crazy linker.
     // Fall-back to the system in this case.
-    if (::dlclose(lib_handle) != 0) {
+    if (SystemLinker::Close(lib_handle) != 0) {
       SaveSystemError();
       LOG("dlclose: could not close foreign library handle %p\n%s", lib_handle,
           GetThreadData()->GetError());