Bug 1460989 - Hold system linker lock while modifying debug map. r=glandium, a=RyanVM
authorJim Chen <nchen@mozilla.com>
Fri, 15 Jun 2018 04:24:10 -0400
changeset 473776 c323b0b175c9e92bd802af3c243a314149a2d4af
parent 473775 9f2e3a37b12f797fbed1037e3d418f072a1addd5
child 473777 30e4f7b85f9eb968e4c13e6fa8be6ae42f00e2f5
push id1731
push userryanvm@gmail.com
push dateWed, 20 Jun 2018 14:21:08 +0000
treeherdermozilla-release@f5ea361ceb75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium, RyanVM
bugs1460989
milestone61.0
Bug 1460989 - Hold system linker lock while modifying debug map. r=glandium, a=RyanVM When we modify the debug map, we could be racing with the system linker, either when we modify the entries or when we change page protection flags. To fix the race, we need to take the system linker's internal lock when we perform any kind of modification on the debug map. One way to hold the system linker lock is to call dl_iterate_phdr, and perform our actions inside the callback, which is invoked with the lock being held. However, dl_iterate_phdr is only present on Android 5.0+, and even then, dl_iterate_phdr is only protected by the linker lock on Android 6.0+. This means that with this patch, we can only safely modify the debug map on Android 6.0+, which I think is acceptable for an operation that only benefits a debugger. MozReview-Commit-ID: BowBEO8tu8Z
mozglue/linker/ElfLoader.cpp
--- a/mozglue/linker/ElfLoader.cpp
+++ b/mozglue/linker/ElfLoader.cpp
@@ -21,16 +21,17 @@
 
 using namespace mozilla;
 
 // From Utils.h
 Atomic<size_t, ReleaseAcquire> gPageSize;
 
 #if defined(ANDROID)
 #include <sys/syscall.h>
+#include <sys/system_properties.h>
 #include <math.h>
 
 #include <android/api-level.h>
 #if __ANDROID_API__ < 8
 /* Android API < 8 doesn't provide sigaltstack */
 
 extern "C" {
 
@@ -306,16 +307,66 @@ const char *
 LeafName(const char *path)
 {
   const char *lastSlash = strrchr(path, '/');
   if (lastSlash)
     return lastSlash + 1;
   return path;
 }
 
+#if defined(ANDROID)
+/**
+ * Return the current Android version, or 0 on failure.
+ */
+int
+GetAndroidSDKVersion() {
+  static int version = 0;
+  if (version) {
+    return version;
+  }
+
+  char version_string[PROP_VALUE_MAX] = {'\0'};
+  int len = __system_property_get("ro.build.version.sdk", version_string);
+  if (len) {
+    version = static_cast<int>(strtol(version_string, nullptr, 10));
+  }
+  return version;
+}
+#endif
+
+/**
+ * Run the given lambda while holding the internal lock of the system linker.
+ * To take the lock, we call the system dl_iterate_phdr and invoke the lambda
+ * from the callback, which is called while the lock is held. Return true on
+ * success.
+ */
+template<class Lambda>
+static bool
+RunWithSystemLinkerLock(Lambda&& aLambda) {
+  if (!dl_iterate_phdr) {
+    // No dl_iterate_phdr support.
+    return false;
+  }
+
+#if defined(ANDROID)
+  if (GetAndroidSDKVersion() < 23) {
+    // dl_iterate_phdr is _not_ protected by a lock on Android < 23.
+    // Also return false here if we failed to get the version.
+    return false;
+  }
+#endif
+
+  dl_iterate_phdr([] (dl_phdr_info*, size_t, void* lambda) -> int {
+    (*static_cast<Lambda*>(lambda))();
+    // Return 1 to stop iterating.
+    return 1;
+  }, &aLambda);
+  return true;
+}
+
 } /* Anonymous namespace */
 
 /**
  * LibHandle
  */
 LibHandle::~LibHandle()
 {
   free(path);
@@ -571,17 +622,19 @@ ElfLoader::Register(LibHandle *handle)
   handles.push_back(handle);
 }
 
 void
 ElfLoader::Register(CustomElf *handle)
 {
   Register(static_cast<LibHandle *>(handle));
   if (dbg) {
-    dbg.Add(handle);
+    // We could race with the system linker when modifying the debug map, so
+    // only do so while holding the system linker's internal lock.
+    RunWithSystemLinkerLock([this, handle] { dbg.Add(handle); });
   }
 }
 
 void
 ElfLoader::Forget(LibHandle *handle)
 {
   /* Ensure logging is initialized or refresh if environment changed. */
   Logging::Init();
@@ -598,17 +651,19 @@ ElfLoader::Forget(LibHandle *handle)
   }
 }
 
 void
 ElfLoader::Forget(CustomElf *handle)
 {
   Forget(static_cast<LibHandle *>(handle));
   if (dbg) {
-    dbg.Remove(handle);
+    // We could race with the system linker when modifying the debug map, so
+    // only do so while holding the system linker's internal lock.
+    RunWithSystemLinkerLock([this, handle] { dbg.Remove(handle); });
   }
 }
 
 void
 ElfLoader::Init()
 {
   Dl_info info;
   /* On Android < 4.1 can't reenter dl* functions. So when the library