[ASan/Win] Unify handling of loaded modules between POSIX and Windows

[timurrrr]

Summary

This is somewhat based on D8513 by Kuba Brecka.

[timurrrr]

Fixed hangs on Linux

[timurrrr]

Addressed the remaining TODO item.

Alexey, can you please take a look?

[samsonov]

Cool, I believe it's a step in right direction. Feel free to submit after fixing a few comments below.

sanitizer_linux_libcdep.cc

439–440

Nit: we don't usually use non-const references. Can you change it to pointer?

sanitizer_procmaps_common.cc

145–147

Ditto

sanitizer_procmaps_mac.cc

174

nullptr

sanitizer_symbolizer.h

143

You don't need this function - it just calls ::GetListOfModules() in both implementations. Please delete it.

sanitizer_win.cc

393

See comment about non-const reference above.

[timurrrr]

r234150, thanks for the review!
Please take a look at the FIXME I've added to lib/sanitizer_common/sanitizer_common.h

sanitizer_linux_libcdep.cc

439–440

Umm, yeah. Fixed

sanitizer_procmaps_common.cc

145–147

Done

sanitizer_procmaps_mac.cc

174

I haven't changed that code really, but fixed anyways.

sanitizer_symbolizer.h

143

This is a little tricky, as it fails to link as GetListOfModules is in libcdep nowadays...
I've added a FIXME -- can you please take a look? I don't really understand the logic behind libcdep yet.

sanitizer_common.h

uptr InternalBinarySearch(const Container &v, uptr first, uptr last,
   }
   return not_found;
 }
 
 // Represents a binary loaded into virtual memory (e.g. this can be an
 // executable or a shared object).
 class LoadedModule {
  public:
-  LoadedModule() : full_name_(nullptr), base_address_(0) {}
-  LoadedModule(const char *module_name, uptr base_address);
+  LoadedModule() : full_name_(nullptr), base_address_(0) { ranges_.clear(); }
+  void set(const char *module_name, uptr base_address);
   void clear();
   void addAddressRange(uptr beg, uptr end, bool executable);
   bool containsAddress(uptr address) const;
 
   const char *full_name() const { return full_name_; }
   uptr base_address() const { return base_address_; }
 
   struct AddressRange {

sanitizer_common.cc

if (!common_flags()->print_summary)
     return;
   InternalScopedString buff(kMaxSummaryLength);
   buff.append("%s ", error_type);
   RenderFrame(&buff, "%L %F", 0, info, common_flags()->strip_path_prefix);
   ReportErrorSummary(buff.data());
 }
 #endif
 
-LoadedModule::LoadedModule(const char *module_name, uptr base_address) {
+void LoadedModule::set(const char *module_name, uptr base_address) {
+  clear();
   full_name_ = internal_strdup(module_name);
   base_address_ = base_address;
-  ranges_.clear();
 }
 
 void LoadedModule::clear() {
   InternalFree(full_name_);
+  full_name_ = nullptr;
   while (!ranges_.empty()) {
     AddressRange *r = ranges_.front();
     ranges_.pop_front();
     InternalFree(r);
   }
 }
 
 void LoadedModule::addAddressRange(uptr beg, uptr end, bool executable) {

sanitizer_linux_libcdep.cc

if (data->first) {
     ReadBinaryName(module_name.data(), module_name.size());
   } else if (info->dlpi_name) {
     module_name.append("%s", info->dlpi_name);
   }
   if (module_name[0] == '\0')
     return 0;
   if (data->filter && !data->filter(module_name.data()))
     return 0;
-  void *mem = &data->modules[data->current_n];
-  LoadedModule *cur_module = new(mem) LoadedModule(module_name.data(),
-                                                   info->dlpi_addr);
+  LoadedModule &cur_module = data->modules[data->current_n];
+  cur_module.set(module_name.data(), info->dlpi_addr);
   data->current_n++;
   for (int i = 0; i < info->dlpi_phnum; i++) {
     const Elf_Phdr *phdr = &info->dlpi_phdr[i];
     if (phdr->p_type == PT_LOAD) {
       uptr cur_beg = info->dlpi_addr + phdr->p_vaddr;
       uptr cur_end = cur_beg + phdr->p_memsz;
       bool executable = phdr->p_flags & PF_X;
-      cur_module->addAddressRange(cur_beg, cur_end, executable);
+      cur_module.addAddressRange(cur_beg, cur_end, executable);
     }
   }
   return 0;
 }
 
 uptr GetListOfModules(LoadedModule *modules, uptr max_modules,
                       string_predicate_t filter) {
   CHECK(modules);

sanitizer_procmaps_common.cc

for (uptr i = 0; n_modules < max_modules &&
                        Next(&cur_beg, &cur_end, &cur_offset, module_name.data(),
                             module_name.size(), &prot);
        i++) {
     const char *cur_name = module_name.data();
     if (cur_name[0] == '\0')
       continue;
     if (filter && !filter(cur_name))
       continue;
-    void *mem = &modules[n_modules];
     // Don't subtract 'cur_beg' from the first entry:
     // * If a binary is compiled w/o -pie, then the first entry in
     //   process maps is likely the binary itself (all dynamic libs
     //   are mapped higher in address space). For such a binary,
     //   instruction offset in binary coincides with the actual
     //   instruction address in virtual memory (as code section
     //   is mapped to a fixed memory range).
     // * If a binary is compiled with -pie, all the modules are
     //   mapped high at address space (in particular, higher than
     //   shadow memory of the tool), so the module can't be the
     //   first entry.
     uptr base_address = (i ? cur_beg : 0) - cur_offset;
-    LoadedModule *cur_module = new(mem) LoadedModule(cur_name, base_address);
-    cur_module->addAddressRange(cur_beg, cur_end, prot & kProtectionExecute);
+    LoadedModule &cur_module = modules[n_modules];
+    cur_module.set(cur_name, base_address);
+    cur_module.addAddressRange(cur_beg, cur_end, prot & kProtectionExecute);
     n_modules++;
   }
   return n_modules;
 }
 
 void GetMemoryProfile(fill_profile_f cb, uptr *stats, uptr stats_size) {
   char *smaps = 0;
   uptr smaps_cap = 0;

sanitizer_procmaps_mac.cc

for (uptr i = 0; n_modules < max_modules &&
                        Next(&cur_beg, &cur_end, 0, module_name.data(),
                             module_name.size(), &prot);
        i++) {
     const char *cur_name = module_name.data();
     if (cur_name[0] == '\0')
       continue;
     if (filter && !filter(cur_name))
       continue;
     LoadedModule *cur_module = 0;
     if (n_modules > 0 &&
         0 == internal_strcmp(cur_name, modules[n_modules - 1].full_name())) {
       cur_module = &modules[n_modules - 1];
     } else {
-      void *mem = &modules[n_modules];
-      cur_module = new(mem) LoadedModule(cur_name, cur_beg);
+      cur_module = &modules[n_modules];
+      cur_module->set(cur_name, cur_beg);
       n_modules++;
     }
     cur_module->addAddressRange(cur_beg, cur_end, prot & kProtectionExecute);
   }
   return n_modules;
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MAC

sanitizer_symbolizer.h

private:
     const char *last_match_;
 
     BlockingMutex *mu_;
   } module_names_;
 
   /// Platform-specific function for creating a Symbolizer object.
   static Symbolizer *PlatformInit();
 
-  virtual bool PlatformFindModuleNameAndOffsetForAddress(
-      uptr address, const char **module_name, uptr *module_offset) {
+  bool FindModuleNameAndOffsetForAddress(uptr address, const char **module_name,
+                                         uptr *module_offset);
+  LoadedModule *FindModuleForAddress(uptr address);
+  virtual uptr PlatformGetListOfModules(LoadedModule *modules,
+                                        uptr max_modules) {
     UNIMPLEMENTED();
   }
+  // 16K loaded modules should be enough for everyone.
+  static const uptr kMaxNumberOfModuleContexts = 1 << 14;
+  LoadedModule modules_[kMaxNumberOfModuleContexts];
+  uptr n_modules_;
+  // If stale, need to reload the modules before looking up addresses.
+  bool modules_fresh_;
+
   // Platform-specific default demangler, must not return nullptr.
   virtual const char *PlatformDemangle(const char *name) { UNIMPLEMENTED(); }
   virtual void PlatformPrepareForSandboxing() { UNIMPLEMENTED(); }
 
   static Symbolizer *symbolizer_;
   static StaticSpinMutex init_mu_;
 
   // Mutex locked from public methods of |Symbolizer|, so that the internals

sanitizer_symbolizer.cc

if (!internal_strcmp(storage_[i], str)) {
       return last_match_;
     }
   }
   last_match_ = internal_strdup(str);
   storage_.push_back(last_match_);
   return last_match_;
 }
 
+bool Symbolizer::FindModuleNameAndOffsetForAddress(uptr address,
+                                                   const char **module_name,
+                                                   uptr *module_offset) {
+  LoadedModule *module = FindModuleForAddress(address);
+  if (module == 0)
+    return false;
+  *module_name = module->full_name();
+  *module_offset = address - module->base_address();
+  return true;
+}
+
+LoadedModule *Symbolizer::FindModuleForAddress(uptr address) {
+  bool modules_were_reloaded = false;
+  if (!modules_fresh_) {
+    for (uptr i = 0; i < n_modules_; i++)
+      modules_[i].clear();
+    n_modules_ = PlatformGetListOfModules(modules_, kMaxNumberOfModuleContexts);
+    CHECK_GT(n_modules_, 0);
+    CHECK_LT(n_modules_, kMaxNumberOfModuleContexts);
+    modules_fresh_ = true;
+    modules_were_reloaded = true;
+  }
+  for (uptr i = 0; i < n_modules_; i++) {
+    if (modules_[i].containsAddress(address)) {
+      return &modules_[i];
+    }
+  }
+  // Reload the modules and look up again, if we haven't tried it yet.
+  if (!modules_were_reloaded) {
+    // FIXME: set modules_fresh_ from dlopen()/dlclose() interceptors.
+    // It's too aggressive to reload the list of modules each time we fail
+    // to find a module for a given address.
+    modules_fresh_ = false;
+    return FindModuleForAddress(address);
+  }
+  return 0;
+}
+
 Symbolizer::Symbolizer(IntrusiveList<SymbolizerTool> tools)
-    : module_names_(&mu_), tools_(tools), start_hook_(0), end_hook_(0) {}
+    : module_names_(&mu_), n_modules_(0), modules_fresh_(false), tools_(tools),
+      start_hook_(0), end_hook_(0) {}
 
 Symbolizer::SymbolizerScope::SymbolizerScope(const Symbolizer *sym)
     : sym_(sym) {
   if (sym_->start_hook_)
     sym_->start_hook_();
 }
 
 Symbolizer::SymbolizerScope::~SymbolizerScope() {
   if (sym_->end_hook_)
     sym_->end_hook_();
 }
 
 SymbolizedStack *Symbolizer::SymbolizePC(uptr addr) {
   BlockingMutexLock l(&mu_);
   const char *module_name;
   uptr module_offset;
   SymbolizedStack *res = SymbolizedStack::New(addr);
-  if (!PlatformFindModuleNameAndOffsetForAddress(addr, &module_name,
-                                                 &module_offset))
+  if (!FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
     return res;
   // Always fill data about module name and offset.
   res->info.FillModuleInfo(module_name, module_offset);
   for (auto iter = Iterator(&tools_); iter.hasNext();) {
     auto *tool = iter.next();
     SymbolizerScope sym_scope(this);
     if (tool->SymbolizePC(addr, res)) {
       return res;
     }
   }
   return res;
 }
 
 bool Symbolizer::SymbolizeData(uptr addr, DataInfo *info) {
   BlockingMutexLock l(&mu_);
   const char *module_name;
   uptr module_offset;
-  if (!PlatformFindModuleNameAndOffsetForAddress(addr, &module_name,
-                                                 &module_offset))
+  if (!FindModuleNameAndOffsetForAddress(addr, &module_name, &module_offset))
     return false;
   info->Clear();
   info->module = internal_strdup(module_name);
   info->module_offset = module_offset;
   for (auto iter = Iterator(&tools_); iter.hasNext();) {
     auto *tool = iter.next();
     SymbolizerScope sym_scope(this);
     if (tool->SymbolizeData(addr, info)) {
       return true;
     }
   }
   return true;
 }
 
 bool Symbolizer::GetModuleNameAndOffsetForPC(uptr pc, const char **module_name,
                                              uptr *module_address) {
   BlockingMutexLock l(&mu_);
   const char *internal_module_name = nullptr;
-  if (!PlatformFindModuleNameAndOffsetForAddress(pc, &internal_module_name,
-                                                 module_address))
+  if (!FindModuleNameAndOffsetForAddress(pc, &internal_module_name,
     return false;
 
   if (module_name)
     *module_name = module_names_.GetOwnedCopy(internal_module_name);
   return true;
 }
 
 void Symbolizer::Flush() {

sanitizer_symbolizer_posix_libcdep.cc

public:
   static InternalSymbolizer *get(LowLevelAllocator *alloc) { return 0; }
 };
 
 #endif  // SANITIZER_SUPPORTS_WEAK_HOOKS
 
 class POSIXSymbolizer : public Symbolizer {
  public:
   explicit POSIXSymbolizer(IntrusiveList<SymbolizerTool> tools)
-      : Symbolizer(tools), n_modules_(0), modules_fresh_(false) {}
+      : Symbolizer(tools) {}
 
  private:
+  uptr PlatformGetListOfModules(LoadedModule *modules,
+                                uptr max_modules) override {
+    return ::GetListOfModules(modules, max_modules, /* filter */ nullptr);
+  }
+
   const char *PlatformDemangle(const char *name) override {
     return DemangleCXXABI(name);
   }
 
   void PlatformPrepareForSandboxing() override {
 #if SANITIZER_LINUX && !SANITIZER_ANDROID
     // Cache /proc/self/exe on Linux.
     CacheBinaryName();
 #endif
   }
-
-  LoadedModule *FindModuleForAddress(uptr address) {
-    bool modules_were_reloaded = false;
-    if (!modules_fresh_) {
-      for (uptr i = 0; i < n_modules_; i++)
-        modules_[i].clear();
-      n_modules_ = GetListOfModules(modules_, kMaxNumberOfModuleContexts,
-                                    /* filter */ 0);
-      CHECK_GT(n_modules_, 0);
-      CHECK_LT(n_modules_, kMaxNumberOfModuleContexts);
-      modules_fresh_ = true;
-      modules_were_reloaded = true;
-    }
-    for (uptr i = 0; i < n_modules_; i++) {
-      if (modules_[i].containsAddress(address)) {
-        return &modules_[i];
-      }
-    }
-    // Reload the modules and look up again, if we haven't tried it yet.
-    if (!modules_were_reloaded) {
-      // FIXME: set modules_fresh_ from dlopen()/dlclose() interceptors.
-      // It's too aggressive to reload the list of modules each time we fail
-      // to find a module for a given address.
-      modules_fresh_ = false;
-      return FindModuleForAddress(address);
-    }
-    return 0;
-  }
-
-  bool PlatformFindModuleNameAndOffsetForAddress(uptr address,
-                                                 const char **module_name,
-                                                 uptr *module_offset) override {
-    LoadedModule *module = FindModuleForAddress(address);
-    if (module == 0)
-      return false;
-    *module_name = module->full_name();
-    *module_offset = address - module->base_address();
-    return true;
-  }
-
-  // 16K loaded modules should be enough for everyone.
-  static const uptr kMaxNumberOfModuleContexts = 1 << 14;
-  LoadedModule modules_[kMaxNumberOfModuleContexts];
-  uptr n_modules_;
-  // If stale, need to reload the modules before looking up addresses.
-  bool modules_fresh_;
 };
 
 static SymbolizerTool *ChooseExternalSymbolizer(LowLevelAllocator *allocator) {
   const char *path = common_flags()->external_symbolizer_path;
   const char *binary_name = path ? StripModuleName(path) : "";
   if (path && path[0] == '\0') {
     VReport(2, "External symbolizer is explicitly disabled.\n");
     return nullptr;

sanitizer_symbolizer_win.cc

const char *WinSymbolizerTool::Demangle(const char *name) {
   if (name[0] == '\01' &&
       UnDecorateSymbolName(name + 1, demangle_buffer, sizeof(demangle_buffer),
                            UNDNAME_NAME_ONLY))
     return demangle_buffer;
   else
     return name;
 }
 
-bool FindModuleNameAndOffsetForAddress(uptr addr, const char **module_name,
-                                       uptr *module_offset) {
-  InitializeDbgHelpIfNeeded();
-
-  IMAGEHLP_MODULE64 mod_info;
-  internal_memset(&mod_info, 0, sizeof(mod_info));
-  mod_info.SizeOfStruct = sizeof(mod_info);
-  if (SymGetModuleInfo64(GetCurrentProcess(), addr, &mod_info)) {
-    *module_name = mod_info.ImageName;
-    *module_offset = addr - (uptr)mod_info.BaseOfImage;
-    return true;
-  }
-  return false;
-}
-
 // TODO(kuba.brecka): To be merged with POSIXSymbolizer.
 class WinSymbolizer : public Symbolizer {
  public:
   explicit WinSymbolizer(IntrusiveList<SymbolizerTool> tools)
       : Symbolizer(tools) {}
 
  private:
-  bool PlatformFindModuleNameAndOffsetForAddress(
-      uptr addr, const char **module_name, uptr *module_offset) override {
-    return ::FindModuleNameAndOffsetForAddress(addr, module_name,
-                                               module_offset);
+  uptr PlatformGetListOfModules(LoadedModule *modules,
+                                uptr max_modules) override {
+    return ::GetListOfModules(modules, max_modules, /* filter */ nullptr);
   }
   const char *PlatformDemangle(const char *name) override { return name; }
   void PlatformPrepareForSandboxing() override { }
 };
 
 Symbolizer *Symbolizer::PlatformInit() {
   IntrusiveList<SymbolizerTool> list;
   list.clear();
   list.push_back(new(symbolizer_allocator_) WinSymbolizerTool());
   return new(symbolizer_allocator_) WinSymbolizer(list);
 }
 
 }  // namespace __sanitizer
 
 #endif  // _WIN32

sanitizer_win.cc

 void Abort() {
   if (::IsDebuggerPresent())
     __debugbreak();
   internal__exit(3);
 }
 
 uptr GetListOfModules(LoadedModule *modules, uptr max_modules,
                       string_predicate_t filter) {
-  UNIMPLEMENTED();
+  HANDLE cur_process = GetCurrentProcess();
+
+  // Query the list of modules.  Start by assuming there are no more than 256
+  // modules and retry if that's not sufficient.
+  HMODULE *hmodules = 0;
+  uptr modules_buffer_size = sizeof(HMODULE) * 256;
+  DWORD bytes_required;
+  while (!hmodules) {
+    hmodules = (HMODULE *)MmapOrDie(modules_buffer_size, __FUNCTION__);
+    CHECK(EnumProcessModules(cur_process, hmodules, modules_buffer_size,
+                             &bytes_required));
+    if (bytes_required > modules_buffer_size) {
+      // Either there turned out to be more than 256 hmodules, or new hmodules
+      // could have loaded since the last try.  Retry.
+      UnmapOrDie(hmodules, modules_buffer_size);
+      hmodules = 0;
+      modules_buffer_size = bytes_required;
+    }
+  }
+
+  // |num_modules| is the number of modules actually present,
+  // |count| is the number of modules we return.
+  size_t nun_modules = bytes_required / sizeof(HMODULE),
+         count = 0;
+  for (size_t i = 0; i < nun_modules && count < max_modules; ++i) {
+    HMODULE handle = hmodules[i];
+    MODULEINFO mi;
+    if (!GetModuleInformation(cur_process, handle, &mi, sizeof(mi)))
+      continue;
+
+    char module_name[MAX_PATH];
+    bool got_module_name =
+        GetModuleFileNameA(handle, module_name, sizeof(module_name));
+    if (!got_module_name)
+      module_name[0] = '\0';
+
+    if (filter && !filter(module_name))
+      continue;
+
+    uptr base_address = (uptr)mi.lpBaseOfDll;
+    uptr end_address = (uptr)mi.lpBaseOfDll + mi.SizeOfImage;
+    LoadedModule &cur_module = modules[count];
+    cur_module.set(module_name, base_address);
+    // We add the whole module as one single address range.
+    cur_module.addAddressRange(base_address, end_address, /*executable*/ true);
+    count++;
+  }
+  UnmapOrDie(hmodules, modules_buffer_size);
+
+  return count;
 };
 
 #ifndef SANITIZER_GO
 // We can't use atexit() directly at __asan_init time as the CRT is not fully
 // initialized at this point.  Place the functions into a vector and use
 // atexit() as soon as it is ready for use (i.e. after .CRT$XIC initializers).
 InternalMmapVectorNoCtor<void (*)(void)> atexit_functions;
 

tests/sanitizer_procmaps_test.cc

 }
 #endif
 
 TEST(MemoryMappingLayout, DumpListOfModules) {
   const char *last_slash = strrchr(argv0, '/');
   const char *binary_name = last_slash ? last_slash + 1 : argv0;
   MemoryMappingLayout memory_mapping(false);
   const uptr kMaxModules = 100;
-  LoadedModule *modules =
-      (LoadedModule *)malloc(kMaxModules * sizeof(LoadedModule));
+  LoadedModule modules[kMaxModules];
   uptr n_modules = memory_mapping.DumpListOfModules(modules, kMaxModules, 0);
   EXPECT_GT(n_modules, 0U);
   bool found = false;
   for (uptr i = 0; i < n_modules; ++i) {
     if (modules[i].containsAddress((uptr)&noop)) {
       // Verify that the module name is sane.
       if (strstr(modules[i].full_name(), binary_name) != 0)
         found = true;
     }
     modules[i].clear();
   }
   EXPECT_TRUE(found);
-  free(modules);
 }
 
 }  // namespace __sanitizer
 #endif  // !defined(_WIN32)