From 44384d80b32a5bb361c2ec3bee667f7ccee566d7 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Wed, 30 Jan 2019 09:26:07 +0100 Subject: Always emit a 32-bit crash address for 32-bit architectures Certain minidumps for 32-bit crashes have the upper 32-bit of the crash address (which is a 64-bit value) set to non-zero values. This caused a crash address with more than 32-bits to be printed out for minidumps of 32-bit architectures. This patch masks out those bits when reading the raw minidump data to ensure this doesn't happen anymore. Bug: google-breakpad:783 Change-Id: Ieef6dff759fd0ee2efc47c4c4a3cf863a48f0659 Reviewed-on: https://chromium-review.googlesource.com/c/1427819 Reviewed-by: Ted Mielczarek --- src/processor/minidump_processor.cc | 26 ++++++++++++++ src/processor/minidump_processor_unittest.cc | 38 ++++++++++++++++----- .../testdata/minidump_32bit_crash_addr.dmp | Bin 0 -> 11317 bytes 3 files changed, 56 insertions(+), 8 deletions(-) create mode 100644 src/processor/testdata/minidump_32bit_crash_addr.dmp (limited to 'src/processor') diff --git a/src/processor/minidump_processor.cc b/src/processor/minidump_processor.cc index 2b40cf96..e3c3562c 100644 --- a/src/processor/minidump_processor.cc +++ b/src/processor/minidump_processor.cc @@ -355,6 +355,26 @@ static const MDRawSystemInfo* GetSystemInfo(Minidump *dump, return minidump_system_info->system_info(); } +static uint64_t GetAddressForArchitecture(const MDCPUArchitecture architecture, + size_t raw_address) +{ + switch (architecture) { + case MD_CPU_ARCHITECTURE_X86: + case MD_CPU_ARCHITECTURE_MIPS: + case MD_CPU_ARCHITECTURE_PPC: + case MD_CPU_ARCHITECTURE_SHX: + case MD_CPU_ARCHITECTURE_ARM: + case MD_CPU_ARCHITECTURE_X86_WIN64: + // 32-bit architectures, mask the upper bits. + return raw_address & 0xffffffffULL; + + default: + // All other architectures either have 64-bit pointers or it's impossible + // to tell from the minidump (e.g. MSIL or SPARC) so use 64-bits anyway. + return raw_address; + } +} + // Extract CPU info string from ARM-specific MDRawSystemInfo structure. // raw_info: pointer to source MDRawSystemInfo. // cpu_info: address of target string, cpu info text will be appended to it. @@ -1637,6 +1657,12 @@ string MinidumpProcessor::GetCrashReason(Minidump *dump, uint64_t *address) { } } + if (address) { + *address = GetAddressForArchitecture( + static_cast(raw_system_info->processor_architecture), + *address); + } + return reason; } diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc index 83682a51..a4ac3685 100644 --- a/src/processor/minidump_processor_unittest.cc +++ b/src/processor/minidump_processor_unittest.cc @@ -203,6 +203,12 @@ static const char *kSystemInfoCPUInfo = #define ASSERT_EQ_ABORT(e1, e2) ASSERT_TRUE_ABORT((e1) == (e2)) +static string GetTestDataPath() { + char *srcdir = getenv("srcdir"); + + return string(srcdir ? srcdir : ".") + "/src/processor/testdata/"; +} + class TestSymbolSupplier : public SymbolSupplier { public: TestSymbolSupplier() : interrupt_(false) {} @@ -249,10 +255,8 @@ SymbolSupplier::SymbolResult TestSymbolSupplier::GetSymbolFile( } if (module && module->code_file() == "c:\\test_app.exe") { - *symbol_file = string(getenv("srcdir") ? getenv("srcdir") : ".") + - "/src/processor/testdata/symbols/test_app.pdb/" + - module->debug_identifier() + - "/test_app.sym"; + *symbol_file = GetTestDataPath() + "symbols/test_app.pdb/" + + module->debug_identifier() + "/test_app.sym"; return FOUND; } @@ -460,8 +464,7 @@ TEST_F(MinidumpProcessorTest, TestSymbolSupplierLookupCounts) { BasicSourceLineResolver resolver; MinidumpProcessor processor(&supplier, &resolver); - string minidump_file = string(getenv("srcdir") ? getenv("srcdir") : ".") + - "/src/processor/testdata/minidump2.dmp"; + string minidump_file = GetTestDataPath() + "minidump2.dmp"; ProcessState state; EXPECT_CALL(supplier, GetCStringSymbolData( Property(&google_breakpad::CodeModule::code_file, @@ -501,8 +504,7 @@ TEST_F(MinidumpProcessorTest, TestBasicProcessing) { BasicSourceLineResolver resolver; MinidumpProcessor processor(&supplier, &resolver); - string minidump_file = string(getenv("srcdir") ? getenv("srcdir") : ".") + - "/src/processor/testdata/minidump2.dmp"; + string minidump_file = GetTestDataPath() + "minidump2.dmp"; ProcessState state; ASSERT_EQ(processor.Process(minidump_file, &state), @@ -739,6 +741,26 @@ TEST_F(MinidumpProcessorTest, TestThreadMissingContext) { ASSERT_EQ(0U, state.threads()->at(0)->frames()->size()); } +TEST_F(MinidumpProcessorTest, Test32BitCrashingAddress) { + TestSymbolSupplier supplier; + BasicSourceLineResolver resolver; + MinidumpProcessor processor(&supplier, &resolver); + + string minidump_file = GetTestDataPath() + "minidump_32bit_crash_addr.dmp"; + + ProcessState state; + ASSERT_EQ(processor.Process(minidump_file, &state), + google_breakpad::PROCESS_OK); + ASSERT_EQ(state.system_info()->os, kSystemInfoOS); + ASSERT_EQ(state.system_info()->os_short, kSystemInfoOSShort); + ASSERT_EQ(state.system_info()->os_version, kSystemInfoOSVersion); + ASSERT_EQ(state.system_info()->cpu, kSystemInfoCPU); + ASSERT_EQ(state.system_info()->cpu_info, kSystemInfoCPUInfo); + ASSERT_TRUE(state.crashed()); + ASSERT_EQ(state.crash_reason(), "EXCEPTION_ACCESS_VIOLATION_WRITE"); + ASSERT_EQ(state.crash_address(), 0x45U); +} + } // namespace int main(int argc, char *argv[]) { diff --git a/src/processor/testdata/minidump_32bit_crash_addr.dmp b/src/processor/testdata/minidump_32bit_crash_addr.dmp new file mode 100644 index 00000000..78becc6f Binary files /dev/null and b/src/processor/testdata/minidump_32bit_crash_addr.dmp differ -- cgit v1.2.1