From 43ac269bdd00d709005f8f3550db6b657b2bf940 Mon Sep 17 00:00:00 2001 From: Alex Langford Date: Fri, 14 Apr 2023 14:16:26 -0700 Subject: [PATCH] [lldb] Lock accesses to PathMappingLists's internals This class is not safe in multithreaded code. It's possible for one thread to modify a PathMappingList's `m_pair` vector while another thread is iterating over it, effectively invalidating the iterator and potentially leading to crashes or other difficult-to-diagnose bugs. rdar://107695786 Differential Revision: https://reviews.llvm.org/D148380 --- lldb/include/lldb/Target/PathMappingList.h | 17 ++++++++++++++--- lldb/source/Target/PathMappingList.cpp | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lldb/include/lldb/Target/PathMappingList.h b/lldb/include/lldb/Target/PathMappingList.h index 447fb43..2834522 100644 --- a/lldb/include/lldb/Target/PathMappingList.h +++ b/lldb/include/lldb/Target/PathMappingList.h @@ -13,6 +13,7 @@ #include "lldb/Utility/Status.h" #include "llvm/Support/JSON.h" #include +#include #include #include @@ -50,9 +51,15 @@ public: llvm::json::Value ToJSON(); - bool IsEmpty() const { return m_pairs.empty(); } + bool IsEmpty() const { + std::lock_guard lock(m_mutex); + return m_pairs.empty(); + } - size_t GetSize() const { return m_pairs.size(); } + size_t GetSize() const { + std::lock_guard lock(m_mutex); + return m_pairs.size(); + } bool GetPathsAtIndex(uint32_t idx, ConstString &path, ConstString &new_path) const; @@ -128,9 +135,13 @@ public: uint32_t FindIndexForPath(llvm::StringRef path) const; - uint32_t GetModificationID() const { return m_mod_id; } + uint32_t GetModificationID() const { + std::lock_guard lock(m_mutex); + return m_mod_id; + } protected: + mutable std::recursive_mutex m_mutex; typedef std::pair pair; typedef std::vector collection; typedef collection::iterator iterator; diff --git a/lldb/source/Target/PathMappingList.cpp b/lldb/source/Target/PathMappingList.cpp index 13bb50b..4efc0bf 100644 --- a/lldb/source/Target/PathMappingList.cpp +++ b/lldb/source/Target/PathMappingList.cpp @@ -48,6 +48,7 @@ PathMappingList::PathMappingList(const PathMappingList &rhs) const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) { if (this != &rhs) { + std::scoped_lock locks(m_mutex, rhs.m_mutex); m_pairs = rhs.m_pairs; m_callback = nullptr; m_callback_baton = nullptr; @@ -60,6 +61,7 @@ PathMappingList::~PathMappingList() = default; void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement, bool notify) { + std::lock_guard lock(m_mutex); ++m_mod_id; m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement))); if (notify && m_callback) @@ -67,6 +69,7 @@ void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement, } void PathMappingList::Append(const PathMappingList &rhs, bool notify) { + std::scoped_lock locks(m_mutex, rhs.m_mutex); ++m_mod_id; if (!rhs.m_pairs.empty()) { const_iterator pos, end = rhs.m_pairs.end(); @@ -81,6 +84,7 @@ bool PathMappingList::AppendUnique(llvm::StringRef path, llvm::StringRef replacement, bool notify) { auto normalized_path = NormalizePath(path); auto normalized_replacement = NormalizePath(replacement); + std::lock_guard lock(m_mutex); for (const auto &pair : m_pairs) { if (pair.first.GetStringRef().equals(normalized_path) && pair.second.GetStringRef().equals(normalized_replacement)) @@ -92,6 +96,7 @@ bool PathMappingList::AppendUnique(llvm::StringRef path, void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement, uint32_t index, bool notify) { + std::lock_guard lock(m_mutex); ++m_mod_id; iterator insert_iter; if (index >= m_pairs.size()) @@ -106,6 +111,7 @@ void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement, bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement, uint32_t index, bool notify) { + std::lock_guard lock(m_mutex); if (index >= m_pairs.size()) return false; ++m_mod_id; @@ -116,6 +122,7 @@ bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement, } bool PathMappingList::Remove(size_t index, bool notify) { + std::lock_guard lock(m_mutex); if (index >= m_pairs.size()) return false; @@ -130,6 +137,7 @@ bool PathMappingList::Remove(size_t index, bool notify) { // For clients which do not need the pair index dumped, pass a pair_index >= 0 // to only dump the indicated pair. void PathMappingList::Dump(Stream *s, int pair_index) { + std::lock_guard lock(m_mutex); unsigned int numPairs = m_pairs.size(); if (pair_index < 0) { @@ -147,6 +155,7 @@ void PathMappingList::Dump(Stream *s, int pair_index) { llvm::json::Value PathMappingList::ToJSON() { llvm::json::Array entries; + std::lock_guard lock(m_mutex); for (const auto &pair : m_pairs) { llvm::json::Array entry{pair.first.GetStringRef().str(), pair.second.GetStringRef().str()}; @@ -156,6 +165,7 @@ llvm::json::Value PathMappingList::ToJSON() { } void PathMappingList::Clear(bool notify) { + std::lock_guard lock(m_mutex); if (!m_pairs.empty()) ++m_mod_id; m_pairs.clear(); @@ -186,6 +196,7 @@ static void AppendPathComponents(FileSpec &path, llvm::StringRef components, std::optional PathMappingList::RemapPath(llvm::StringRef mapping_path, bool only_if_exists) const { + std::lock_guard lock(m_mutex); if (m_pairs.empty() || mapping_path.empty()) return {}; LazyBool path_is_relative = eLazyBoolCalculate; @@ -224,6 +235,7 @@ std::optional PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const { std::string path = file.GetPath(); llvm::StringRef path_ref(path); + std::lock_guard lock(m_mutex); for (const auto &it : m_pairs) { llvm::StringRef removed_prefix = it.second.GetStringRef(); if (!path_ref.consume_front(it.second.GetStringRef())) @@ -252,6 +264,7 @@ PathMappingList::FindFile(const FileSpec &orig_spec) const { bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path, bool notify) { + std::lock_guard lock(m_mutex); uint32_t idx = FindIndexForPath(path); if (idx < m_pairs.size()) { ++m_mod_id; @@ -264,6 +277,7 @@ bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path, } bool PathMappingList::Remove(ConstString path, bool notify) { + std::lock_guard lock(m_mutex); iterator pos = FindIteratorForPath(path); if (pos != m_pairs.end()) { ++m_mod_id; @@ -277,6 +291,7 @@ bool PathMappingList::Remove(ConstString path, bool notify) { PathMappingList::const_iterator PathMappingList::FindIteratorForPath(ConstString path) const { + std::lock_guard lock(m_mutex); const_iterator pos; const_iterator begin = m_pairs.begin(); const_iterator end = m_pairs.end(); @@ -290,6 +305,7 @@ PathMappingList::FindIteratorForPath(ConstString path) const { PathMappingList::iterator PathMappingList::FindIteratorForPath(ConstString path) { + std::lock_guard lock(m_mutex); iterator pos; iterator begin = m_pairs.begin(); iterator end = m_pairs.end(); @@ -303,6 +319,7 @@ PathMappingList::FindIteratorForPath(ConstString path) { bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path, ConstString &new_path) const { + std::lock_guard lock(m_mutex); if (idx < m_pairs.size()) { path = m_pairs[idx].first; new_path = m_pairs[idx].second; @@ -313,6 +330,7 @@ bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path, uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const { const ConstString path = ConstString(NormalizePath(orig_path)); + std::lock_guard lock(m_mutex); const_iterator pos; const_iterator begin = m_pairs.begin(); const_iterator end = m_pairs.end(); -- 2.7.4