Fix race condition updating local map data.nougat-mr2.3-releasenougat-mr2.2-releasenougat-mr2.1-releasenougat-mr2-security-releasenougat-mr2-releasenougat-mr2-pixel-releasenougat-mr2-devnougat-mr1.8-releasenougat-mr1.7-releasenougat-mr1.6-releasenougat-mr1.5-releasenougat-mr1.4-releasenougat-mr1.3-releasenougat-mr1.2-releasenougat-mr1.1-releasenougat-mr1-volantis-releasenougat-mr1-security-releasenougat-mr1-releasenougat-mr1-flounder-releasenougat-mr1-devnougat-mr1-cts-releasenougat-mr1-cts-dev

If the underlying local map changes, it's possible for multiple
threads to try and modify the map data associated with the UnwindLocalMap
object. Add a lock when generating the local map to avoid this problem.

In addition, add a read lock whenever any caller gets the maps iterator.
Updated all iterator callers to make this lock.

Bug: 29387050
Bug: 31067025

(cherry picked from commit 3a14004c7f521cf2ca6dfea182fa7441e77c97e7)

Change-Id: Id00116f156a24b36085c0d5dfc3dde4d2ac55194

4 files changed, 24 insertions, 5 deletions

diff --git a/libbacktrace/BacktraceMap.cpp b/libbacktrace/BacktraceMap.cpp
index ba86632..85f2436 100644
--- a/libbacktrace/BacktraceMap.cpp
+++ b/libbacktrace/BacktraceMap.cpp
@@ -35,8 +35,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 34d79f9..af79562 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 111401f..f85b54a 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 f6b2591..913e12d 100644
--- a/libbacktrace/backtrace_test.cpp
+++ b/libbacktrace/backtrace_test.cpp
@@ -896,6 +896,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());