From 0c572d01afdf9bfee340ab5c167d744ea91316bf Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Wed, 15 Jun 2016 15:49:50 -0700 Subject: [PATCH] Fix race condition updating local map data. 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 --- debuggerd/tombstone.cpp | 1 + include/backtrace/BacktraceMap.h | 20 ++++++++++++++++++++ libbacktrace/BacktraceMap.cpp | 4 ++-- libbacktrace/UnwindMap.cpp | 18 +++++++++++++++--- libbacktrace/UnwindMap.h | 6 ++++++ libbacktrace/backtrace_test.cpp | 1 + 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::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 ba866328c..85f24361e 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 34d79f970..af7956246 100644 --- a/libbacktrace/UnwindMap.cpp +++ b/libbacktrace/UnwindMap.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include #include @@ -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 #include #include @@ -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 f6b2591a2..913e12dca 100644 --- a/libbacktrace/backtrace_test.cpp +++ b/libbacktrace/backtrace_test.cpp @@ -896,6 +896,7 @@ void VerifyMap(pid_t pid) { std::unique_ptr map(BacktraceMap::Create(pid)); // Basic test that verifies that the map is in the expected order. + ScopedBacktraceMapIteratorLock lock(map.get()); std::vector::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());