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
This commit is contained in:
Frederic Riss 2019-04-23 20:17:04 +00:00
parent 5c3117b0a9
commit acbf0058e9
13 changed files with 58 additions and 32 deletions

View File

@ -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;
};

View File

@ -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;

View File

@ -43,7 +43,7 @@ public:
///
const RegularExpression *GetSymbolsToAvoidRegexp();
FileSpecList &GetLibrariesToAvoid() const;
FileSpecList GetLibrariesToAvoid() const;
bool GetTraceEnabledState() const;

View File

@ -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(),

View File

@ -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));
}

View File

@ -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);
}
}

View File

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

View File

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

View File

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

View File

@ -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

View File

@ -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);
return option_value->GetCurrentValue();
option_value->AppendCurrentValue(dir);
}
FileSpecList &TargetProperties::GetDebugFileSearchPaths() {
const uint32_t idx = ePropertyDebugFileSearchPaths;
OptionValueFileSpecList *option_value =
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::GetClangModuleSearchPaths() {
FileSpecList TargetProperties::GetDebugFileSearchPaths() {
const uint32_t idx = ePropertyDebugFileSearchPaths;
const OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
false, idx);
assert(option_value);
return option_value->GetCurrentValue();
}
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);

View File

@ -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.

View File

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