Merge "Fix race condition updating local map data."

6 files changed, 45 insertions, 5 deletions

diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp
index fa983fa1f..dfdf29cdf 100644
--- a/debuggerd/tombstone.cpp
+++ b/debuggerd/tombstone.cpp
@@ -368,6 +368,7 @@ static void dump_all_maps(Backtrace* backtrace, BacktraceMap* map, log_t* log, p
     ALOGE("Cannot get siginfo for %d: %s\n", tid, strerror(errno));
   }
 
+  ScopedBacktraceMapIteratorLock lock(map);
   _LOG(log, logtype::MAPS, "\n");
   if (!print_fault_address_marker) {
     _LOG(log, logtype::MAPS, "memory map:\n");
diff --git a/include/backtrace/BacktraceMap.h b/include/backtrace/BacktraceMap.h
index 2373c45e8..b80045fe1 100644
--- a/include/backtrace/BacktraceMap.h
+++ b/include/backtrace/BacktraceMap.h
@@ -71,6 +71,12 @@ public:
   bool IsWritable(uintptr_t pc) { return GetFlags(pc) & PROT_WRITE; }
   bool IsExecutable(uintptr_t pc) { return GetFlags(pc) & PROT_EXEC; }
 
+  // In order to use the iterators on this object, a caller must
+  // call the LockIterator and UnlockIterator function to guarantee
+  // that the data does not change while it's being used.
+  virtual void LockIterator() {}
+  virtual void UnlockIterator() {}
+
   typedef std::deque<backtrace_map_t>::iterator iterator;
   iterator begin() { return maps_.begin(); }
   iterator end() { return maps_.end(); }
@@ -102,4 +108,18 @@ protected:
   pid_t pid_;
 };
 
+class ScopedBacktraceMapIteratorLock {
+public:
+  explicit ScopedBacktraceMapIteratorLock(BacktraceMap* map) : map_(map) {
+    map->LockIterator();
+  }
+
+  ~ScopedBacktraceMapIteratorLock() {
+    map_->UnlockIterator();
+  }
+
+private:
+  BacktraceMap* map_;
+};
+
 #endif // _BACKTRACE_BACKTRACE_MAP_H
diff --git a/libbacktrace/BacktraceMap.cpp b/libbacktrace/BacktraceMap.cpp
index 19551dc57..00c35b18d 100644
--- a/libbacktrace/BacktraceMap.cpp
+++ b/libbacktrace/BacktraceMap.cpp
@@ -36,8 +36,8 @@ BacktraceMap::~BacktraceMap() {
 }
 
 void BacktraceMap::FillIn(uintptr_t addr, backtrace_map_t* map) {
-  for (BacktraceMap::const_iterator it = begin();
-       it != end(); ++it) {
+  ScopedBacktraceMapIteratorLock lock(this);
+  for (BacktraceMap::const_iterator it = begin(); it != end(); ++it) {
     if (addr >= it->start && addr < it->end) {
       *map = *it;
       return;
diff --git a/libbacktrace/UnwindMap.cpp b/libbacktrace/UnwindMap.cpp
index 34d79f970..af7956246 100644
--- a/libbacktrace/UnwindMap.cpp
+++ b/libbacktrace/UnwindMap.cpp
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include <pthread.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/types.h>
@@ -72,6 +73,7 @@ bool UnwindMapRemote::Build() {
 }
 
 UnwindMapLocal::UnwindMapLocal() : UnwindMap(getpid()), map_created_(false) {
+  pthread_rwlock_init(&map_lock_, nullptr);
 }
 
 UnwindMapLocal::~UnwindMapLocal() {
@@ -82,9 +84,14 @@ UnwindMapLocal::~UnwindMapLocal() {
 }
 
 bool UnwindMapLocal::GenerateMap() {
+  // Lock so that multiple threads cannot modify the maps data at the
+  // same time.
+  pthread_rwlock_wrlock(&map_lock_);
+
   // It's possible for the map to be regenerated while this loop is occurring.
   // If that happens, get the map again, but only try at most three times
   // before giving up.
+  bool generated = false;
   for (int i = 0; i < 3; i++) {
     maps_.clear();
 
@@ -110,12 +117,17 @@ bool UnwindMapLocal::GenerateMap() {
     }
     // Check to see if the map changed while getting the data.
     if (ret != -UNW_EINVAL) {
-      return true;
+      generated = true;
+      break;
     }
   }
 
-  BACK_LOGW("Unable to generate the map.");
-  return false;
+  pthread_rwlock_unlock(&map_lock_);
+
+  if (!generated) {
+    BACK_LOGW("Unable to generate the map.");
+  }
+  return generated;
 }
 
 bool UnwindMapLocal::Build() {
diff --git a/libbacktrace/UnwindMap.h b/libbacktrace/UnwindMap.h
index 111401ffa..f85b54a9f 100644
--- a/libbacktrace/UnwindMap.h
+++ b/libbacktrace/UnwindMap.h
@@ -17,6 +17,7 @@
 #ifndef _LIBBACKTRACE_UNWIND_MAP_H
 #define _LIBBACKTRACE_UNWIND_MAP_H
 
+#include <pthread.h>
 #include <stdint.h>
 #include <sys/types.h>
 
@@ -56,10 +57,15 @@ public:
 
   void FillIn(uintptr_t addr, backtrace_map_t* map) override;
 
+  void LockIterator() override { pthread_rwlock_rdlock(&map_lock_); }
+  void UnlockIterator() override { pthread_rwlock_unlock(&map_lock_); }
+
 private:
   bool GenerateMap();
 
   bool map_created_;
+
+  pthread_rwlock_t map_lock_;
 };
 
 #endif // _LIBBACKTRACE_UNWIND_MAP_H
diff --git a/libbacktrace/backtrace_test.cpp b/libbacktrace/backtrace_test.cpp
index 7066c798c..e25c8e966 100644
--- a/libbacktrace/backtrace_test.cpp
+++ b/libbacktrace/backtrace_test.cpp
@@ -895,6 +895,7 @@ void VerifyMap(pid_t pid) {
   std::unique_ptr<BacktraceMap> map(BacktraceMap::Create(pid));
 
   // Basic test that verifies that the map is in the expected order.
+  ScopedBacktraceMapIteratorLock lock(map.get());
   std::vector<map_test_t>::const_iterator test_it = test_maps.begin();
   for (BacktraceMap::const_iterator it = map->begin(); it != map->end(); ++it) {
     ASSERT_TRUE(test_it != test_maps.end());