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());