Recover memory mappings before writing dump on ChromeOS

src/client/linux/minidump_writer/linux_dumper.cc

 // Copyright (c) 2010, Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 // notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
@@ skipping 66 matching lines @@
   // because the semantics of the open may be driver-specific so we'd risk
   // hanging the crash dumper. And a file in /dev/ almost certainly has no
   // ELF file identifier anyways.
   return my_strncmp(mapping.name,
                     kMappedFileUnsafePrefix,
                     sizeof(kMappedFileUnsafePrefix) - 1) == 0;
 }
 
 namespace google_breakpad {
 
+#if defined(__CHROMEOS__)
+
+namespace {
+
+// Recover memory mappings before writing dump on ChromeOS
+//
+// On Linux, breakpad relies on /proc/[pid]/maps to associate symbols from
+// addresses. ChromeOS' hugepage implementation replaces some segments with
+// anonymous private pages, which is a restriction of current implementation
+// in Linux kernel at the time of writing. Thus, breakpad can no longer
+// symbolize addresses from those text segments replaced with hugepages.
+//
+// This postprocess tries to recover the mappings. Because hugepages are always
+// inserted in between some .text sections, it tries to infer the names and
+// offsets of the segments, by looking at segments immediately precede and
+// succeed them.
+//
+// For example, a text segment before hugepage optimization
+//   02001000-03002000 r-xp /opt/google/chrome/chrome
+//
+// can be broken into
+//   02001000-02200000 r-xp /opt/google/chrome/chrome
+//   02200000-03000000 r-xp
+//   03000000-03002000 r-xp /opt/google/chrome/chrome
+//
+// For more details, see:
+// crbug.com/628040 ChromeOS' use of hugepages confuses crash symbolization

[Wez, 2016/07/18 22:28:15]

Do/will we have a bug filed to remove this hack if/when Linux supports remapping
executable pages to hugepages, or better still does it dynamically itself?

+
+// Copied from CrOS' hugepage implementation, which is unlikely to change.
+// The hugepage size is 2M.
+const unsigned int kHpageShift = 21;
+const size_t kHpageSize = (1 << kHpageShift);
+const size_t kHpageMask = (~(kHpageSize - 1));
+
+// Find and merge anonymous r-xp segments with surrounding named segments.
+// There are two cases:
+
+// Case 1: curr, next

[Wez, 2016/07/18 22:28:15]

What do these "cases" mean? Do we sometimes see Chrome with two segments and
sometimes with three, in minidumps?  The example listed above only shows the
three-segment case. When do we see two?

+//   curr is anonymous
+//   curr is r-xp
+//   curr.size >= 2M
+//   curr.size is a multiple of 2M.

[Wez, 2016/07/18 22:28:15]

Do we need to be this specific about the sizes? How likely is it that we have
contiguous mapped & anonymous segments? Could we avoid making assumptions about
hugepage constants if we took the layout of the executable into account (e.g. if
we see a contiguous anonymous+mapped pair and the offset into the source file
matches, is that sufficient?)

+//   next is backed by some file.
+//   curr and next are contiguous.
+//   offset(next) == sizeof(curr)
+void TryRecoverMappings(MappingInfo *curr, MappingInfo *next) {

[Wez, 2016/07/18 22:28:15]

Looks like this function actually checks whether |curr| can be treated as part
of |next| - should it be called TryTwoWayMerge() for example?

+  // Merged segments are marked with size = 0.

[Wez, 2016/07/18 22:28:15]

It's not clear from the context that it's possible to reach here having already
merged some segments.  I'd assumed that we generally expect to merge at-most
three segments into a single one, given the way that hugepages work right now?

+  if (curr->size == 0 || next->size == 0)
+    return;
+
+  if (curr->size >= kHpageSize &&
+      curr->exec &&
+      (curr->size & kHpageMask) == curr->size &&
+      (curr->start_addr & kHpageMask) == curr->start_addr &&
+      curr->name[0] == '\0' &&
+      next->name[0] != '\0' &&
+      curr->start_addr + curr->size == next->start_addr &&
+      curr->size == next->offset) {

[Wez, 2016/07/18 22:28:15]

Strongly suggest turning this into a set of early-exit conditions, and
commenting on what each check is intended to achieve, rather than having a big
block comment and then a big block if.

+
+    // matched

[Wez, 2016/07/18 22:28:15]

nit: This comment doesn't add any information :P

+    my_strlcpy(curr->name, next->name, NAME_MAX);
+    if (next->exec) {
+      // (curr, next)

[Wez, 2016/07/18 22:28:14]

nit: Nor does this one ;)

+      curr->size += next->size;
+      next->size = 0;
+    }
+  }
+}
+
+// Case 2: prev, curr, next
+//   curr is anonymous
+//   curr is r-xp
+//   curr.size >= 2M
+//   curr.size is a multiple of 2M.
+//   next and prev are backed by the same file.
+//   prev, curr and next are contiguous.
+//   offset(next) == offset(prev) + sizeof(prev) + sizeof(curr)
+void TryRecoverMappings(MappingInfo *prev, MappingInfo *curr,
+    MappingInfo *next) {
+  // Merged segments are marked with size = 0.
+  if (prev->size == 0 || curr->size == 0 || next->size == 0)
+    return;
+
+  if (curr->size >= kHpageSize &&
+      curr->exec &&
+      (curr->size & kHpageMask) == curr->size &&
+      (curr->start_addr & kHpageMask) == curr->start_addr &&
+      curr->name[0] == '\0' &&
+      next->name[0] != '\0' &&
+      curr->start_addr + curr->size == next->start_addr &&
+      prev->start_addr + prev->size == curr->start_addr &&
+      my_strncmp(prev->name, next->name, NAME_MAX) == 0 &&
+      next->offset == prev->offset + prev->size + curr->size) {
+
+    // matched
+    my_strlcpy(curr->name, prev->name, NAME_MAX);
+    if (prev->exec) {
+      curr->offset = prev->offset;
+      curr->start_addr = prev->start_addr;
+      if (next->exec) {
+        // (prev, curr, next)
+        curr->size += prev->size + next->size;
+        prev->size = 0;
+        next->size = 0;
+      } else {
+        // (prev, curr), next
+        curr->size += prev->size;
+        prev->size = 0;
+      }
+    } else {
+      curr->offset = prev->offset + prev->size;
+      if (next->exec) {
+        // prev, (curr, next)
+        curr->size += next->size;
+        next->size = 0;
+      } else {
+        // prev, curr, next
+      }
+    }
+  }
+}
+
+// mappings_ is sorted excepted for the first entry.
+// This function tries to merge segemnts into the first entry,

[Wez, 2016/07/18 22:28:15]

typo: segments

+// then check for other sorted entries.
+// See LinuxDumper::EnumerateMappings().

[Wez, 2016/07/18 22:28:15]

nit: EnumerateMappings() doesn't have any documentating comment to define its
behaviour, though, so it's not clear what you're asking the reader to see there?

+void CrOSPostProcessMappings(wasteful_vector<MappingInfo*>& mappings) {
+  // Find the candidate "next" to first segment, which is the only one that
+  // could be out-of-order.
+  size_t l = 1;
+  size_t r = mappings.size();

[Wez, 2016/07/18 22:28:15]

nit: Please use descriptive variable names, for readability. :)

+  size_t next = mappings.size();
+  while (l < r) {
+    int m = (l + r) / 2;
+    if (mappings[m]->start_addr > mappings[0]->start_addr)
+      r = next = m;
+    else
+      l = m + 1;

[Wez, 2016/07/18 22:28:15]

nit: I'd recommend using {} around the two cases here, for readability.

+  }
+
+  // Try to merge segments into the first.

[Wez, 2016/07/18 22:28:15]

It's not clear to me what this is actually trying to do; I see the comment says
"merge segments into the first entry.." but _why_ are we expecting to be able to
merge mappings[0] into some arbitrary later segment, and why do we need to do
that outside the scope of the loops below? Is there something special about the
first entry? Does EnumerateMappings() guarantee that the first entry is the
chrome exe, for example (though I see a comment in that function that implies
that is not generally the case ;)?

+  if (next < mappings.size()) {
+    TryRecoverMappings(mappings[0], mappings[next]);
+    if (next - 1 > 0)
+      TryRecoverMappings(mappings[next - 1], mappings[0], mappings[next]);
+  }
+
+  // Iterate through normal, sorted cases.
+  // Normal case 1.
+  for (size_t i = 1; i < mappings.size() - 1; i++)
+    TryRecoverMappings(mappings[i], mappings[i + 1]);
+
+  // Normal case 2.
+  for (size_t i = 1; i < mappings.size() - 2; i++)
+    TryRecoverMappings(mappings[i], mappings[i + 1], mappings[i + 2]);

[Wez, 2016/07/18 22:30:48]

If we already ran through trying to merge (anon, mapped) into single mapped
entries then won't we miss three-entry (mapped, anon, mapped) cases now)?

+
+  // Collect merged (size == 0) segments.
+  size_t f, e;
+  for (f = e = 0; e < mappings.size(); e++)
+    if (mappings[e]->size > 0)
+      mappings[f++] = mappings[e];
+  mappings.resize(f);
+}
+
+}  // namespace
+#endif  // __CHROMEOS__
+
 // All interesting auvx entry types are below AT_SYSINFO_EHDR
 #define AT_MAX AT_SYSINFO_EHDR
 
 LinuxDumper::LinuxDumper(pid_t pid, const char* root_prefix)
     : pid_(pid),
       root_prefix_(root_prefix),
       crash_address_(0),
       crash_signal_(0),
       crash_thread_(pid),
       threads_(&allocator_, 8),
       mappings_(&allocator_),
       auxv_(&allocator_, AT_MAX + 1) {
   assert(root_prefix_ && my_strlen(root_prefix_) < PATH_MAX);
   // The passed-in size to the constructor (above) is only a hint.
   // Must call .resize() to do actual initialization of the elements.
   auxv_.resize(AT_MAX + 1);
 }
 
 LinuxDumper::~LinuxDumper() {
 }
 
 bool LinuxDumper::Init() {
   return ReadAuxv() && EnumerateThreads() && EnumerateMappings();
 }
 
 bool LinuxDumper::LateInit() {
 #if defined(__ANDROID__)
   LatePostprocessMappings();
 #endif
+
+#if defined(__CHROMEOS__)
+  CrOSPostProcessMappings(mappings_);
+#endif
+
   return true;
 }
 
 bool
 LinuxDumper::ElfFileIdentifierForMapping(const MappingInfo& mapping,
                                          bool member,
                                          unsigned int mapping_id,
                                          wasteful_vector<uint8_t>& identifier) {
   assert(!member || mapping_id < mappings_.size());
   if (IsMappedFileOpenUnsafe(mapping))
@@ skipping 210 matching lines @@
         if (*i3 == ' ') {
           const char* name = NULL;
           // Only copy name if the name is a valid path name, or if
           // it's the VDSO image.
           if (((name = my_strchr(line, '/')) == NULL) &&
               linux_gate_loc &&
               reinterpret_cast<void*>(start_addr) == linux_gate_loc) {
             name = kLinuxGateLibraryName;
             offset = 0;
           }
           // Merge adjacent mappings with the same name into one module,

[Mark Mentovai, 2016/07/18 22:03:29]

Would it have been easier to work the change in here?

           // assuming they're a single library mapped by the dynamic linker
           if (name && !mappings_.empty()) {
             MappingInfo* module = mappings_.back();
             if ((start_addr == module->start_addr + module->size) &&
                 (my_strlen(name) == my_strlen(module->name)) &&
                 (my_strncmp(name, module->name, my_strlen(name)) == 0) &&
                 (exec == module->exec)) {
               module->size = end_addr - module->start_addr;
               line_reader->PopLine(line_len);
               continue;
@@ skipping 221 matching lines @@
       exe_stat.st_dev == new_path_stat.st_dev &&
       exe_stat.st_ino == new_path_stat.st_ino) {
     return false;
   }
 
   my_memcpy(path, exe_link, NAME_MAX);
   return true;
 }
 
 }  // namespace google_breakpad