Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002dbuild_002did-292

The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1688743002 .

5 files changed, 100 insertions, 74 deletions

diff --git a/src/client/linux/minidump_writer/linux_dumper.cc b/src/client/linux/minidump_writer/linux_dumper.cc
index 060e6c7c..cfa28a87 100644
--- a/src/client/linux/minidump_writer/linux_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_dumper.cc
@@ -121,9 +121,8 @@ bool
 LinuxDumper::ElfFileIdentifierForMapping(const MappingInfo& mapping,
                                          bool member,
                                          unsigned int mapping_id,
-                                         uint8_t identifier[sizeof(MDGUID)]) {
+                                         wasteful_vector<uint8_t>& identifier) {
   assert(!member || mapping_id < mappings_.size());
-  my_memset(identifier, 0, sizeof(MDGUID));
   if (IsMappedFileOpenUnsafe(mapping))
     return false;
 
diff --git a/src/client/linux/minidump_writer/linux_dumper.h b/src/client/linux/minidump_writer/linux_dumper.h
index f7fe1dd9..c3c79926 100644
--- a/src/client/linux/minidump_writer/linux_dumper.h
+++ b/src/client/linux/minidump_writer/linux_dumper.h
@@ -49,6 +49,7 @@
 
 #include "client/linux/dump_writer_common/mapping_info.h"
 #include "client/linux/dump_writer_common/thread_info.h"
+#include "common/linux/file_id.h"
 #include "common/memory.h"
 #include "google_breakpad/common/minidump_format.h"
 
@@ -129,7 +130,7 @@ class LinuxDumper {
   bool ElfFileIdentifierForMapping(const MappingInfo& mapping,
                                    bool member,
                                    unsigned int mapping_id,
-                                   uint8_t identifier[sizeof(MDGUID)]);
+                                   wasteful_vector<uint8_t>& identifier);
 
   uintptr_t crash_address() const { return crash_address_; }
   void set_crash_address(uintptr_t crash_address) {
diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc
index 838ea5f6..be533e15 100644
--- a/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc
+++ b/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc
@@ -66,6 +66,7 @@ using namespace google_breakpad;
 
 namespace {
 
+typedef wasteful_vector<uint8_t> id_vector;
 typedef testing::Test LinuxPtraceDumperTest;
 
 /* Fixture for running tests in a child process. */
@@ -105,11 +106,17 @@ class LinuxPtraceDumperChildTest : public testing::Test {
    * This is achieved by defining a TestBody macro further below.
    */
   virtual void RealTestBody() = 0;
+
+  id_vector make_vector() {
+    return id_vector(&allocator, kDefaultBuildIdSize);
+  }
+
  private:
   static const int kFatalFailure = 1;
   static const int kNonFatalFailure = 2;
 
   pid_t child_pid_;
+  PageAllocator allocator;
 };
 
 }  // namespace
@@ -310,14 +317,15 @@ TEST_F(LinuxPtraceDumperChildTest, LinuxGateMappingID) {
 
   // Need to suspend the child so ptrace actually works.
   ASSERT_TRUE(dumper.ThreadsSuspend());
-  uint8_t identifier[sizeof(MDGUID)];
+  id_vector identifier(make_vector());
   ASSERT_TRUE(dumper.ElfFileIdentifierForMapping(*mappings[index],
                                                  true,
                                                  index,
                                                  identifier));
-  uint8_t empty_identifier[sizeof(MDGUID)];
-  memset(empty_identifier, 0, sizeof(empty_identifier));
-  EXPECT_NE(0, memcmp(empty_identifier, identifier, sizeof(identifier)));
+
+  id_vector empty_identifier(make_vector());
+  empty_identifier.resize(kDefaultBuildIdSize, 0);
+  EXPECT_NE(empty_identifier, identifier);
   EXPECT_TRUE(dumper.ThreadsResume());
 }
 #endif
@@ -343,19 +351,18 @@ TEST_F(LinuxPtraceDumperChildTest, FileIDsMatch) {
   }
   ASSERT_TRUE(found_exe);
 
-  uint8_t identifier1[sizeof(MDGUID)];
-  uint8_t identifier2[sizeof(MDGUID)];
+  id_vector identifier1(make_vector());
+  id_vector identifier2(make_vector());
   EXPECT_TRUE(dumper.ElfFileIdentifierForMapping(*mappings[i], true, i,
                                                  identifier1));
   FileID fileid(exe_name);
   EXPECT_TRUE(fileid.ElfFileIdentifier(identifier2));
-  char identifier_string1[37];
-  char identifier_string2[37];
-  FileID::ConvertIdentifierToString(identifier1, identifier_string1,
-                                    37);
-  FileID::ConvertIdentifierToString(identifier2, identifier_string2,
-                                    37);
-  EXPECT_STREQ(identifier_string1, identifier_string2);
+
+  string identifier_string1 =
+      FileID::ConvertIdentifierToUUIDString(identifier1);
+  string identifier_string2 =
+      FileID::ConvertIdentifierToUUIDString(identifier2);
+  EXPECT_EQ(identifier_string1, identifier_string2);
 }
 
 /* Get back to normal behavior of TEST*() macros wrt TestBody. */
diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc
index 3103761f..f407caa7 100644
--- a/src/client/linux/minidump_writer/minidump_writer.cc
+++ b/src/client/linux/minidump_writer/minidump_writer.cc
@@ -73,6 +73,7 @@
 #include "client/linux/minidump_writer/linux_ptrace_dumper.h"
 #include "client/linux/minidump_writer/proc_cpuinfo_reader.h"
 #include "client/minidump_file_writer.h"
+#include "common/linux/file_id.h"
 #include "common/linux/linux_libc_support.h"
 #include "common/minidump_type_helper.h"
 #include "google_breakpad/common/minidump_format.h"
@@ -81,8 +82,10 @@
 namespace {
 
 using google_breakpad::AppMemoryList;
+using google_breakpad::auto_wasteful_vector;
 using google_breakpad::ExceptionHandler;
 using google_breakpad::CpuSet;
+using google_breakpad::kDefaultBuildIdSize;
 using google_breakpad::LineReader;
 using google_breakpad::LinuxDumper;
 using google_breakpad::LinuxPtraceDumper;
@@ -546,41 +549,40 @@ class MinidumpWriter {
     mod->base_of_image = mapping.start_addr;
     mod->size_of_image = mapping.size;
 
-    uint8_t cv_buf[MDCVInfoPDB70_minsize + NAME_MAX];
-    uint8_t* cv_ptr = cv_buf;
+    auto_wasteful_vector<uint8_t, kDefaultBuildIdSize> identifier_bytes(
+        dumper_->allocator());
 
-    const uint32_t cv_signature = MD_CVINFOPDB70_SIGNATURE;
-    my_memcpy(cv_ptr, &cv_signature, sizeof(cv_signature));
-    cv_ptr += sizeof(cv_signature);
-    uint8_t* signature = cv_ptr;
-    cv_ptr += sizeof(MDGUID);
     if (identifier) {
       // GUID was provided by caller.
-      my_memcpy(signature, identifier, sizeof(MDGUID));
+      identifier_bytes.insert(identifier_bytes.end(),
+                              identifier,
+                              identifier + sizeof(MDGUID));
     } else {
       // Note: ElfFileIdentifierForMapping() can manipulate the |mapping.name|.
-      dumper_->ElfFileIdentifierForMapping(mapping, member,
-                                           mapping_id, signature);
+      dumper_->ElfFileIdentifierForMapping(mapping,
+                                           member,
+                                           mapping_id,
+                                           identifier_bytes);
+    }
+
+    if (!identifier_bytes.empty()) {
+      UntypedMDRVA cv(&minidump_writer_);
+      if (!cv.Allocate(MDCVInfoELF_minsize + identifier_bytes.size()))
+        return false;
+
+      const uint32_t cv_signature = MD_CVINFOELF_SIGNATURE;
+      cv.Copy(&cv_signature, sizeof(cv_signature));
+      cv.Copy(cv.position() + sizeof(cv_signature), &identifier_bytes[0],
+              identifier_bytes.size());
+
+      mod->cv_record = cv.location();
     }
-    my_memset(cv_ptr, 0, sizeof(uint32_t));  // Set age to 0 on Linux.
-    cv_ptr += sizeof(uint32_t);
 
     char file_name[NAME_MAX];
     char file_path[NAME_MAX];
     dumper_->GetMappingEffectiveNameAndPath(
         mapping, file_path, sizeof(file_path), file_name, sizeof(file_name));
 
-    const size_t file_name_len = my_strlen(file_name);
-    UntypedMDRVA cv(&minidump_writer_);
-    if (!cv.Allocate(MDCVInfoPDB70_minsize + file_name_len + 1))
-      return false;
-
-    // Write pdb_file_name
-    my_memcpy(cv_ptr, file_name, file_name_len + 1);
-    cv.Copy(cv_buf, MDCVInfoPDB70_minsize + file_name_len + 1);
-
-    mod->cv_record = cv.location();
-
     MDLocationDescriptor ld;
     if (!minidump_writer_.WriteString(file_path, my_strlen(file_path), &ld))
       return false;
diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc
index e1046e12..db7d4f5d 100644
--- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc
+++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc
@@ -54,10 +54,6 @@
 
 using namespace google_breakpad;
 
-// Length of a formatted GUID string =
-// sizeof(MDGUID) * 2 + 4 (for dashes) + 1 (null terminator)
-const int kGUIDStringSize = 37;
-
 namespace {
 
 typedef testing::Test MinidumpWriterTest;
@@ -137,19 +133,7 @@ TEST(MinidumpWriterTest, MappingInfo) {
     0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
     0x88, 0x99, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF
   };
-  char module_identifier_buffer[kGUIDStringSize];
-  FileID::ConvertIdentifierToString(kModuleGUID,
-                                    module_identifier_buffer,
-                                    sizeof(module_identifier_buffer));
-  string module_identifier(module_identifier_buffer);
-  // Strip out dashes
-  size_t pos;
-  while ((pos = module_identifier.find('-')) != string::npos) {
-    module_identifier.erase(pos, 1);
-  }
-  // And append a zero, because module IDs include an "age" field
-  // which is always zero on Linux.
-  module_identifier += "0";
+  const string module_identifier = "33221100554477668899AABBCCDDEEFF0";
 
   // Get some memory.
   char* memory =
@@ -230,6 +214,53 @@ TEST(MinidumpWriterTest, MappingInfo) {
   close(fds[1]);
 }
 
+// Test that a binary with a longer-than-usual build id note
+// makes its way all the way through to the minidump unscathed.
+// The linux_client_unittest is linked with an explicit --build-id
+// in Makefile.am.
+TEST(MinidumpWriterTest, BuildIDLong) {
+  int fds[2];
+  ASSERT_NE(-1, pipe(fds));
+
+  const pid_t child = fork();
+  if (child == 0) {
+    close(fds[1]);
+    char b;
+    IGNORE_RET(HANDLE_EINTR(read(fds[0], &b, sizeof(b))));
+    close(fds[0]);
+    syscall(__NR_exit);
+  }
+  close(fds[0]);
+
+  ExceptionHandler::CrashContext context;
+  memset(&context, 0, sizeof(context));
+  ASSERT_EQ(0, getcontext(&context.context));
+  context.tid = child;
+
+  AutoTempDir temp_dir;
+  const string dump_path = temp_dir.path() + kMDWriterUnitTestFileName;
+
+  EXPECT_TRUE(WriteMinidump(dump_path.c_str(),
+                            child, &context, sizeof(context)));
+  close(fds[1]);
+
+  // Read the minidump. Load the module list, and ensure that
+  // the main module has the correct debug id and code id.
+  Minidump minidump(dump_path);
+  ASSERT_TRUE(minidump.Read());
+
+  MinidumpModuleList* module_list = minidump.GetModuleList();
+  ASSERT_TRUE(module_list);
+  const MinidumpModule* module = module_list->GetMainModule();
+  ASSERT_TRUE(module);
+  const string module_identifier = "030201000504070608090A0B0C0D0E0F0";
+  // This is passed explicitly to the linker in Makefile.am
+  const string build_id =
+      "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
+  EXPECT_EQ(module_identifier, module->debug_identifier());
+  EXPECT_EQ(build_id, module->code_identifier());
+}
+
 // Test that mapping info can be specified, and that it overrides
 // existing mappings that are wholly contained within the specified
 // range.
@@ -245,19 +276,7 @@ TEST(MinidumpWriterTest, MappingInfoContained) {
     0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
     0x88, 0x99, 0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF
   };
-  char module_identifier_buffer[kGUIDStringSize];
-  FileID::ConvertIdentifierToString(kModuleGUID,
-                                    module_identifier_buffer,
-                                    sizeof(module_identifier_buffer));
-  string module_identifier(module_identifier_buffer);
-  // Strip out dashes
-  size_t pos;
-  while ((pos = module_identifier.find('-')) != string::npos) {
-    module_identifier.erase(pos, 1);
-  }
-  // And append a zero, because module IDs include an "age" field
-  // which is always zero on Linux.
-  module_identifier += "0";
+  const string module_identifier = "33221100554477668899AABBCCDDEEFF0";
 
   // mmap a file
   AutoTempDir temp_dir;
@@ -410,12 +429,10 @@ TEST(MinidumpWriterTest, DeletedBinary) {
   EXPECT_STREQ(binpath.c_str(), module->code_file().c_str());
   // Check that the file ID is correct.
   FileID fileid(helper_path.c_str());
-  uint8_t identifier[sizeof(MDGUID)];
+  PageAllocator allocator;
+  wasteful_vector<uint8_t> identifier(&allocator, kDefaultBuildIdSize);
   EXPECT_TRUE(fileid.ElfFileIdentifier(identifier));
-  char identifier_string[kGUIDStringSize];
-  FileID::ConvertIdentifierToString(identifier,
-                                    identifier_string,
-                                    kGUIDStringSize);
+  string identifier_string = FileID::ConvertIdentifierToUUIDString(identifier);
   string module_identifier(identifier_string);
   // Strip out dashes
   size_t pos;