Fixup non-canonical fault addresses for amd64.

This uses DisassemblerObjdump to add a processing step in
MinidumpProcessor to compute the true faulting address from register
state and disassembly of the fault instruction when the fault address
is suspicious (-1).

Bug: 901847
Change-Id: Ia1f77d542c4055c82ce2504db8c84a9e52001866
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3932957
Reviewed-by: Ivan Penkov <ivanpe@chromium.org>

src/google_breakpad/processor/minidump_processor.h

@@ -101,8 +101,10 @@ class MinidumpProcessor {
   // exception, if this information is available.  This will be a code
   // address when the crash was caused by problems such as illegal
   // instructions or divisions by zero, or a data address when the crash
-  // was caused by a memory access violation.
-  static string GetCrashReason(Minidump* dump, uint64_t* address);
+  // was caused by a memory access violation. If enable_objdump is set, this
+  // may use disassembly to compute the faulting address.
+  static string GetCrashReason(Minidump* dump, uint64_t* address,
+                               bool enable_objdump);
 
   // This function returns true if the passed-in error code is
   // something unrecoverable(i.e. retry should not happen).  For

src/processor/minidump_processor.cc

@@ -31,6 +31,7 @@
 #include <assert.h>
 
 #include <algorithm>
+#include <limits>
 #include <map>
 #include <string>
 #include <utility>
@@ -43,6 +44,7 @@
 #include "google_breakpad/processor/process_state.h"
 #include "google_breakpad/processor/exploitability.h"
 #include "google_breakpad/processor/stack_frame_symbolizer.h"
+#include "processor/disassembler_objdump.h"
 #include "processor/logging.h"
 #include "processor/stackwalker_x86.h"
 #include "processor/symbolic_constants_win.h"
@@ -117,7 +119,7 @@ ProcessResult MinidumpProcessor::Process(
     has_requesting_thread = exception->GetThreadID(&requesting_thread_id);
 
     process_state->crash_reason_ = GetCrashReason(
-        dump, &process_state->crash_address_);
+        dump, &process_state->crash_address_, enable_objdump_);
 
     process_state->exception_record_.set_code(
         exception->exception()->exception_record.exception_code,
@@ -758,8 +760,82 @@ bool MinidumpProcessor::GetProcessCreateTime(Minidump* dump,
   return true;
 }
 
+static bool IsCanonicalAddress(uint64_t address) {
+  uint64_t sign_bit = (address >> 63) & 1;
+  for (int shift = 48; shift < 63; ++shift) {
+    if (sign_bit != ((address >> shift) & 1)) {
+      return false;
+    }
+  }
+  return true;
+}
+
+static void CalculateFaultAddressFromInstruction(Minidump* dump,
+                                                 uint64_t* address) {
+  MinidumpException* exception = dump->GetException();
+  if (exception == NULL) {
+    BPLOG(INFO) << "Failed to get exception.";
+    return;
+  }
+
+  MinidumpContext* context = exception->GetContext();
+  if (context == NULL) {
+    BPLOG(INFO) << "Failed to get exception context.";
+    return;
+  }
+
+  uint64_t instruction_ptr = 0;
+  if (!context->GetInstructionPointer(&instruction_ptr)) {
+    BPLOG(INFO) << "Failed to get instruction pointer.";
+    return;
+  }
+
+  // Get memory region containing instruction pointer.
+  MinidumpMemoryList* memory_list = dump->GetMemoryList();
+  MinidumpMemoryRegion* memory_region =
+    memory_list ?
+    memory_list->GetMemoryRegionForAddress(instruction_ptr) : NULL;
+  if (!memory_region) {
+    BPLOG(INFO) << "No memory region around instruction pointer.";
+    return;
+  }
+
+  DisassemblerObjdump disassembler(context->GetContextCPU(), memory_region,
+                                   instruction_ptr);
+  fprintf(stderr, "%s %s %s\n", disassembler.operation().c_str(),
+    disassembler.src().c_str(), disassembler.dest().c_str());
+  if (!disassembler.IsValid()) {
+    BPLOG(INFO) << "Disassembling fault instruction failed.";
+    return;
+  }
+
+  // It's possible that we reach here when the faulting address is already
+  // correct, so we only update it if we find that at least one of the src/dest
+  // addresses is non-canonical. If both are non-canonical, we arbitrarily set
+  // it to the larger of the two, as this is more likely to be a known poison
+  // value.
+
+  bool valid_read, valid_write;
+  uint64_t read_address, write_address;
+
+  valid_read = disassembler.CalculateSrcAddress(*context, read_address);
+  valid_read &= !IsCanonicalAddress(read_address);
+
+  valid_write = disassembler.CalculateDestAddress(*context, write_address);
+  valid_write &= !IsCanonicalAddress(write_address);
+
+  if (valid_read && valid_write) {
+    *address = read_address > write_address ? read_address : write_address;
+  } else if (valid_read) {
+    *address = read_address;
+  } else if (valid_write) {
+    *address = write_address;
+  }
+}
+
 // static
-string MinidumpProcessor::GetCrashReason(Minidump* dump, uint64_t* address) {
+string MinidumpProcessor::GetCrashReason(Minidump* dump, uint64_t* address,
+                                         bool enable_objdump) {
   MinidumpException* exception = dump->GetException();
   if (!exception)
     return "";
@@ -1985,6 +2061,15 @@ string MinidumpProcessor::GetCrashReason(Minidump* dump, uint64_t* address) {
     *address = GetAddressForArchitecture(
       static_cast<MDCPUArchitecture>(raw_system_info->processor_architecture),
       *address);
+
+    // For invalid accesses to non-canonical addresses, amd64 cpus don't provide
+    // the fault address, so recover it from the disassembly and register state
+    // if possible.
+    if (enable_objdump
+        && raw_system_info->processor_architecture == MD_CPU_ARCHITECTURE_AMD64
+        && std::numeric_limits<uint64_t>::max() == *address) {
+      CalculateFaultAddressFromInstruction(dump, address);
+    }
   }
 
   return reason;

src/processor/minidump_processor_unittest.cc

@@ -799,6 +799,25 @@ TEST_F(MinidumpProcessorTest, TestFastFailException) {
   ASSERT_EQ(state.crash_reason(), "FAST_FAIL_FATAL_APP_EXIT");
 }
 
+#ifdef __linux__
+TEST_F(MinidumpProcessorTest, TestNonCanonicalAddress) {
+  // This tests if we can correctly fixup non-canonical address GPF fault
+  // addresses.
+  // Dump is captured from a toy executable and is readable by windbg.
+  MinidumpProcessor processor(nullptr, nullptr /*&supplier, &resolver*/);
+  processor.set_enable_objdump(true);
+
+  string minidump_file = GetTestDataPath()
+                         + "write_av_non_canonical.dmp";
+
+  ProcessState state;
+  ASSERT_EQ(processor.Process(minidump_file, &state),
+            google_breakpad::PROCESS_OK);
+  ASSERT_TRUE(state.crashed());
+  ASSERT_EQ(state.crash_address(), 0xfefefefefefefefeU);
+}
+#endif // __linux__
+
 }  // namespace
 
 int main(int argc, char* argv[]) {