LLDB isn't checking signatures when loading modules from a minidump

[amccarth-google]

Bugzilla Link	47528
Version	unspecified
OS	Windows NT
CC	@JDevlieghere,@labath

Extended Description

When loading a minidump for post-mortem debugging, LLDB may select the wrong executable as the target because it checks only file names rather then signatures, timestamps, etc. (Also applies to other modules, like DLLs.)

For example, given a minidump from a crashed C:\Program Files\foo.exe on a user's machine, loading that minidump in LLDB in a developer machine might load a local foo.exe simply because it happens to have the same path, even if it is a different version of the program.

The local one should be rejected based on signature (UUID) and/or other version information stored in the minidump. Once rejected, LLDB should continue searching for a matching binary until it finds one that matches or exhausts all the locations to be searched.

[llvmbot]

I agree. If this extra code is a problem we can restrict to linux or android? The minidump has a info in the system information that can help us enable this for linux or Android only?

MINIDUMP_SYSTEM_INFO: ProcessorArchitecture = 0x00000005 "ARM" ProcessorLevel = 0x00000007 ProcessorRevision = 0x0000002a NumberOfProcessors = 0x00000004 ProductType = 0x00000000 MajorVersion = 0x00000000 MinorVersion = 0x00000000 BuildNumber = 0x00000000 PlatformId = 0x00008203 "Android" CSDVersionRva = 0x0007aac0 "Linux 3.10.65-1122970 #​2 SMP PREEMPT Thu Sep 15 04:39:55 KST 2016 armv7l" SuiteMask = 0x00000000 Reserved2 = 0x00000000 ProcessorFeatures[0] = 0x0000000000000000 ProcessorFeatures[1] = 0x0000000000000000

[labath]

That special codepath in the minidump code was added for a very specific reason -- to hac^H^H^Hwork around the fact that the breakpad minidump generator was truncating the "UUIDs" in elf files, and so the UUID direct comparison (which is done in Target::GetOrCreateModule would not work. I don't think it was ever intended for this to apply to COFF files, and I would say the fact that is does is a bug.

If this extra code is what is causing the problem, then I think we should do one of two things (maybe both):

• reduce the scope of the workaround to ELF files -- as far as I am aware of, other formats do not suffer from this problem. For example, before we start any funky UUID matching, we could check that the second GetOrCreateModule call did in fact return an ELF file.
• strenghten this check: // We consider the module to be a match if the minidump UUID is a // prefix of the actual UUID, or if either of the UUIDs are empty. const bool match = dmp_bytes.empty() || mod_bytes.empty() || mod_bytes.take_front(dmp_bytes.size()) == dmp_bytes;

Given that this is supposed to handle truncated UUIDs, I don't think we need to give empty UUIDs a free pass. The generic code already has special handling for empty UUIDs, so this should be already handled by the first GetOrCreateModule call. I think this should consider to match only of both uuids are non-empty and the prefix of the actual UUID matches.

[llvmbot]

So the code currently fills out a ModuleSpec with the UUID and architecture:

const auto uuid = m_minidump_parser->GetModuleUUID(module); auto file_spec = FileSpec(name, GetArchitecture().GetTriple()); ModuleSpec module_spec(file_spec, uuid); module_spec.GetArchitecture() = GetArchitecture(); Status error; // Try and find a module with a full UUID that matches. This function will // add the module to the target if it finds one. lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, true / notify /, &error);

But if the COFF file isn't correctly extracting the UUID when it loads the object file, this can be the cause as Pavel suggested. So the fix here is to fix the COFF plug-in to correctly extract the UUID.

[amccarth-google]

Thanks for the tip. There's definitely a special path through the code for a file that matches the path stored in the minidump. That path is bypassing the version checking. The solution might involve keeping it as a candidate match and proceeding through the search to see if there's one that does have a matching UUID. Obviously, a matching UUID should win. But if there if there are no matches, then we can use the candidate that didn't have the UUID.

[labath]

We already have code which tries to extract the (U/G)UID from the coff file (ObjectFilePECOFF.cpp:GetCoffUUID).

So, I think this should work as long as all files have their UUIDs. One way this can possibly go wrong is if the file that's found first does not have any UUID. In that case I believe lldb will consider this to be a match, and not look any further.

I'm not entirely sure what's the best solution here. I'm pretty sure that the current lldb behavior is deliberate but I can see how it can create problems...

[amccarth-google]

assigned to @amccarth-google