diff --git a/src/google_breakpad/processor/minidump_processor.h b/src/google_breakpad/processor/minidump_processor.h index aa44e86c..137ef444 100644 --- a/src/google_breakpad/processor/minidump_processor.h +++ b/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 diff --git a/src/processor/minidump_processor.cc b/src/processor/minidump_processor.cc index a80baf45..bf561dfa 100644 --- a/src/processor/minidump_processor.cc +++ b/src/processor/minidump_processor.cc @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -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(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::max() == *address) { + CalculateFaultAddressFromInstruction(dump, address); + } } return reason; diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc index 6dfa54a6..1ca8c9fb 100644 --- a/src/processor/minidump_processor_unittest.cc +++ b/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[]) { diff --git a/src/processor/testdata/write_av_non_canonical.dmp b/src/processor/testdata/write_av_non_canonical.dmp new file mode 100644 index 00000000..02da25ee Binary files /dev/null and b/src/processor/testdata/write_av_non_canonical.dmp differ