Fix multiple module loaded with the same UUID

[aadsm]

Summary

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.

After these fixes you can see both modules independently loaded in lldb:

(lldb) image list libcutils.so
[  0] 872E94B2-A24A-BF07-447E-ABE347E9D9F1 0xe5ba3000 /Users/aadsm/.lldb/module_cache/remote-android/.cache/872E94B2-A24A-BF07-447E-ABE347E9D9F1/libcutils.so 
[  1] 872E94B2-A24A-BF07-447E-ABE347E9D9F1 0xc84d7000 /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/libcutils.so'
(lldb) script str(lldb.target.GetModuleAtIndex(233).GetPlatformFileSpec())
'/system/lib/vndk-sp/libcutils.so'

[labath]

I agree we need to support a module being loaded multiple times, but I'm not sure if this is the right way to achieve that. I mean, if two files are exactly the same, then it should be fine (and even preferable) to download them just once, store them under a single entry (based on the UUID) in the module cache, and parse their symbols just once. It seems like somebody even intended for us to support a module being loaded multiple times as there is this TODO https://github.com/llvm-mirror/lldb/blob/1b2170d0116d52a219574780e7eb01043c3712e1/source/Target/SectionLoadList.cpp#L50 in the SectionLoadList class.

However, this is not an area I'm super familiar with. Maybe @jingham knows more about this?

[aadsm]

Interesting, I did miss that comment when I checked that class. This is something @clayborg was concerned with when I discussed this solution with him. The main reason I was not keen in having multiple load addresses for a section is because that would change the GetLoadAddress to return an array instead of an addr_t or some kind of mechanism to be able to correctly resolve the load address of an Address (felt like too big of a change for what it looks like an edge case?).
So I preferred to look at this situation (of 2 modules with the exact same data) as a coincidence and be ok with parsing the same data twice. It is actually not clear to me why Samsung ships these vndks if they're an exact copy of the system ones.

[clayborg]

In D62505#1518833, @labath wrote:

I agree we need to support a module being loaded multiple times, but I'm not sure if this is the right way to achieve that. I mean, if two files are exactly the same, then it should be fine (and even preferable) to download them just once, store them under a single entry (based on the UUID) in the module cache, and parse their symbols just once. It seems like somebody even intended for us to support a module being loaded multiple times as there is this TODO https://github.com/llvm-mirror/lldb/blob/1b2170d0116d52a219574780e7eb01043c3712e1/source/Target/SectionLoadList.cpp#L50 in the SectionLoadList class.

However, this is not an area I'm super familiar with. Maybe @jingham knows more about this?

So I am familiar with this code and I was working with Antonio on this. I suggested the same thing, but then the amount of work we would need to do to support loading the same file in different areas was a lot more intrusive. What we really want is to have the same module loaded in two places, but each module would need to have its own platform path, one with /system/lib/libcutils.so and one with /system/lib/vndk-sp/libcutils.so. If we don't do this, then we show the user one of those file paths twice which doesn't match reality. That led us to think about a BaseModule class that wouldn't contain the platform file spec, and then we could have two Module instances that share the same BaseModule class. The BaseModule would always contain the local FileSpec that was uniqued. That seemed like a lot of hassle for not much gain. If we allow a lldb_private::Section to be loaded multiple times, there are a lot of places that would need to change in LLDB code where anytime someone tries to resolve a lldb_private::Address to a load address, it will need to be able to deal with multiple results. That will change many locations around the LLDB code base. That is the right fix IMHO, but it will make this code change quite large. So we decided to post this diff up for review so we could talk about the different solutions.

So if we end up going with one module, how do we propose to get around the fact that there are multiple platform paths ("/system/lib/libcutils.so" and "/system/lib/vndk-sp/libcutils.so") for this module? Just make platform path be an array instead of a single path?

[labath]

In D62505#1519166, @aadsm wrote:

Interesting, I did miss that comment when I checked that class. This is something @clayborg was concerned with when I discussed this solution with him. The main reason I was not keen in having multiple load addresses for a section is because that would change the GetLoadAddress to return an array instead of an addr_t or some kind of mechanism to be able to correctly resolve the load address of an Address (felt like too big of a change for what it looks like an edge case?).

I wouldn't say this is specific to some class of samsung phones. It is easy to use dlmopen(3) to open the same library multiple times. Just try calling dlmopen(LM_ID_NEWLM, "/lib64/libc.so.6", RTLD_NOW) in a loop a couple of times and see what happens in /proc/<pid>/maps. (I believe this is the same function that android uses to implement vndk separation.)

In D62505#1519178, @clayborg wrote:

So if we end up going with one module, how do we propose to get around the fact that there are multiple platform paths ("/system/lib/libcutils.so" and "/system/lib/vndk-sp/libcutils.so") for this module? Just make platform path be an array instead of a single path?

TBH, I'm not really convinced that the "platform path" should be a property of the Module.

Imagine a situation where I have the same module loaded in multiple targets (maybe on different remote platforms, or maybe not). It makes sense that each target can have the module located at a different path. So maybe the "platform path" should be a property of the target, saying "where did this target load the module from"? Of course, this won't help when you have the same module loaded multiple times in the same target, so maybe this does need to be a list of paths? I'm not sure, I'm just thinking aloud here...

Right now, I'm also lean towards making a Section be loadable multiple times, however this definitely needs more discussion. I'm interested to hear what @jingham thinks of this.

lldb/source/Core/Module.cpp

if (object_file != nullptr) {
     return true;
   } else {
     changed = false;
   }
   return false;
 }
 
 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()) &&
         !FileSpec::Equal(file_spec, m_platform_file,
                          (bool)file_spec.GetDirectory()))
       return false;
   }
 
   const FileSpec &platform_file_spec = module_ref.GetPlatformFileSpec();
   if (platform_file_spec) {
     if (!FileSpec::Equal(platform_file_spec, GetPlatformFileSpec(),
                          (bool)platform_file_spec.GetDirectory()))
       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))
       return false;
   }
 
   ConstString object_name = module_ref.GetObjectName();
   if (object_name) {

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

Status NativeProcessLinux::GetLoadedModuleFileSpec(const char *module_path,
   Status error = PopulateMemoryRegionCache();
   if (error.Fail())
     return error;
 
   FileSpec module_file_spec(module_path);
   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();
     }
   }
   return Status("Module file (%s) not found in /proc/%" PRIu64 "/maps file!",
                 module_file_spec.GetFilename().AsCString(), GetID());
 }
 

lldb/source/Target/ModuleCache.cpp

if (error.Fail())
     return Status("Failed to create link to %s: %s",
                   module_file_path.GetPath().c_str(), error.AsCString());
   return Status();
 }
 
 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)
       return Status();
     m_loaded_modules.erase(find_it);
   }
 
   const auto module_spec_dir =
error = ModuleList::GetSharedModule(cached_module_spec, cached_module_sp,
                                       nullptr, nullptr, did_create_ptr, false);
   if (error.Fail())
     return error;
 
   FileSpec symfile_spec = GetSymbolFileSpec(cached_module_sp->GetFileSpec());
   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();
 }
 
 Status ModuleCache::GetAndPut(const FileSpec &root_dir_spec,
                               const char *hostname,
                               const ModuleSpec &module_spec,
                               const ModuleDownloader &module_downloader,