[Lldb-commits] [PATCH] D62505: Fix multiple module loaded with the same UUID

Mon May 27 15:58:42 PDT 2019

aadsm created this revision.
aadsm added reviewers: clayborg, xiaobai, labath.
Herald added subscribers: lldb-commits, srhines.
Herald added a project: LLDB.

This solves the problem of lldb not recognizing android vendor's ndks that can live along side the system ndks. More about this feature here: https://source.android.com/devices/architecture/vndk.

A quick description of the problem
==================================

An android app can have vendor ndk's that are modified versions of the system ndk's (e.g.: `/system/lib/vndk-sp/libcutils.so` and `/system/lib/libcutils.so`). It's possible to configure how your app depends on one or the other and in the end both libraries will be loaded into the process. Additionally, a vndk can be an exact copy of the system ndk and both will still be loaded into the process (this is true on the Samsung S9).

As of now lldb is not able to distinguish one from the other due to a combination of relying solely on the module filename (instead of full path) and on the assumption that every library loaded into memory will have a different uuid.

You can see this by checking the memory regions of the running app, and see the 2 versions fo the same module loaded:

  $ cat /proc/25047/maps | grep libcutils
  c84dd000-c84e9000 r-xp 00000000 fd:00 3466     /system/lib/vndk-sp/libcutils.so
  e5ba3000-e5baf000 r-xp 00000000 fd:00 2989     /system/lib/libcutils.so
  
  $ md5sum /system/lib/libcutils.so /system/lib/vndk-sp/libcutils.so
  35b488c4b4711ee9be7abee85abfbfdb  /system/lib/libcutils.so
  35b488c4b4711ee9be7abee85abfbfdb  /system/lib/vndk-sp/libcutils.so

On lldb you can see that there are 2 modules loaded but they actually point to the same file on disk and also the same memory region:

  (lldb) image list libcutils.so
  [  0] 872E94B2-A24A-BF07-447E-ABE347E9D9F1 0xc84dd000 /Users/aadsm/.lldb/module_cache/remote-android/.cache/872E94B2-A24A-BF07-447E-ABE347E9D9F1/libcutils.so 
  [  1] 872E94B2-A24A-BF07-447E-ABE347E9D9F1 0xc84dd000 /Users/aadsm/.lldb/module_cache/remote-android/.cache/872E94B2-A24A-BF07-447E-ABE347E9D9F1/libcutils.so 
  
  (lldb) script str(lldb.target.GetModuleAtIndex(5).GetPlatformFileSpec())
  '/system/lib/vndk-sp/libcutils.so'
  (lldb) script str(lldb.target.GetModuleAtIndex(233).GetPlatformFileSpec())
  '/system/lib/vndk-sp/libcutils.so'

The proposed fix
----------------

This fix happens in 3 different places:

**lldb-server:**
The `qModuleInfo` packet was matching libraries by using only the filename and not the full path so it was always returning the same file path back.
I fixed this by making sure it checks the full path when the file received has a dirname.

**Module::MatchesModuleSpec:**
This function is happy to match module specs purely based on the UUID, meaning that even if we have the right path from the server we still find a match against the wrong module spec.
I fixed this by checking the paths before the UUID. The UUID comparison first will probably be faster but I don't it's a big deal because `FileSpec::Equal` uses `ConstString` so it still should be just pointer comparison.

**ModuleCache::Get:**
The module cache is being keyed by the UUID so again it will load the wrong Module even if we have the right module spec. To get around this I changed the cache key to be the module spec file spec path.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62505

Files:
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Target/ModuleCache.cpp


Index: lldb/source/Target/ModuleCache.cpp
===================================================================

--- lldb/source/Target/ModuleCache.cpp
+++ lldb/source/Target/ModuleCache.cpp
@@ -211,8 +211,8 @@
 Status ModuleCache::Get(const FileSpec &root_dir_spec, const char *hostname,
                         const ModuleSpec &module_spec,
                         ModuleSP &cached_module_sp, bool *did_create_ptr) {
-  const auto find_it =
-      m_loaded_modules.find(module_spec.GetUUID().GetAsString());
+  const auto module_path = module_spec.GetFileSpec().GetCString();
+  const auto find_it = m_loaded_modules.find(module_path);
   if (find_it != m_loaded_modules.end()) {
     cached_module_sp = (*find_it).second.lock();
     if (cached_module_sp)
@@ -256,8 +256,7 @@
   if (FileSystem::Instance().Exists(symfile_spec))
     cached_module_sp->SetSymbolFileFileSpec(symfile_spec);
 
-  m_loaded_modules.insert(
-      std::make_pair(module_spec.GetUUID().GetAsString(), cached_module_sp));
+  m_loaded_modules.insert(std::make_pair(module_path, cached_module_sp));
 
   return Status();
 }
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===================================================================
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -1628,8 +1628,9 @@
   FileSystem::Instance().Resolve(module_file_spec);
 
   file_spec.Clear();
+  bool full_comparison = false;
   for (const auto &it : m_mem_region_cache) {
-    if (it.second.GetFilename() == module_file_spec.GetFilename()) {
+    if (FileSpec::Compare(it.second, module_file_spec, full_comparison) == 0) {
       file_spec = it.second;
       return Status();
     }
Index: lldb/source/Core/Module.cpp
===================================================================
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -1606,13 +1606,9 @@
 }
 
 bool Module::MatchesModuleSpec(const ModuleSpec &module_ref) {
-  const UUID &uuid = module_ref.GetUUID();
-
-  if (uuid.IsValid()) {
-    // If the UUID matches, then nothing more needs to match...
-    return (uuid == GetUUID());
-  }
-
+  // We first check the file paths as there can be more than one loaded module
+  // at different paths with the same UUID. This happens with some Android
+  // installations where the VNDK modules are the same as the system ones.
   const FileSpec &file_spec = module_ref.GetFileSpec();
   if (file_spec) {
     if (!FileSpec::Equal(file_spec, m_file, (bool)file_spec.GetDirectory()) &&
@@ -1628,6 +1624,12 @@
       return false;
   }
 
+  const UUID &uuid = module_ref.GetUUID();
+  if (uuid.IsValid()) {
+    // If the UUID matches, then nothing more needs to match...
+    return (uuid == GetUUID());
+  }
+
   const ArchSpec &arch = module_ref.GetArchitecture();
   if (arch.IsValid()) {
     if (!m_arch.IsCompatibleMatch(arch))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D62505.201594.patch
Type: text/x-patch
Size: 2917 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190527/1d67face/attachment.bin>
