Page MenuHomePhabricator

[lldb] Minidump: check for .text hash match with directory
ClosedPublic

Authored by JosephTremoulet on Oct 9 2020, 12:14 PM.

Details

Summary

When opening a minidump, we might discover that it reports a UUID for a
module that doesn't match the build ID, but rather a hash of the .text
section (according to either of two different hash functions, used by
breakpad and Facebook respectively). The current logic searches for a
module by filename only to check the hash; this change updates it to
first search by directory+filename. This is important when the
directory specified in the minidump must be interpreted relative to a
user-provided sysoort, as the leaf directory won't be in the search path
in that case.

Also add a regression test; without this change, module validation fails
because we have just the placeholder module which reports as its path
the platform path in the minidump.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
JosephTremoulet requested review of this revision.Oct 9 2020, 12:14 PM
clayborg requested changes to this revision.Oct 9 2020, 5:00 PM
clayborg added inline comments.
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
562–563

This whole "if" statement will need to be tried with both files. We might end up finding a file in the sysroot that is a valid file, but does not actually match the .text hash. Then the search by basename, which will search using the "target.exec-search-paths" or "target.debug-file-search-paths" settings might find another file that you might have downloaded, and it will need to go through all of the .text checks as well.

So as we now have more than 1 file to check, this "if (module_sp)" and its contents should be made into a function. Look at the code above I would create a method of ProcessMinidump that does all this:

ModuleSP ProcessMinidump::GetOrCreateModule(UUID minidump_uuid, ModuleSpec module_spec) {
  ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, true /* notify */, &error);
  if (!module_sp)
    return module_sp;
  // 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 auto dmp_bytes = minidump_uuid.GetBytes();
  const auto mod_bytes = module_sp->GetUUID().GetBytes();
  const bool match = dmp_bytes.empty() || mod_bytes.empty() ||
  mod_bytes.take_front(dmp_bytes.size()) == dmp_bytes;
  if (match) {
    LLDB_LOG(log, "Partial uuid match for {0}.", name);
    return module_sp;
  }

  // Breakpad generates minindump files, and if there is no GNU build
  // ID in the binary, it will calculate a UUID by hashing first 4096
  // bytes of the .text section and using that as the UUID for a module
  // in the minidump. Facebook uses a modified breakpad client that
  // uses a slightly modified this hash to avoid collisions. Check for
  // UUIDs from the minindump that match these cases and accept the
  // module we find if they do match.
  std::vector<uint8_t> breakpad_uuid;
  std::vector<uint8_t> facebook_uuid;
  HashElfTextSection(module_sp, breakpad_uuid, facebook_uuid);
  if (dmp_bytes == llvm::ArrayRef<uint8_t>(breakpad_uuid)) {
    LLDB_LOG(log, "Breakpad .text hash match for {0}.", name);
    return module_sp;
  }
  if (dmp_bytes == llvm::ArrayRef<uint8_t>(facebook_uuid)) {
    LLDB_LOG(log, "Facebook .text hash match for {0}.", name);
    return module_sp;
  }
  // The UUID wasn't a partial match and didn't match the .text hash
  // so remove the module from the target, we will need to create a
  // placeholder object file.
  GetTarget().GetImages().Remove(module_sp);
  module_sp.reset();
  return module_sp;
}

Then call this function once with basename_module_spec with a full path, and if no matching module is returned, call the remove the directory and call it again.

This revision now requires changes to proceed.Oct 9 2020, 5:00 PM
  • Handle finding wrong version before right one
JosephTremoulet marked an inline comment as done.Oct 10 2020, 9:58 AM

Thanks for the speedy review! I've updated it to include a testcase that shows the problem you described, and include the fix you suggested.

clayborg accepted this revision.Oct 14 2020, 2:27 PM

Sorry for the delay, I was out of town until today. Changes look good! Always good to have more robust support for finding binaries in the sysroot.

This revision is now accepted and ready to land.Oct 14 2020, 2:27 PM
This revision was landed with ongoing or failed builds.Oct 16 2020, 6:33 AM
This revision was automatically updated to reflect the committed changes.