[lldb] Lock accesses to PathMappingLists's internals

[bulbazord]

Summary

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

[JDevlieghere]

Does this actually have to be a recursive mutex?

[bulbazord]

In D148380#4269862, @JDevlieghere wrote:

Does this actually have to be a recursive mutex?

Good point, I don't think it does. I'll update this.

[bulbazord]

In D148380#4269876, @bulbazord wrote:

In D148380#4269862, @JDevlieghere wrote:

Does this actually have to be a recursive mutex?

Good point, I don't think it does. I'll update this.

Scratch that -- It unfortunately does need to be a recursive mutex. PathMappingList has a callback that may try to perform another operation on the list itself. This happens if you try to use target modules search-paths add -- We append to the list, the specified callback tries to read the size of the list. With a std::mutex we deadlock.

[JDevlieghere]

In D148380#4270085, @bulbazord wrote:

In D148380#4269876, @bulbazord wrote:

In D148380#4269862, @JDevlieghere wrote:

Does this actually have to be a recursive mutex?

Good point, I don't think it does. I'll update this.

Scratch that -- It unfortunately does need to be a recursive mutex. PathMappingList has a callback that may try to perform another operation on the list itself. This happens if you try to use target modules search-paths add -- We append to the list, the specified callback tries to read the size of the list. With a std::mutex we deadlock.

I see, and I assume it doesn't make sense to unlock the mutex before the callback?

[JDevlieghere]

LGTM

[bulbazord]

In D148380#4270090, @JDevlieghere wrote:

In D148380#4270085, @bulbazord wrote:

In D148380#4269876, @bulbazord wrote:

In D148380#4269862, @JDevlieghere wrote:

Does this actually have to be a recursive mutex?

Good point, I don't think it does. I'll update this.

Scratch that -- It unfortunately does need to be a recursive mutex. PathMappingList has a callback that may try to perform another operation on the list itself. This happens if you try to use target modules search-paths add -- We append to the list, the specified callback tries to read the size of the list. With a std::mutex we deadlock.

I see, and I assume it doesn't make sense to unlock the mutex before the callback?

I modified this patch locally to use std::mutex and release the mutex before invoking the callback. The test suite seems to like it, but it's still technically incorrect because of AppendUnique. Using std::mutex, we'd have to first lock the mutex, check to see if the given paths are already in m_pairs, and if they are not, unlock the mutex and call Append. If 2 things try to AppendUnique the same things at the same time and the stars align, they're going to Append the same thing twice (which is what happens without this patch anyway). We'd need to do some refactors if we wanted std::mutex to work correctly instead of std::recursive_mutex. That may be worth doing in the future, but I'm going to land this as-is for now.

[nickdesaulniers]

lldb/source/Target/PathMappingList.cpp

51

[412/1011] Building CXX object tools/lldb/sourc...CMakeFiles/lldbTarget.dir/PathMappingList.cpp.o
/android0/llvm-project/lldb/source/Target/PathMappingList.cpp:51:5: warning: 'scoped_lock' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
    std::scoped_lock locks(m_mutex, rhs.m_mutex);
    ^
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/mutex:692:11: note: add a deduction guide to suppress this warning
    class scoped_lock
          ^

72

/android0/llvm-project/lldb/source/Target/PathMappingList.cpp:72:3: warning: 'scoped_lock' may not intend to support class template argument deduction [-Wctad-maybe-unsupported]
  std::scoped_lock locks(m_mutex, rhs.m_mutex);
  ^
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/mutex:692:11: note: add a deduction guide to suppress this warning
    class scoped_lock
          ^

[bulbazord]

lldb/source/Target/PathMappingList.cpp

72

Fixed in c5fc7809e05940674424aaed7dd06c6be0639864

lldb/include/lldb/Target/PathMappingList.h

 //===-- PathMappingList.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_PATHMAPPINGLIST_H
 #define LLDB_TARGET_PATHMAPPINGLIST_H
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
 #include "llvm/Support/JSON.h"
 #include <map>
+#include <mutex>
 #include <optional>
 #include <vector>
 
 namespace lldb_private {
 
 class PathMappingList {
 public:
   typedef void (*ChangedCallback)(const PathMappingList &path_list,
public:
 
   void Clear(bool notify);
 
   // By default, dump all pairs.
   void Dump(Stream *s, int pair_index = -1);
 
   llvm::json::Value ToJSON();
 
-  bool IsEmpty() const { return m_pairs.empty(); }
-
-  size_t GetSize() const { return m_pairs.size(); }
+  bool IsEmpty() const {
+    std::lock_guard<std::recursive_mutex> lock(m_mutex);
+    return m_pairs.empty();
+  }
+
+  size_t GetSize() const {
+    std::lock_guard<std::recursive_mutex> lock(m_mutex);
+    return m_pairs.size();
+  }
 
   bool GetPathsAtIndex(uint32_t idx, ConstString &path,
                        ConstString &new_path) const;
 
   void Insert(llvm::StringRef path, llvm::StringRef replacement,
               uint32_t insert_idx, bool notify);
 
   bool Remove(size_t index, bool notify);
public:
   ///     The original source file path to try and remap.
   ///
   /// \return
   ///     The newly remapped filespec that is guaranteed to exist.
   std::optional<FileSpec> FindFile(const FileSpec &orig_spec) const;
 
   uint32_t FindIndexForPath(llvm::StringRef path) const;
 
-  uint32_t GetModificationID() const { return m_mod_id; }
+  uint32_t GetModificationID() const {
+    std::lock_guard<std::recursive_mutex> lock(m_mutex);
+    return m_mod_id;
+  }
 
 protected:
+  mutable std::recursive_mutex m_mutex;
   typedef std::pair<ConstString, ConstString> pair;
   typedef std::vector<pair> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
   iterator FindIteratorForPath(ConstString path);
 
   const_iterator FindIteratorForPath(ConstString path) const;

lldb/source/Target/PathMappingList.cpp

 PathMappingList::PathMappingList(ChangedCallback callback, void *callback_baton)
     : m_pairs(), m_callback(callback), m_callback_baton(callback_baton) {}
 
 PathMappingList::PathMappingList(const PathMappingList &rhs)
     : m_pairs(rhs.m_pairs) {}
 
 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;
     m_mod_id = rhs.m_mod_id;
   }
   return *this;
 }
 
 PathMappingList::~PathMappingList() = default;
 
 void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
                              bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 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();
     for (pos = rhs.m_pairs.begin(); pos != end; ++pos)
       m_pairs.push_back(*pos);
     if (notify && m_callback)
       m_callback(*this, m_callback_baton);
   }
 }
 
 bool PathMappingList::AppendUnique(llvm::StringRef path,
                                    llvm::StringRef replacement, bool notify) {
   auto normalized_path = NormalizePath(path);
   auto normalized_replacement = NormalizePath(replacement);
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   for (const auto &pair : m_pairs) {
     if (pair.first.GetStringRef().equals(normalized_path) &&
         pair.second.GetStringRef().equals(normalized_replacement))
       return false;
   }
   Append(path, replacement, notify);
   return true;
 }
 
 void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
                              uint32_t index, bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
     insert_iter = m_pairs.end();
   else
     insert_iter = m_pairs.begin() + index;
   m_pairs.emplace(insert_iter, pair(NormalizePath(path),
                                     NormalizePath(replacement)));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
                               uint32_t index, bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (index >= m_pairs.size())
     return false;
   ++m_mod_id;
   m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement));
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
   return true;
 }
 
 bool PathMappingList::Remove(size_t index, bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (index >= m_pairs.size())
     return false;
 
   ++m_mod_id;
   iterator iter = m_pairs.begin() + index;
   m_pairs.erase(iter);
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
   return true;
 }
 
 // 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<std::recursive_mutex> lock(m_mutex);
   unsigned int numPairs = m_pairs.size();
 
   if (pair_index < 0) {
     unsigned int index;
     for (index = 0; index < numPairs; ++index)
       s->Printf("[%d] \"%s\" -> \"%s\"\n", index,
                 m_pairs[index].first.GetCString(),
                 m_pairs[index].second.GetCString());
   } else {
     if (static_cast<unsigned int>(pair_index) < numPairs)
       s->Printf("%s -> %s", m_pairs[pair_index].first.GetCString(),
                 m_pairs[pair_index].second.GetCString());
   }
 }
 
 llvm::json::Value PathMappingList::ToJSON() {
   llvm::json::Array entries;
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   for (const auto &pair : m_pairs) {
     llvm::json::Array entry{pair.first.GetStringRef().str(),
                             pair.second.GetStringRef().str()};
     entries.emplace_back(std::move(entry));
   }
   return entries;
 }
 
 void PathMappingList::Clear(bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (!m_pairs.empty())
     ++m_mod_id;
   m_pairs.clear();
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 
 bool PathMappingList::RemapPath(ConstString path,
while (component != e &&
          llvm::sys::path::is_separator(*component->data(), style))
     ++component;
   for (; component != e; ++component)
     path.AppendPathComponent(*component);
 }
 
 std::optional<FileSpec> PathMappingList::RemapPath(llvm::StringRef mapping_path,
                                                    bool only_if_exists) const {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (m_pairs.empty() || mapping_path.empty())
     return {};
   LazyBool path_is_relative = eLazyBoolCalculate;
 
   for (const auto &it : m_pairs) {
     llvm::StringRef prefix = it.first.GetStringRef();
     // We create a copy of mapping_path because StringRef::consume_from
     // effectively modifies the instance itself.
std::optional<FileSpec> PathMappingList::RemapPath(llvm::StringRef mapping_path,
   }
   return {};
 }
 
 std::optional<llvm::StringRef>
 PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
   std::string path = file.GetPath();
   llvm::StringRef path_ref(path);
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   for (const auto &it : m_pairs) {
     llvm::StringRef removed_prefix = it.second.GetStringRef();
     if (!path_ref.consume_front(it.second.GetStringRef()))
       continue;
     auto orig_file = it.first.GetStringRef();
     auto orig_style = FileSpec::GuessPathStyle(orig_file).value_or(
         llvm::sys::path::Style::native);
     fixed.SetFile(orig_file, orig_style);
if (auto remapped = RemapPath(NormalizePath(orig_spec.GetPath()),
                                 /*only_if_exists=*/true))
     return remapped;
 
   return {};
 }
 
 bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path,
                               bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   uint32_t idx = FindIndexForPath(path);
   if (idx < m_pairs.size()) {
     ++m_mod_id;
     m_pairs[idx].second = ConstString(new_path);
     if (notify && m_callback)
       m_callback(*this, m_callback_baton);
     return true;
   }
   return false;
 }
 
 bool PathMappingList::Remove(ConstString path, bool notify) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   iterator pos = FindIteratorForPath(path);
   if (pos != m_pairs.end()) {
     ++m_mod_id;
     m_pairs.erase(pos);
     if (notify && m_callback)
       m_callback(*this, m_callback_baton);
     return true;
   }
   return false;
 }
 
 PathMappingList::const_iterator
 PathMappingList::FindIteratorForPath(ConstString path) const {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   const_iterator pos;
   const_iterator begin = m_pairs.begin();
   const_iterator end = m_pairs.end();
 
   for (pos = begin; pos != end; ++pos) {
     if (pos->first == path)
       break;
   }
   return pos;
 }
 
 PathMappingList::iterator
 PathMappingList::FindIteratorForPath(ConstString path) {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   iterator pos;
   iterator begin = m_pairs.begin();
   iterator end = m_pairs.end();
 
   for (pos = begin; pos != end; ++pos) {
     if (pos->first == path)
       break;
   }
   return pos;
 }
 
 bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path,
                                       ConstString &new_path) const {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (idx < m_pairs.size()) {
     path = m_pairs[idx].first;
     new_path = m_pairs[idx].second;
     return true;
   }
   return false;
 }
 
 uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const {
   const ConstString path = ConstString(NormalizePath(orig_path));
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   const_iterator pos;
   const_iterator begin = m_pairs.begin();
   const_iterator end = m_pairs.end();
 
   for (pos = begin; pos != end; ++pos) {
     if (pos->first == path)
       return std::distance(begin, pos);
   }
   return UINT32_MAX;
 }