android crazy linker: better Globals implementation.

A small cleanup and/or improvement to the way global
data is managed in the crazy linker library:

- Replace ScopedGlobalLock with ScopedLockedGlobals
  which allows reducing the number of Globals::Get()
  call performed at runtime.

- Provide property accessors to the globals that
  don't call Globals::Get() on each call, e.g.

    the static Globals::GetLibraries() is replaced
    by the non-static Globals::libraries() call.

- In crazy_linker_api.cpp, Fix a minor bug where
  the ScopedDelayedCallbackPoster is constructed
  before the global lock is acquired (i.e. a
  thread-race).

  Note that there are still other thread-related
  issues with deferred tasks / callback handling
  but these will be addressed in a future CL.

- Don't use heap allocation for the Globals
  instance storage. This is mostly because there
  were suspicion of invalid pointer addresses
  being passed through callbacks, causing runtime
  crashes (see http://crbug.com/796938 for more
  details). A fixed address in the .bss section
  would be easier to spot in minidump CPU register
  dumps.

BUG=801548
R=agrieve@chromium.org,pasko@chromium.org,rmcilroy@chromium.org

Change-Id: I68cba5360c5b5a3aaa7847172a6e73a87a64d44b
Reviewed-on: https://chromium-review.googlesource.com/893259Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532889}

third_party/android_crazy_linker/src/src/crazy_linker_api.cpp

@@ -21,8 +21,9 @@
 
 using crazy::Globals;
 using crazy::Error;
+using crazy::RDebug;
 using crazy::SearchPathList;
-using crazy::ScopedGlobalLock;
+using crazy::ScopedLockedGlobals;
 using crazy::LibraryView;
 
 //
@@ -66,7 +67,9 @@ void crazy_context_t::ResetSearchPaths() {
 extern "C" {
 
 void crazy_set_sdk_build_version(int sdk_build_version) {
-  *Globals::GetSDKBuildVersion() = sdk_build_version;
+  // NOTE: This must be called before creating the Globals instance,
+  // so do not use Globals::Get() or a ScopedLockedGlobals instance here.
+  Globals::sdk_build_version = sdk_build_version;
 }
 
 crazy_context_t* crazy_context_create() {
@@ -167,19 +170,16 @@ void crazy_context_destroy(crazy_context_t* context) { delete context; }
 // if callback is NULL.
 class ScopedDelayedCallbackPoster {
  public:
-  ScopedDelayedCallbackPoster(crazy_context_t* context) {
+  ScopedDelayedCallbackPoster(crazy_context_t* context, RDebug* rdebug) {
     if (context && context->callback_poster) {
-      crazy::Globals::GetRDebug()->SetDelayedCallbackPoster(&PostFromContext,
-                                                            context);
-      set_delayed_callback_poster_ = true;
-    } else {
-      set_delayed_callback_poster_ = false;
+      rdebug->SetDelayedCallbackPoster(&PostFromContext, context);
+      rdebug_ = rdebug;
     }
   }
 
   ~ScopedDelayedCallbackPoster() {
-    if (set_delayed_callback_poster_)
-      crazy::Globals::GetRDebug()->SetDelayedCallbackPoster(NULL, NULL);
+    if (rdebug_)
+      rdebug_->SetDelayedCallbackPoster(nullptr, nullptr);
   }
 
  private:
@@ -187,32 +187,26 @@ class ScopedDelayedCallbackPoster {
   static bool PostFromContext(void* crazy_context,
                               crazy_callback_handler_t handler,
                               void* opaque) {
-    crazy_context_t* context = static_cast<crazy_context_t*>(crazy_context);
+    auto* context = static_cast<crazy_context_t*>(crazy_context);
     crazy_callback_t callback;
     callback.handler = handler;
     callback.opaque = opaque;
-    return context->callback_poster(&callback,
-                                    context->callback_poster_opaque);
+    return context->callback_poster(&callback, context->callback_poster_opaque);
   }
 
-  // True if the context offered a callback_poster, otherwise false.
-  bool set_delayed_callback_poster_;
+  // Non-null iff the context offered a callback_poster.
+  RDebug* rdebug_ = nullptr;
 };
 
 crazy_status_t crazy_library_open(crazy_library_t** library,
                                   const char* lib_name,
                                   crazy_context_t* context) {
-  ScopedDelayedCallbackPoster poster(context);
-  ScopedGlobalLock lock;
-
-  LibraryView* wrap =
-      crazy::Globals::GetLibraries()->LoadLibrary(lib_name,
-                                                  RTLD_NOW,
-                                                  context->load_address,
-                                                  context->file_offset,
-                                                  &context->search_paths,
-                                                  false,
-                                                  &context->error);
+  ScopedLockedGlobals globals;
+  ScopedDelayedCallbackPoster poster(context, globals->rdebug());
+
+  LibraryView* wrap = globals->libraries()->LoadLibrary(
+      lib_name, RTLD_NOW, context->load_address, context->file_offset,
+      &context->search_paths, false, &context->error);
 
   if (!wrap)
     return CRAZY_STATUS_FAILURE;
@@ -221,7 +215,7 @@ crazy_status_t crazy_library_open(crazy_library_t** library,
     crazy::SharedLibrary* lib = wrap->GetCrazy();
     if (!lib->SetJavaVM(
              context->java_vm, context->minimum_jni_version, &context->error)) {
-      crazy::Globals::GetLibraries()->UnloadLibrary(wrap);
+      globals->libraries()->UnloadLibrary(wrap);
       return CRAZY_STATUS_FAILURE;
     }
   }
@@ -234,18 +228,12 @@ crazy_status_t crazy_library_open_in_zip_file(crazy_library_t** library,
                                               const char* zipfile_name,
                                               const char* lib_name,
                                               crazy_context_t* context) {
-  ScopedDelayedCallbackPoster poster(context);
-  ScopedGlobalLock lock;
-
-  LibraryView* wrap =
-      crazy::Globals::GetLibraries()->LoadLibraryInZipFile(
-          zipfile_name,
-          lib_name,
-          RTLD_NOW,
-          context->load_address,
-          &context->search_paths,
-          false,
-          &context->error);
+  ScopedLockedGlobals globals;
+  ScopedDelayedCallbackPoster poster(context, globals->rdebug());
+
+  LibraryView* wrap = globals->libraries()->LoadLibraryInZipFile(
+      zipfile_name, lib_name, RTLD_NOW, context->load_address,
+      &context->search_paths, false, &context->error);
 
   if (!wrap)
     return CRAZY_STATUS_FAILURE;
@@ -254,7 +242,7 @@ crazy_status_t crazy_library_open_in_zip_file(crazy_library_t** library,
     crazy::SharedLibrary* lib = wrap->GetCrazy();
     if (!lib->SetJavaVM(
              context->java_vm, context->minimum_jni_version, &context->error)) {
-      crazy::Globals::GetLibraries()->UnloadLibrary(wrap);
+      globals->libraries()->UnloadLibrary(wrap);
       return CRAZY_STATUS_FAILURE;
     }
   }
@@ -326,9 +314,8 @@ crazy_status_t crazy_library_use_shared_relro(crazy_library_t* library,
 crazy_status_t crazy_library_find_by_name(const char* library_name,
                                           crazy_library_t** library) {
   {
-    ScopedGlobalLock lock;
-    LibraryView* wrap =
-        Globals::GetLibraries()->FindLibraryByName(library_name);
+    ScopedLockedGlobals globals;
+    LibraryView* wrap = globals->libraries()->FindLibraryByName(library_name);
     if (!wrap)
       return CRAZY_STATUS_FAILURE;
 
@@ -358,8 +345,8 @@ crazy_status_t crazy_linker_find_symbol(const char* symbol_name,
 crazy_status_t crazy_library_find_from_address(void* address,
                                                crazy_library_t** library) {
   {
-    ScopedGlobalLock lock;
-    LibraryView* wrap = Globals::GetLibraries()->FindLibraryForAddress(address);
+    ScopedLockedGlobals globals;
+    LibraryView* wrap = globals->libraries()->FindLibraryForAddress(address);
     if (!wrap)
       return CRAZY_STATUS_FAILURE;
 
@@ -377,11 +364,11 @@ void crazy_library_close(crazy_library_t* library) {
 void crazy_library_close_with_context(crazy_library_t* library,
                                       crazy_context_t* context) {
   if (library) {
-    ScopedDelayedCallbackPoster poster(context);
-    ScopedGlobalLock lock;
+    ScopedLockedGlobals globals;
+    ScopedDelayedCallbackPoster poster(context, globals->rdebug());
     LibraryView* wrap = reinterpret_cast<LibraryView*>(library);
 
-    Globals::GetLibraries()->UnloadLibrary(wrap);
+    globals->libraries()->UnloadLibrary(wrap);
   }
 }
 

third_party/android_crazy_linker/src/src/crazy_linker_globals.cpp

@@ -4,22 +4,39 @@
 
 #include "crazy_linker_globals.h"
 
-#include <pthread.h>
+#include <new>
 
-#include "crazy_linker_system.h"
+#include <pthread.h>
 
 namespace crazy {
 
 namespace {
 
-Globals* g_globals = NULL;
-pthread_once_t g_globals_once = PTHREAD_ONCE_INIT;
+// Implement lazy-initialized static variable without a C++ constructor.
+// Note that this is leaky, i.e. the instance is never destroyed, but
+// this was also the case with the previous heap-based implementation.
+pthread_once_t s_once = PTHREAD_ONCE_INIT;
+
+union Storage {
+  char dummy;
+  Globals globals;
 
-void CreateGlobalsInstance() { g_globals = new Globals(); }
+  Storage() {}
+  ~Storage() {}
+};
+
+Storage s_storage;
+
+void InitGlobals() {
+  new (&s_storage.globals) Globals();
+}
 
 }  // namespace
 
-Globals::Globals() : search_paths_(), rdebug_() {
+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);
@@ -27,14 +44,25 @@ Globals::Globals() : search_paths_(), rdebug_() {
   search_paths_.ResetFromEnv("LD_LIBRARY_PATH");
 }
 
-Globals::~Globals() { pthread_mutex_destroy(&lock_); }
+Globals::~Globals() {
+  pthread_mutex_destroy(&lock_);
+}
+
+void Globals::Lock() {
+  pthread_mutex_lock(&lock_);
+}
 
+void Globals::Unlock() {
+  pthread_mutex_unlock(&lock_);
+}
+
+// static
 Globals* Globals::Get() {
-  pthread_once(&g_globals_once, CreateGlobalsInstance);
-  return g_globals;
+  pthread_once(&s_once, InitGlobals);
+  return &s_storage.globals;
 }
 
 // static
-int Globals::sdk_build_version_ = 0;
+int Globals::sdk_build_version = 0;
 
 }  // namespace crazy

third_party/android_crazy_linker/src/src/crazy_linker_globals.h

@@ -18,27 +18,41 @@ namespace crazy {
 
 class Globals {
  public:
-  Globals();
-  ~Globals();
-
-  void Lock() { pthread_mutex_lock(&lock_); }
-
-  void Unlock() { pthread_mutex_unlock(&lock_); }
-
+  // Get the single Globals instance for this process.
   static Globals* Get();
 
-  static LibraryList* GetLibraries() { return &Get()->libraries_; }
+  // Default constructor.
+  Globals();
+
+  // Destructor.
+  ~Globals();
 
-  static SearchPathList* GetSearchPaths() { return &Get()->search_paths_; }
+  // Acquire and release the mutex that protects all other non-static members.
+  // ScopedLockedGlobals is recommended, to avoid using these directly.
+  void Lock();
+  void Unlock();
 
-  static RDebug* GetRDebug() { return &Get()->rdebug_; }
+  // The list of libraries known to the crazy linker.
+  LibraryList* libraries() { return &libraries_; }
 
-  static int* GetSDKBuildVersion() { return &sdk_build_version_; }
+  // The RDebug instance for this process.
+  RDebug* rdebug() { return &rdebug_; }
 
   // Set of valid handles returned by the dlopen() wrapper. This is
   // required to deal with rare cases where the wrapper is passed
   // a handle that was opened with the system linker by mistake.
-  static PointerSet* GetValidHandles() { return &Get()->valid_handles_; }
+  PointerSet* valid_handles() { return &valid_handles_; }
+
+  // The current library search path list used by the dlopen() wrapper.
+  // Initialized from LD_LIBRARY_PATH when ::Get() creates the instance.
+  SearchPathList* search_path_list() { return &search_paths_; }
+
+  // Android API level for the current device (if known).
+  // This is static because it must be set before the first call to Get().
+  static int sdk_build_version;
+
+  // Convenience function to get the global RDebug instance.
+  static RDebug* GetRDebug() { return Get()->rdebug(); }
 
  private:
   pthread_mutex_t lock_;
@@ -46,15 +60,28 @@ class Globals {
   SearchPathList search_paths_;
   RDebug rdebug_;
   PointerSet valid_handles_;
-  static int sdk_build_version_;
 };
 
-// Helper class to access the globals with scoped locking.
-class ScopedGlobalLock {
+// Convenience class to retrieve the Globals instance and lock it at the same
+// time on construction, then release it on destruction. Also dereference can
+// be used to access global methods and members.
+class ScopedLockedGlobals {
  public:
-  ScopedGlobalLock() { Globals::Get()->Lock(); }
+  // Default constructor acquires the lock on the global instance.
+  ScopedLockedGlobals() : globals_(Globals::Get()) { globals_->Lock(); }
+
+  // Destructor releases the lock.
+  ~ScopedLockedGlobals() { globals_->Unlock(); }
 
-  ~ScopedGlobalLock() { Globals::Get()->Unlock(); }
+  // Disallow copy operations.
+  ScopedLockedGlobals(const ScopedLockedGlobals&) = delete;
+  ScopedLockedGlobals& operator=(const ScopedLockedGlobals&) = delete;
+
+  // Dereference operator.
+  Globals* operator->() { return globals_; }
+
+ private:
+  Globals* globals_;
 };
 
 }  // namespace crazy

third_party/android_crazy_linker/src/src/crazy_linker_globals_unittest.cpp

@@ -12,10 +12,11 @@ namespace crazy {
 
 TEST(Globals, Get) {
   SystemMock sys;
-  ASSERT_TRUE(Globals::Get());
-  ASSERT_TRUE(Globals::GetLibraries());
-  ASSERT_TRUE(Globals::GetSearchPaths());
-  ASSERT_TRUE(Globals::GetRDebug());
+  Globals* globals = Globals::Get();
+  ASSERT_TRUE(globals);
+  ASSERT_TRUE(globals->libraries());
+  ASSERT_TRUE(globals->search_path_list());
+  ASSERT_TRUE(globals->rdebug());
 }
 
 }  // namespace crazy

third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp

@@ -63,7 +63,10 @@ struct SymbolLookupState {
 }  // namespace
 
 LibraryList::LibraryList() : head_(0), has_error_(false) {
-  const int sdk_build_version = *Globals::GetSDKBuildVersion();
+  // NOTE: This constructor is called from the Globals::Globals() constructor,
+  // hence it is important that Globals::sdk_build_version is a static member
+  // that can be set before Globals::Get() is called for the first time.
+  const int sdk_build_version = Globals::sdk_build_version;
 
   // If SDK version is Lollipop or earlier, we need to load anything
   // listed in LD_PRELOAD explicitly, because dlsym() on the main executable

third_party/android_crazy_linker/src/src/crazy_linker_library_view.cpp

@@ -29,7 +29,7 @@ void* LibraryView::LookupSymbol(const char* symbol_name) {
     return ::dlsym(system_, symbol_name);
 
   if (type_ == TYPE_CRAZY) {
-    LibraryList* lib_list = Globals::GetLibraries();
+    LibraryList* lib_list = Globals::Get()->libraries();
     return lib_list->FindSymbolFrom(symbol_name, this);
   }
 

third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp

@@ -411,7 +411,7 @@ void RDebug::WriteLinkMapField(link_map_t** link_pointer, link_map_t* entry) {
 }
 
 void RDebug::AddEntryImpl(link_map_t* entry) {
-  ScopedGlobalLock lock;
+  ScopedLockedGlobals globals;  // TODO(digit): Remove this lock.
   LOG("Adding: %s", entry->l_name);
   if (!init_)
     Init();
@@ -469,7 +469,7 @@ void RDebug::AddEntryImpl(link_map_t* entry) {
 }
 
 void RDebug::DelEntryImpl(link_map_t* entry) {
-  ScopedGlobalLock lock;
+  ScopedLockedGlobals globals;  // TODO(digit): Remove this lock.
   LOG("Deleting: %s", entry->l_name);
   if (!r_debug_)
     return;

third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp

@@ -258,6 +258,7 @@ bool SharedLibrary::Load(const char* full_path,
 
   LOG("Parsing dynamic table for %s", base_name_);
   ElfView::DynamicIterator dyn(&view_);
+  RDebug* rdebug = Globals::GetRDebug();
   for (; dyn.HasNext(); dyn.GetNext()) {
     ELF::Addr dyn_value = dyn.GetValue();
     uintptr_t dyn_addr = dyn.GetAddress(load_bias());
@@ -265,7 +266,7 @@ bool SharedLibrary::Load(const char* full_path,
       case DT_DEBUG:
         if (view_.dynamic_flags() & PF_W) {
           *dyn.GetValuePointer() =
-              reinterpret_cast<uintptr_t>(Globals::GetRDebug()->GetAddress());
+              reinterpret_cast<uintptr_t>(rdebug->GetAddress());
         }
         break;
       case DT_INIT:
@@ -314,7 +315,7 @@ bool SharedLibrary::Load(const char* full_path,
 #if defined(__mips__)
       case DT_MIPS_RLD_MAP:
         *dyn.GetValuePointer() =
-            reinterpret_cast<ELF::Addr>(Globals::GetRDebug()->GetAddress());
+            reinterpret_cast<ELF::Addr>(rdebug->GetAddress());
         break;
 #endif
       default:

third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp

@@ -88,23 +88,18 @@ char* WrapDlerror() {
 }
 
 void* WrapDlopen(const char* path, int mode) {
-  ScopedGlobalLock lock;
+  ScopedLockedGlobals globals;
 
   // NOTE: If |path| is NULL, the wrapper should return a handle
   // corresponding to the current executable. This can't be a crazy
   // library, so don't try to handle it with the crazy linker.
   if (path) {
-    LibraryList* lib_list = Globals::GetLibraries();
     Error error;
-    LibraryView* wrap = lib_list->LoadLibrary(path,
-                                              mode,
-                                              0U /* load_address */,
-                                              0U /* file_offset */,
-                                              Globals::GetSearchPaths(),
-                                              false,
-                                              &error);
+    LibraryView* wrap = globals->libraries()->LoadLibrary(
+        path, mode, 0U /* load_address */, 0U /* file_offset */,
+        globals->search_path_list(), false, &error);
     if (wrap) {
-      Globals::GetValidHandles()->Add(wrap);
+      globals->valid_handles()->Add(wrap);
       return wrap;
     }
   }
@@ -119,8 +114,8 @@ void* WrapDlopen(const char* path, int mode) {
 
   LibraryView* wrap_lib = new LibraryView();
   wrap_lib->SetSystem(system_lib, path ? path : "<executable>");
-  Globals::GetLibraries()->AddLibrary(wrap_lib);
-  Globals::GetValidHandles()->Add(wrap_lib);
+  globals->libraries()->AddLibrary(wrap_lib);
+  globals->valid_handles()->Add(wrap_lib);
   return wrap_lib;
 }
 
@@ -149,8 +144,8 @@ void* WrapDlsym(void* lib_handle, const char* symbol_name) {
   // when |lib_handle| corresponds to a crazy library, except that
   // it stops at system libraries that it depends on.
 
-  ScopedGlobalLock lock;
-  if (!Globals::GetValidHandles()->Has(lib_handle)) {
+  ScopedLockedGlobals globals;
+  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);
@@ -178,8 +173,7 @@ void* WrapDlsym(void* lib_handle, const char* symbol_name) {
   }
 
   if (wrap_lib->IsCrazy()) {
-    LibraryList* lib_list = Globals::GetLibraries();
-    void* addr = lib_list->FindSymbolFrom(symbol_name, wrap_lib);
+    void* addr = globals->libraries()->FindSymbolFrom(symbol_name, wrap_lib);
     if (addr)
       return addr;
 
@@ -198,9 +192,8 @@ void* WrapDlsym(void* lib_handle, const char* symbol_name) {
 int WrapDladdr(void* address, Dl_info* info) {
   // First, perform search in crazy libraries.
   {
-    ScopedGlobalLock lock;
-    LibraryList* lib_list = Globals::GetLibraries();
-    LibraryView* wrap = lib_list->FindLibraryForAddress(address);
+    ScopedLockedGlobals globals;
+    LibraryView* wrap = globals->libraries()->FindLibraryForAddress(address);
     if (wrap && wrap->IsCrazy()) {
       size_t sym_size = 0;
 
@@ -229,8 +222,8 @@ int WrapDlclose(void* lib_handle) {
     return -1;
   }
 
-  ScopedGlobalLock lock;
-  if (!Globals::GetValidHandles()->Remove(lib_handle)) {
+  ScopedLockedGlobals globals;
+  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) {
@@ -244,8 +237,7 @@ int WrapDlclose(void* lib_handle) {
 
   LibraryView* wrap_lib = reinterpret_cast<LibraryView*>(lib_handle);
   if (wrap_lib->IsSystem() || wrap_lib->IsCrazy()) {
-    LibraryList* lib_list = Globals::GetLibraries();
-    lib_list->UnloadLibrary(wrap_lib);
+    globals->libraries()->UnloadLibrary(wrap_lib);
     return 0;
   }
 
@@ -258,10 +250,9 @@ int WrapDlclose(void* lib_handle) {
 _Unwind_Ptr WrapDl_unwind_find_exidx(_Unwind_Ptr pc, int* pcount) {
   // First lookup in crazy libraries.
   {
-    ScopedGlobalLock lock;
-    LibraryList* list = Globals::GetLibraries();
+    ScopedLockedGlobals globals;
     _Unwind_Ptr result =
-        list->FindArmExIdx(reinterpret_cast<void*>(pc), pcount);
+        globals->libraries()->FindArmExIdx(reinterpret_cast<void*>(pc), pcount);
     if (result)
       return result;
   }
@@ -272,9 +263,8 @@ _Unwind_Ptr WrapDl_unwind_find_exidx(_Unwind_Ptr pc, int* pcount) {
 int WrapDl_iterate_phdr(int (*cb)(dl_phdr_info*, size_t, void*), void* data) {
   // First, iterate over crazy libraries.
   {
-    ScopedGlobalLock lock;
-    LibraryList* list = Globals::GetLibraries();
-    int result = list->IteratePhdr(cb, data);
+    ScopedLockedGlobals globals;
+    int result = globals->libraries()->IteratePhdr(cb, data);
     if (result)
       return result;
   }
@@ -298,9 +288,9 @@ void* GetDlCloseWrapperAddressForTesting() {
 // free()-ed by the caller. This returns nullptr is the array is empty.
 // On exit, sets |*p_count| to the number of items in the array.
 void** GetValidDlopenHandlesForTesting(size_t* p_count) {
-  ScopedGlobalLock lock;
+  ScopedLockedGlobals globals;
   const Vector<void*>& handles =
-      Globals::GetValidHandles()->GetValuesForTesting();
+      globals->valid_handles()->GetValuesForTesting();
   *p_count = handles.GetCount();
   if (handles.IsEmpty())
     return nullptr;