Lock accesses to OptionValueFileSpecList objects

Before a Debugger gets a Target, target settings are routed to a global set
of settings. Even without this, some part of the LLDB which exist independently
of the Debugger object (the Module cache, the Symbol vendors, ...) access
directly the global default store for those settings.

Of course, if you modify one of those global settings while they are being read,
bad things happen. We see this quite a bit with FileSpecList settings. In
particular, we see many cases where one debug session changes
target.exec-search-paths while another session starts up and it crashes when
one of those accesses invalid FileSpecs.

This patch addresses the specific FileSpecList issue by adding locking to
OptionValueFileSpecList and never returning by reference.

Reviewers: clayborg

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D60468

llvm-svn: 359028

diff --git a/lldb/include/lldb/Interpreter/OptionValueFileSpecList.h b/lldb/include/lldb/Interpreter/OptionValueFileSpecList.h
index 67e562e..ed79002 100644 (file)
--- a/lldb/include/lldb/Interpreter/OptionValueFileSpecList.h
+++ b/lldb/include/lldb/Interpreter/OptionValueFileSpecList.h
@@ -9,6 +9,8 @@
 #ifndef liblldb_OptionValueFileSpecList_h_
 #define liblldb_OptionValueFileSpecList_h_
 
+#include <mutex>
+
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Interpreter/OptionValue.h"
 
@@ -49,13 +51,23 @@ public:
 
   // Subclass specific functions
 
-  FileSpecList &GetCurrentValue() { return m_current_value; }
+  FileSpecList GetCurrentValue() const {
+    std::lock_guard<std::mutex> lock(m_mutex);
+    return m_current_value;
+  }
 
-  const FileSpecList &GetCurrentValue() const { return m_current_value; }
+  void SetCurrentValue(const FileSpecList &value) {
+    std::lock_guard<std::mutex> lock(m_mutex);
+    m_current_value = value;
+  }
 
-  void SetCurrentValue(const FileSpecList &value) { m_current_value = value; }
+  void AppendCurrentValue(const FileSpec &value) {
+    std::lock_guard<std::mutex> lock(m_mutex);
+    m_current_value.Append(value);
+  }
 
 protected:
+  mutable std::mutex m_mutex;
   FileSpecList m_current_value;
 };
 

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index d51a778..ca88763 100644 (file)
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -119,11 +119,13 @@ public:
 
   PathMappingList &GetSourcePathMap() const;
 
-  FileSpecList &GetExecutableSearchPaths();
+  FileSpecList GetExecutableSearchPaths();
 
-  FileSpecList &GetDebugFileSearchPaths();
+  void AppendExecutableSearchPaths(const FileSpec&);
 
-  FileSpecList &GetClangModuleSearchPaths();
+  FileSpecList GetDebugFileSearchPaths();
+
+  FileSpecList GetClangModuleSearchPaths();
 
   bool GetEnableAutoImportClangModules() const;
 

diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index bc99fe3..7aeaece 100644 (file)
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -43,7 +43,7 @@ public:
   ///
   const RegularExpression *GetSymbolsToAvoidRegexp();
 
-  FileSpecList &GetLibrariesToAvoid() const;
+  FileSpecList GetLibrariesToAvoid() const;
 
   bool GetTraceEnabledState() const;
 

diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index 1343741..bc59557 100644 (file)
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -410,7 +410,7 @@ protected:
             }
             FileSpec core_file_dir;
             core_file_dir.GetDirectory() = core_file.GetDirectory();
-            target_sp->GetExecutableSearchPaths().Append(core_file_dir);
+            target_sp->AppendExecutableSearchPaths(core_file_dir);
 
             ProcessSP process_sp(target_sp->CreateProcess(
                 m_interpreter.GetDebugger().GetListener(), llvm::StringRef(),

diff --git a/lldb/source/Interpreter/OptionValueFileSpecLIst.cpp b/lldb/source/Interpreter/OptionValueFileSpecLIst.cpp
index 0e91bc7..778d8e5 100644 (file)
--- a/lldb/source/Interpreter/OptionValueFileSpecLIst.cpp
+++ b/lldb/source/Interpreter/OptionValueFileSpecLIst.cpp
@@ -163,5 +163,6 @@ Status OptionValueFileSpecList::SetValueFromString(llvm::StringRef value,
 }
 
 lldb::OptionValueSP OptionValueFileSpecList::DeepCopy() const {
-  return OptionValueSP(new OptionValueFileSpecList(*this));
+  std::lock_guard<std::mutex> lock(m_mutex);
+  return OptionValueSP(new OptionValueFileSpecList(m_current_value));
 }

diff --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index b377c23..ab1cd93 100644 (file)
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -804,10 +804,11 @@ bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
         if (platform_name == g_platform_name) {
           ModuleSpec kext_bundle_module_spec(module_spec);
           FileSpec kext_filespec(m_name.c_str());
+         FileSpecList search_paths = target.GetExecutableSearchPaths();
           kext_bundle_module_spec.GetFileSpec() = kext_filespec;
           platform_sp->GetSharedModule(
               kext_bundle_module_spec, process, m_module_sp,
-              &target.GetExecutableSearchPaths(), NULL, NULL);
+              &search_paths, NULL, NULL);
         }
       }
 

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
index 993843f..941306f 100644 (file)
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -608,7 +608,7 @@ ClangModulesDeclVendor::Create(Target &target) {
     compiler_invocation_arguments.push_back(module_cache_argument);
   }
 
-  FileSpecList &module_search_paths = target.GetClangModuleSearchPaths();
+  FileSpecList module_search_paths = target.GetClangModuleSearchPaths();
 
   for (size_t spi = 0, spe = module_search_paths.GetSize(); spi < spe; ++spi) {
     const FileSpec &search_path = module_search_paths.GetFileSpecAtIndex(spi);

diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
index f67ddda..cb453b5 100644 (file)
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -208,9 +208,9 @@ public:
         NULL, idx, g_properties[idx].default_uint_value != 0);
   }
 
-  FileSpecList &GetKextDirectories() const {
+  FileSpecList GetKextDirectories() const {
     const uint32_t idx = ePropertyKextDirectories;
-    OptionValueFileSpecList *option_value =
+    const OptionValueFileSpecList *option_value =
         m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(
             NULL, false, idx);
     assert(option_value);

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index f9b3143..b9ad32c 100644 (file)
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -136,8 +136,8 @@ public:
     m_collection_sp->Initialize(g_properties);
   }
 
-  FileSpecList &GetSymLinkPaths() {
-    OptionValueFileSpecList *option_value =
+  FileSpecList GetSymLinkPaths() {
+    const OptionValueFileSpecList *option_value =
         m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(
             nullptr, true, ePropertySymLinkPaths);
     assert(option_value);
@@ -159,7 +159,7 @@ static const SymbolFileDWARFPropertiesSP &GetGlobalPluginProperties() {
 
 } // anonymous namespace end
 
-const FileSpecList &SymbolFileDWARF::GetSymlinkPaths() {
+FileSpecList SymbolFileDWARF::GetSymlinkPaths() {
   return GetGlobalPluginProperties()->GetSymLinkPaths();
 }
 

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 42ae231..2100880 100644 (file)
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -80,7 +80,7 @@ public:
   static lldb_private::SymbolFile *
   CreateInstance(lldb_private::ObjectFile *obj_file);
 
-  static const lldb_private::FileSpecList &GetSymlinkPaths();
+  static lldb_private::FileSpecList GetSymlinkPaths();
 
   // Constructors and Destructors
 

diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index af88e47..b173a68 100644 (file)
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1574,8 +1574,9 @@ bool Target::SetArchitecture(const ArchSpec &arch_spec, bool set_platform) {
                   arch_spec.GetArchitectureName(),
                   arch_spec.GetTriple().getTriple().c_str());
     ModuleSpec module_spec(executable_sp->GetFileSpec(), other);
+    FileSpecList search_paths = GetExecutableSearchPaths();
     Status error = ModuleList::GetSharedModule(module_spec, executable_sp,
-                                               &GetExecutableSearchPaths(),
+                                               &search_paths,
                                                nullptr, nullptr);
 
     if (!error.Fail() && executable_sp) {
@@ -2015,7 +2016,7 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
     ModuleSP old_module_sp; // This will get filled in if we have a new version
                             // of the library
     bool did_create_module = false;
-
+    FileSpecList search_paths = GetExecutableSearchPaths();
     // If there are image search path entries, try to use them first to acquire
     // a suitable image.
     if (m_image_search_paths.GetSize()) {
@@ -2026,7 +2027,7 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
         transformed_spec.GetFileSpec().GetFilename() =
             module_spec.GetFileSpec().GetFilename();
         error = ModuleList::GetSharedModule(transformed_spec, module_sp,
-                                            &GetExecutableSearchPaths(),
+                                            &search_paths,
                                             &old_module_sp, &did_create_module);
       }
     }
@@ -2043,7 +2044,7 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
       if (module_spec.GetUUID().IsValid()) {
         // We have a UUID, it is OK to check the global module list...
         error = ModuleList::GetSharedModule(module_spec, module_sp,
-                                            &GetExecutableSearchPaths(),
+                                            &search_paths,
                                             &old_module_sp, &did_create_module);
       }
 
@@ -2053,7 +2054,7 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
         if (m_platform_sp) {
           error = m_platform_sp->GetSharedModule(
               module_spec, m_process_sp.get(), module_sp,
-              &GetExecutableSearchPaths(), &old_module_sp, &did_create_module);
+              &search_paths, &old_module_sp, &did_create_module);
         } else {
           error.SetErrorString("no platform is currently set");
         }
@@ -3865,27 +3866,36 @@ PathMappingList &TargetProperties::GetSourcePathMap() const {
   return option_value->GetCurrentValue();
 }
 
-FileSpecList &TargetProperties::GetExecutableSearchPaths() {
+void TargetProperties::AppendExecutableSearchPaths(const FileSpec& dir) {
   const uint32_t idx = ePropertyExecutableSearchPaths;
   OptionValueFileSpecList *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
                                                                    false, idx);
   assert(option_value);
+  option_value->AppendCurrentValue(dir);
+}
+
+FileSpecList TargetProperties::GetExecutableSearchPaths() {
+  const uint32_t idx = ePropertyExecutableSearchPaths;
+  const OptionValueFileSpecList *option_value =
+      m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
+                                                                   false, idx);
+  assert(option_value);
   return option_value->GetCurrentValue();
 }
 
-FileSpecList &TargetProperties::GetDebugFileSearchPaths() {
+FileSpecList TargetProperties::GetDebugFileSearchPaths() {
   const uint32_t idx = ePropertyDebugFileSearchPaths;
-  OptionValueFileSpecList *option_value =
+  const OptionValueFileSpecList *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
                                                                    false, idx);
   assert(option_value);
   return option_value->GetCurrentValue();
 }
 
-FileSpecList &TargetProperties::GetClangModuleSearchPaths() {
+FileSpecList TargetProperties::GetClangModuleSearchPaths() {
   const uint32_t idx = ePropertyClangModuleSearchPaths;
-  OptionValueFileSpecList *option_value =
+  const OptionValueFileSpecList *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
                                                                    false, idx);
   assert(option_value);

diff --git a/lldb/source/Target/TargetList.cpp b/lldb/source/Target/TargetList.cpp
index 8c77876..7c7a36e 100644 (file)
--- a/lldb/source/Target/TargetList.cpp
+++ b/lldb/source/Target/TargetList.cpp
@@ -422,7 +422,7 @@ Status TargetList::CreateTargetInternal(Debugger &debugger,
     if (file.GetDirectory()) {
       FileSpec file_dir;
       file_dir.GetDirectory() = file.GetDirectory();
-      target_sp->GetExecutableSearchPaths().Append(file_dir);
+      target_sp->AppendExecutableSearchPaths(file_dir);
     }
 
     // Don't put the dummy target in the target list, it's held separately.

diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index e08e3bb..d100b8b 100644 (file)
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -138,9 +138,9 @@ const RegularExpression *ThreadProperties::GetSymbolsToAvoidRegexp() {
   return m_collection_sp->GetPropertyAtIndexAsOptionValueRegex(nullptr, idx);
 }
 
-FileSpecList &ThreadProperties::GetLibrariesToAvoid() const {
+FileSpecList ThreadProperties::GetLibrariesToAvoid() const {
   const uint32_t idx = ePropertyStepAvoidLibraries;
-  OptionValueFileSpecList *option_value =
+  const OptionValueFileSpecList *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
                                                                    false, idx);
   assert(option_value);