crazy linker: Fix failure to cleanly unload libraries.

On library load and unload we post callbacks to the main thread
to update gdb data.  Having the callback run asynchronously on
load is okay, and in fact required because the loop that invokes
the callback does not run until after library load is complete.

However, having it run asynchronously on unload is problematic
because the objects manipulated by the callback may have been
deleted before the callback is invoked, leading to a crash.

Fix by adding the concept of a synchronous callback, so that on
unloading a library we wait for the callback to occur before
completing the unload process.

Note: an unload callback crash may occur in unit tests but not
currently in the browser itself.  This is because the browser
terminates without attempting to cleanly unload its libraries.

BUG=

Review URL: https://codereview.chromium.org/724823002

Cr-Commit-Position: refs/heads/master@{#304620}

third_party/android_crazy_linker/README.chromium

@@ -64,3 +64,5 @@ Local Modifications:
 
 - Control static_assert invocations with a new COMPILE_ASSERT macro.
 
+- Fix unit test crash caused by use of deleted data inside an unload callback.
+

third_party/android_crazy_linker/src/include/crazy_linker.h

@@ -182,6 +182,14 @@ typedef bool (*crazy_callback_poster_t)(
 // |crazy_callback_poster_t| function, that will be called during library
 // loading to let the user record callbacks for delayed operations.
 // Callers must copy the |crazy_callback_t| passed to |poster|.
+//
+// Note: If client code calls this function to supply a callback poster,
+// it must guarantee to invoke any callback requested through the poster.
+// The call will be (typically) on another thread, but may instead be
+// immediate from the poster. However, the callback must be invoked,
+// otherwise if it is a blocking callback the crazy linker will deadlock
+// waiting for it.
+//
 // |poster_opaque| is an opaque value for client code use, passed back
 // on each call to |poster|.
 // |poster| can be NULL to disable the feature.

third_party/android_crazy_linker/src/src/crazy_linker_rdebug.cpp

@@ -6,6 +6,7 @@
 
 #include <elf.h>
 #include <inttypes.h>
+#include <pthread.h>
 #include <sys/mman.h>
 #include <unistd.h>
 
@@ -266,22 +267,46 @@ bool RDebug::Init() {
 
 namespace {
 
+// Helper class providing a simple scoped pthreads mutex.
+class ScopedMutexLock {
+ public:
+  explicit ScopedMutexLock(pthread_mutex_t* mutex) : mutex_(mutex) {
+    pthread_mutex_lock(mutex_);
+  }
+  ~ScopedMutexLock() {
+    pthread_mutex_unlock(mutex_);
+  }
+
+ private:
+  pthread_mutex_t* mutex_;
+};
+
 // Helper runnable class. Handler is one of the two static functions
 // AddEntryInternal() or DelEntryInternal(). Calling these invokes
 // AddEntryImpl() or DelEntryImpl() respectively on rdebug.
 class RDebugRunnable {
  public:
   RDebugRunnable(rdebug_callback_handler_t handler,
                  RDebug* rdebug,
-                 link_map_t* entry)
-      : handler_(handler), rdebug_(rdebug), entry_(entry) { }
+                 link_map_t* entry,
+                 bool is_blocking)
+      : handler_(handler), rdebug_(rdebug),
+        entry_(entry), is_blocking_(is_blocking), has_run_(false) {
+    pthread_mutex_init(&mutex_, NULL);
+    pthread_cond_init(&cond_, NULL);
+  }
 
   static void Run(void* opaque);
+  static void WaitForCallback(void* opaque);
 
  private:
   rdebug_callback_handler_t handler_;
   RDebug* rdebug_;
   link_map_t* entry_;
+  bool is_blocking_;
+  bool has_run_;
+  pthread_mutex_t mutex_;
+  pthread_cond_t cond_;
 };
 
 // Callback entry point.
@@ -290,6 +315,36 @@ void RDebugRunnable::Run(void* opaque) {
 
   LOG("%s: Callback received, runnable=%p\n", __FUNCTION__, runnable);
   (*runnable->handler_)(runnable->rdebug_, runnable->entry_);
+
+  if (!runnable->is_blocking_) {
+    delete runnable;
+    return;
+  }
+
+  LOG("%s: Signalling callback, runnable=%p\n", __FUNCTION__, runnable);
+  {
+    ScopedMutexLock m(&runnable->mutex_);
+    runnable->has_run_ = true;
+    pthread_cond_signal(&runnable->cond_);
+  }
+}
+
+// For blocking callbacks, wait for the call to Run().
+void RDebugRunnable::WaitForCallback(void* opaque) {
+  RDebugRunnable* runnable = static_cast<RDebugRunnable*>(opaque);
+
+  if (!runnable->is_blocking_) {
+    LOG("%s: Non-blocking, not waiting, runnable=%p\n", __FUNCTION__, runnable);
+    return;
+  }
+
+  LOG("%s: Waiting for signal, runnable=%p\n", __FUNCTION__, runnable);
+  {
+    ScopedMutexLock m(&runnable->mutex_);
+    while (!runnable->has_run_)
+      pthread_cond_wait(&runnable->cond_, &runnable->mutex_);
+  }
+
   delete runnable;
 }
 
@@ -300,13 +355,15 @@ void RDebugRunnable::Run(void* opaque) {
 // linker, which expects to be able to set r_map pages readonly when it
 // is not using them and which may run simultaneously on the main thread.
 bool RDebug::PostCallback(rdebug_callback_handler_t handler,
-                          link_map_t* entry) {
+                          link_map_t* entry,
+                          bool is_blocking) {
   if (!post_for_later_execution_) {
     LOG("%s: Deferred execution disabled\n", __FUNCTION__);
     return false;
   }
 
-  RDebugRunnable* runnable = new RDebugRunnable(handler, this, entry);
+  RDebugRunnable* runnable =
+      new RDebugRunnable(handler, this, entry, is_blocking);
   void* context = post_for_later_execution_context_;
 
   if (!(*post_for_later_execution_)(context, &RDebugRunnable::Run, runnable)) {
@@ -316,6 +373,12 @@ bool RDebug::PostCallback(rdebug_callback_handler_t handler,
   }
 
   LOG("%s: Posted for later execution, runnable=%p\n", __FUNCTION__, runnable);
+
+  if (is_blocking) {
+    RDebugRunnable::WaitForCallback(runnable);
+    LOG("%s: Completed execution, runnable=%p\n", __FUNCTION__, runnable);
+  }
+
   return true;
 }
 

third_party/android_crazy_linker/src/src/crazy_linker_rdebug.h

@@ -171,10 +171,21 @@ class RDebug {
   ~RDebug() {}
 
   // Add entries to and remove entries from the list. If post for later
-  // execution is enabled, schedule callbacks and return. Otherwise
-  // action immediately.
-  void AddEntry(link_map_t* entry) { RunOrDelay(&AddEntryInternal, entry); }
-  void DelEntry(link_map_t* entry) { RunOrDelay(&DelEntryInternal, entry); }
+  // execution is enabled, schedule callbacks, otherwise action immediately.
+  //
+  // Callbacks may be blocking or non-blocking. On a blocking callback
+  // we wait for the other thread to call the callback before proceeding;
+  // on a non-blocking one, we proceed without waiting. Adding an entry
+  // requires a non-blocking callback, because the loop that invokes the
+  // callback is not started until after libraries are loaded. Deleting an
+  // entry requires a blocking callback, so that the objects referenced
+  // by the callback code are not destroyed before the callback is invoked.
+  void AddEntry(link_map_t* entry) {
+    RunOrDelay(&AddEntryInternal, entry, false);
+  }
+  void DelEntry(link_map_t* entry) {
+    RunOrDelay(&DelEntryInternal, entry, true);
+  }
 
   // Assign the function used to request a callback from another thread.
   // The context here is opaque, but is the API's crazy_context.
@@ -207,12 +218,18 @@ class RDebug {
   }
 
   // Post handler for delayed execution. Return true if delayed execution
-  // is enabled and posting succeeded.
-  bool PostCallback(rdebug_callback_handler_t handler, link_map_t* entry);
-
-  // Run handler as a callback if enabled, otherwise immediately.
-  void RunOrDelay(rdebug_callback_handler_t handler, link_map_t* entry) {
-    if (!PostCallback(handler, entry))
+  // is enabled and posting succeeded. If is_blocking, waits until the
+  // callback is received before returning.
+  bool PostCallback(rdebug_callback_handler_t handler,
+                    link_map_t* entry,
+                    bool is_blocking);
+
+  // Run handler as a callback if enabled, otherwise immediately. Posts
+  // either a blocking or a non-blocking callback.
+  void RunOrDelay(rdebug_callback_handler_t handler,
+                  link_map_t* entry,
+                  bool is_blocking) {
+    if (!PostCallback(handler, entry, is_blocking))
       (*handler)(this, entry);
   }
 

third_party/android_crazy_linker/src/tests/test_load_library_callbacks.cpp

@@ -4,41 +4,104 @@
 
 // A crazy linker test to test callbacks for delayed execution.
 
+#include <pthread.h>
 #include <stdio.h>
 #include <crazy_linker.h>
 
 #include "test_util.h"
 
+namespace {
+
 typedef void (*FunctionPtr)();
 
-namespace {
+// Data block passed between callback poster and callback handler.
+class CallbackData {
+ public:
+  CallbackData() {
+    callback_.handler = NULL;
+    callback_.opaque = NULL;
+    pthread_mutex_init(&mutex_, NULL);
+    pthread_cond_init(&cond_, NULL);
+  }
+
+  crazy_callback_t callback_;
+  pthread_mutex_t mutex_;
+  pthread_cond_t cond_;
+};
 
 bool PostCallback(crazy_callback_t* callback, void* poster_opaque) {
   printf("Post callback, poster_opaque %p, handler %p, opaque %p\n",
          poster_opaque,
          callback->handler,
          callback->opaque);
 
-  // Copy callback to the address given in poster_opaque.
-  crazy_callback_t* request =
-      reinterpret_cast<crazy_callback_t*>(poster_opaque);
-  *request = *callback;
+  CallbackData* callback_data = reinterpret_cast<CallbackData*>(poster_opaque);
+
+  // Set callback_ and signal the arrival of the PostCallback() call.
+  pthread_mutex_lock(&callback_data->mutex_);
+  callback_data->callback_ = *callback;
+  pthread_cond_signal(&callback_data->cond_);
+  pthread_mutex_unlock(&callback_data->mutex_);
 
   return true;
 }
 
-void CheckAndRunCallback(crazy_callback_t* callback) {
-  printf("Run callback, handler %p, opaque %p\n",
-         callback->handler,
-         callback->opaque);
+void CheckAndRunCallback(CallbackData* callback_data) {
+  printf("Run callback, callback_data %p\n", callback_data);
 
-  if (!callback->handler) {
+  if (!callback_data->callback_.handler) {
     Panic("Post for delayed execution not invoked\n");
   }
 
-  // Run the callback, then clear it for re-use.
-  crazy_callback_run(callback);
-  memset(callback, 0, sizeof(*callback));
+  // Run the callback, then clear it.
+  crazy_callback_run(&callback_data->callback_);
+  callback_data->callback_.handler = NULL;
+  callback_data->callback_.opaque = NULL;
+}
+
+struct ThreadData {
+  crazy_library_t* library;
+  crazy_context_t* context;
+};
+
+void* ThreadBody(void *thread_arg) {
+  const ThreadData* thread_data = reinterpret_cast<ThreadData*>(thread_arg);
+
+  // Close the library, asynchronously.
+  crazy_library_close_with_context(thread_data->library, thread_data->context);
+  pthread_exit(NULL);
+}
+
+pthread_t AsyncCrazyLibraryCloseWithContext(crazy_library_t* library,
+                                            crazy_context_t* context,
+                                            CallbackData* callback_data) {
+  printf("Async close, library %p, context %p\n", library, context);
+
+  ThreadData thread_data = {library, context};
+  void* thread_arg = reinterpret_cast<void*>(&thread_data);
+
+  // Clear the indication that the new thread has called PostCallback().
+  pthread_mutex_lock(&callback_data->mutex_);
+  callback_data->callback_.handler = NULL;
+  callback_data->callback_.opaque = NULL;
+  pthread_mutex_unlock(&callback_data->mutex_);
+
+  // Start the thread that closes the library.
+  pthread_t thread;
+  if (pthread_create(&thread, NULL, ThreadBody, thread_arg) != 0) {
+    Panic("Failed to create thread for close\n");
+  }
+
+  // Wait for the library close to call PostCallback() before returning.
+  printf("Waiting for PostCallback() call\n");
+  pthread_mutex_lock(&callback_data->mutex_);
+  while (!callback_data->callback_.handler) {
+    pthread_cond_wait(&callback_data->cond_, &callback_data->mutex_);
+  }
+  pthread_mutex_unlock(&callback_data->mutex_);
+  printf("Done waiting for PostCallback() call\n");
+
+  return thread;
 }
 
 }  // namespace
@@ -50,26 +113,24 @@ int main() {
   // DEBUG
   crazy_context_set_load_address(context, 0x20000000);
 
-  // Create a new callback, initialized to NULLs.
-  crazy_callback_t callback = {NULL, NULL};
-
-  // Set a callback poster that copies its callback to &callback.
-  crazy_context_set_callback_poster(context, &PostCallback, &callback);
+  // Set a callback poster.
+  CallbackData callback_data;
+  crazy_context_set_callback_poster(context, &PostCallback, &callback_data);
 
   crazy_callback_poster_t poster;
   void* poster_opaque;
 
   // Check that the API returns the values we set.
   crazy_context_get_callback_poster(context, &poster, &poster_opaque);
-  if (poster != &PostCallback || poster_opaque != &callback) {
+  if (poster != &PostCallback || poster_opaque != &callback_data) {
     Panic("Get callback poster error\n");
   }
 
   // Load libfoo.so.
   if (!crazy_library_open(&library, "libfoo.so", context)) {
     Panic("Could not open library: %s\n", crazy_context_get_error(context));
   }
-  CheckAndRunCallback(&callback);
+  CheckAndRunCallback(&callback_data);
 
   // Find the "Foo" symbol.
   FunctionPtr foo_func;
@@ -81,11 +142,17 @@ int main() {
   // Call it.
   (*foo_func)();
 
-  // Close the library.
-  crazy_library_close_with_context(library, context);
-  CheckAndRunCallback(&callback);
+  // Close the library.  Because the close operation will wait for the
+  // callback before returning, we have to run it in a separate thread, and
+  // wait for it to call PostCallback() before continuing.
+  pthread_t thread =
+      AsyncCrazyLibraryCloseWithContext(library, context, &callback_data);
+  CheckAndRunCallback(&callback_data);
 
-  crazy_context_destroy(context);
+  if (pthread_join(thread, NULL) != 0) {
+    Panic("Failed to join thread for close\n");
+  }
 
+  crazy_context_destroy(context);
   return 0;
 }