[LLDB] Fix potential data race in Symtab initialization (#192753)

Claude pointed out to me that Symtab::FindFunctionSymbols doesn't lock
the mutex before checking m_name_indexes_computed and recomputing it. On
top of that all the initialization flags are bitfields, which makes any
unguarded concurrent accesses UB. Changing them to bools should no
longer be necessary after introducing a lock, but several of the public
methods trust that their caller holds the lock so I'm opting to remove
this footgun just in case.

rdar://174988238

Author: adrian-prantl

lldb/include/lldb/Symbol/Symtab.h

@@ -270,17 +270,20 @@ class Symtab {
   void InitNameIndexes();
   void InitAddressIndexes();
 
+  /// Provide thread safety for this symbol table.
+  mutable std::recursive_mutex m_mutex;
+
   ObjectFile *m_objfile;
   collection m_symbols;
   FileRangeToIndexMap m_file_addr_to_index;
 
   /// Maps function names to symbol indices (grouped by FunctionNameTypes)
   std::map<lldb::FunctionNameType, UniqueCStringMap<uint32_t>>
       m_name_to_symbol_indices;
-  mutable std::recursive_mutex
-      m_mutex; // Provide thread safety for this symbol table
-  bool m_file_addr_to_index_computed : 1, m_name_indexes_computed : 1,
-    m_loaded_from_cache : 1, m_saved_to_cache : 1;
+  bool m_file_addr_to_index_computed = false;
+  bool m_name_indexes_computed = false;
+  bool m_loaded_from_cache = false;
+  bool m_saved_to_cache = false;
 
 private:
   UniqueCStringMap<uint32_t> &

lldb/source/Symbol/Symtab.cpp

@@ -32,10 +32,7 @@ using namespace lldb;
 using namespace lldb_private;
 
 Symtab::Symtab(ObjectFile *objfile)
-    : m_objfile(objfile), m_symbols(), m_file_addr_to_index(*this),
-      m_name_to_symbol_indices(), m_mutex(),
-      m_file_addr_to_index_computed(false), m_name_indexes_computed(false),
-      m_loaded_from_cache(false), m_saved_to_cache(false) {
+    : m_objfile(objfile), m_file_addr_to_index(*this) {
   m_name_to_symbol_indices.emplace(std::make_pair(
       lldb::eFunctionNameTypeNone, UniqueCStringMap<uint32_t>()));
   m_name_to_symbol_indices.emplace(std::make_pair(
@@ -280,116 +277,116 @@ static bool lldb_skip_name(llvm::StringRef mangled,
 
 void Symtab::InitNameIndexes() {
   // Protected function, no need to lock mutex...
-  if (!m_name_indexes_computed) {
-    m_name_indexes_computed = true;
-    ElapsedTime elapsed(m_objfile->GetModule()->GetSymtabIndexTime());
-    LLDB_SCOPED_TIMER();
+  if (m_name_indexes_computed)
+    return;
 
-    // Collect all loaded language plugins.
-    std::vector<Language *> languages;
-    Language::ForEach([&languages](Language *l) {
-      languages.push_back(l);
-      return IterationAction::Continue;
-    });
+  m_name_indexes_computed = true;
+  ElapsedTime elapsed(m_objfile->GetModule()->GetSymtabIndexTime());
+  LLDB_SCOPED_TIMER();
 
-    auto &name_to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
-    auto &basename_to_index =
-        GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
-    auto &method_to_index =
-        GetNameToSymbolIndexMap(lldb::eFunctionNameTypeMethod);
-    auto &selector_to_index =
-        GetNameToSymbolIndexMap(lldb::eFunctionNameTypeSelector);
-    // Create the name index vector to be able to quickly search by name
-    const size_t num_symbols = m_symbols.size();
-    name_to_index.Reserve(num_symbols);
-
-    // The "const char *" in "class_contexts" and backlog::value_type::second
-    // must come from a ConstString::GetCString()
-    std::set<const char *> class_contexts;
-    std::vector<std::pair<NameToIndexMap::Entry, const char *>> backlog;
-    backlog.reserve(num_symbols / 2);
-
-    // Instantiation of the demangler is expensive, so better use a single one
-    // for all entries during batch processing.
-    RichManglingContext rmc;
-    for (uint32_t value = 0; value < num_symbols; ++value) {
-      Symbol *symbol = &m_symbols[value];
-
-      // Don't let trampolines get into the lookup by name map If we ever need
-      // the trampoline symbols to be searchable by name we can remove this and
-      // then possibly add a new bool to any of the Symtab functions that
-      // lookup symbols by name to indicate if they want trampolines. We also
-      // don't want any synthetic symbols with auto generated names in the
-      // name lookups.
-      if (symbol->IsTrampoline() || symbol->IsSyntheticWithAutoGeneratedName())
-        continue;
+  // Collect all loaded language plugins.
+  std::vector<Language *> languages;
+  Language::ForEach([&languages](Language *l) {
+    languages.push_back(l);
+    return IterationAction::Continue;
+  });
 
-      // If the symbol's name string matched a Mangled::ManglingScheme, it is
-      // stored in the mangled field.
-      Mangled &mangled = symbol->GetMangled();
-      if (ConstString name = mangled.GetMangledName()) {
-        name_to_index.Append(name, value);
+  auto &name_to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  auto &basename_to_index =
+      GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
+  auto &method_to_index =
+      GetNameToSymbolIndexMap(lldb::eFunctionNameTypeMethod);
+  auto &selector_to_index =
+      GetNameToSymbolIndexMap(lldb::eFunctionNameTypeSelector);
+  // Create the name index vector to be able to quickly search by name
+  const size_t num_symbols = m_symbols.size();
+  name_to_index.Reserve(num_symbols);
+
+  // The "const char *" in "class_contexts" and backlog::value_type::second
+  // must come from a ConstString::GetCString()
+  std::set<const char *> class_contexts;
+  std::vector<std::pair<NameToIndexMap::Entry, const char *>> backlog;
+  backlog.reserve(num_symbols / 2);
+
+  // Instantiation of the demangler is expensive, so better use a single one
+  // for all entries during batch processing.
+  RichManglingContext rmc;
+  for (size_t value = 0; value < num_symbols; ++value) {
+    Symbol *symbol = &m_symbols[value];
+
+    // Don't let trampolines get into the lookup by name map If we ever need
+    // the trampoline symbols to be searchable by name we can remove this and
+    // then possibly add a new bool to any of the Symtab functions that
+    // lookup symbols by name to indicate if they want trampolines. We also
+    // don't want any synthetic symbols with auto generated names in the
+    // name lookups.
+    if (symbol->IsTrampoline() || symbol->IsSyntheticWithAutoGeneratedName())
+      continue;
 
-        if (symbol->ContainsLinkerAnnotations()) {
-          // If the symbol has linker annotations, also add the version without
-          // the annotations.
-          ConstString stripped = ConstString(
-              m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
-          name_to_index.Append(stripped, value);
-        }
+    // If the symbol's name string matched a Mangled::ManglingScheme, it is
+    // stored in the mangled field.
+    Mangled &mangled = symbol->GetMangled();
+    if (ConstString name = mangled.GetMangledName()) {
+      name_to_index.Append(name, value);
+
+      if (symbol->ContainsLinkerAnnotations()) {
+        // If the symbol has linker annotations, also add the version without
+        // the annotations.
+        ConstString stripped = ConstString(
+            m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
+        name_to_index.Append(stripped, value);
+      }
 
-        const SymbolType type = symbol->GetType();
-        if (type == eSymbolTypeCode || type == eSymbolTypeResolver) {
-          if (mangled.GetRichManglingInfo(rmc, lldb_skip_name)) {
-            RegisterMangledNameEntry(value, class_contexts, backlog, rmc);
-            continue;
-          }
+      const SymbolType type = symbol->GetType();
+      if (type == eSymbolTypeCode || type == eSymbolTypeResolver) {
+        if (mangled.GetRichManglingInfo(rmc, lldb_skip_name)) {
+          RegisterMangledNameEntry(value, class_contexts, backlog, rmc);
+          continue;
         }
       }
+    }
 
-      // Symbol name strings that didn't match a Mangled::ManglingScheme, are
-      // stored in the demangled field.
-      if (ConstString name = mangled.GetDemangledName()) {
-        name_to_index.Append(name, value);
+    // Symbol name strings that didn't match a Mangled::ManglingScheme, are
+    // stored in the demangled field.
+    if (ConstString name = mangled.GetDemangledName()) {
+      name_to_index.Append(name, value);
 
-        if (symbol->ContainsLinkerAnnotations()) {
-          // If the symbol has linker annotations, also add the version without
-          // the annotations.
-          name = ConstString(
-              m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
-          name_to_index.Append(name, value);
-        }
+      if (symbol->ContainsLinkerAnnotations()) {
+        // If the symbol has linker annotations, also add the version without
+        // the annotations.
+        name = ConstString(
+            m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
+        name_to_index.Append(name, value);
+      }
 
-        // If the demangled name turns out to be an ObjC name, and is a category
-        // name, add the version without categories to the index too.
-        for (Language *lang : languages) {
-          for (auto variant : lang->GetMethodNameVariants(name)) {
-            if (variant.GetType() & lldb::eFunctionNameTypeSelector)
-              selector_to_index.Append(variant.GetName(), value);
-            else if (variant.GetType() & lldb::eFunctionNameTypeFull)
-              name_to_index.Append(variant.GetName(), value);
-            else if (variant.GetType() & lldb::eFunctionNameTypeMethod)
-              method_to_index.Append(variant.GetName(), value);
-            else if (variant.GetType() & lldb::eFunctionNameTypeBase)
-              basename_to_index.Append(variant.GetName(), value);
-          }
+      // If the demangled name turns out to be an ObjC name, and is a category
+      // name, add the version without categories to the index too.
+      for (Language *lang : languages) {
+        for (auto variant : lang->GetMethodNameVariants(name)) {
+          if (variant.GetType() & lldb::eFunctionNameTypeSelector)
+            selector_to_index.Append(variant.GetName(), value);
+          else if (variant.GetType() & lldb::eFunctionNameTypeFull)
+            name_to_index.Append(variant.GetName(), value);
+          else if (variant.GetType() & lldb::eFunctionNameTypeMethod)
+            method_to_index.Append(variant.GetName(), value);
+          else if (variant.GetType() & lldb::eFunctionNameTypeBase)
+            basename_to_index.Append(variant.GetName(), value);
         }
       }
     }
-
-    for (const auto &record : backlog) {
-      RegisterBacklogEntry(record.first, record.second, class_contexts);
-    }
-
-    name_to_index.Sort();
-    name_to_index.SizeToFit();
-    selector_to_index.Sort();
-    selector_to_index.SizeToFit();
-    basename_to_index.Sort();
-    basename_to_index.SizeToFit();
-    method_to_index.Sort();
-    method_to_index.SizeToFit();
   }
+
+  for (const auto &record : backlog)
+    RegisterBacklogEntry(record.first, record.second, class_contexts);
+
+  name_to_index.Sort();
+  name_to_index.SizeToFit();
+  selector_to_index.Sort();
+  selector_to_index.SizeToFit();
+  basename_to_index.Sort();
+  basename_to_index.SizeToFit();
+  method_to_index.Sort();
+  method_to_index.SizeToFit();
 }
 
 void Symtab::RegisterMangledNameEntry(
@@ -684,8 +681,7 @@ uint32_t Symtab::AppendSymbolIndexesWithName(ConstString symbol_name,
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
   if (symbol_name) {
-    if (!m_name_indexes_computed)
-      InitNameIndexes();
+    InitNameIndexes();
 
     return GetNameIndexes(symbol_name, indexes);
   }
@@ -701,8 +697,7 @@ uint32_t Symtab::AppendSymbolIndexesWithName(ConstString symbol_name,
   LLDB_SCOPED_TIMER();
   if (symbol_name) {
     const size_t old_size = indexes.size();
-    if (!m_name_indexes_computed)
-      InitNameIndexes();
+    InitNameIndexes();
 
     std::vector<uint32_t> all_name_indexes;
     const size_t name_match_count =
@@ -823,8 +818,7 @@ Symtab::FindAllSymbolsWithNameAndType(ConstString name,
 
   // Initialize all of the lookup by name indexes before converting NAME to a
   // uniqued string NAME_STR below.
-  if (!m_name_indexes_computed)
-    InitNameIndexes();
+  InitNameIndexes();
 
   if (name) {
     // The string table did have a string that matched, but we need to check
@@ -841,8 +835,7 @@ void Symtab::FindAllSymbolsWithNameAndType(
   LLDB_SCOPED_TIMER();
   // Initialize all of the lookup by name indexes before converting NAME to a
   // uniqued string NAME_STR below.
-  if (!m_name_indexes_computed)
-    InitNameIndexes();
+  InitNameIndexes();
 
   if (name) {
     // The string table did have a string that matched, but we need to check
@@ -870,8 +863,7 @@ Symbol *Symtab::FindFirstSymbolWithNameAndType(ConstString name,
                                                Visibility symbol_visibility) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   LLDB_SCOPED_TIMER();
-  if (!m_name_indexes_computed)
-    InitNameIndexes();
+  InitNameIndexes();
 
   if (name) {
     std::vector<uint32_t> matching_indexes;
@@ -1126,8 +1118,8 @@ void Symtab::FindFunctionSymbols(ConstString name, uint32_t name_type_mask,
     }
   }
 
-  if (!m_name_indexes_computed)
-    InitNameIndexes();
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  InitNameIndexes();
 
   for (lldb::FunctionNameType type :
        {lldb::eFunctionNameTypeBase, lldb::eFunctionNameTypeMethod,
@@ -1178,7 +1170,9 @@ void Symtab::SaveToCache() {
   DataFileCache *cache = Module::GetIndexCache();
   if (!cache)
     return; // Caching is not enabled.
-  InitNameIndexes(); // Init the name indexes so we can cache them as well.
+
+  // Init the name indexes so we can cache them as well.
+  InitNameIndexes();
   const auto byte_order = endian::InlHostByteOrder();
   DataEncoder file(byte_order, /*addr_size=*/8);
   // Encode will return false if the symbol table's object file doesn't have