From 0924d424e444d57dd95c647652a11f2d655c11a0 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 19 Jan 2017 11:18:41 -0800 Subject: [PATCH] Populate stack frames with unloaded module info. This CL hits lots of source files because: 1. An update to the CodeModule virtual class. I added an is_loaded method to specify whether the module is loaded. There were several mocks/test classes that needed to be updated with an implementation. An alternative to this route would be to modify MinidumpUnloadedModule::code_file to prepend "Unloaded_" to the module name. 2. Added an unloaded_modules parameter to StackFrameSymbolizer::FillSourceLineInfo. BUG= Change-Id: Ic9c7f7c7b7e932a154a5d4ccf292c1527d8da09f Reviewed-on: https://chromium-review.googlesource.com/430241 Reviewed-by: Ivan Penkov --- src/google_breakpad/processor/code_module.h | 3 + src/google_breakpad/processor/minidump.h | 2 + src/google_breakpad/processor/process_state.h | 7 +- .../processor/stack_frame_symbolizer.h | 8 +- src/google_breakpad/processor/stackwalker.h | 7 ++ src/processor/basic_code_module.h | 11 ++- .../basic_source_line_resolver_unittest.cc | 1 + .../fast_source_line_resolver_unittest.cc | 1 + src/processor/microdump_processor.cc | 1 + src/processor/minidump_processor.cc | 7 ++ src/processor/minidump_processor_unittest.cc | 99 +++++++++++++++++++ src/processor/process_state.cc | 1 + src/processor/stack_frame_symbolizer.cc | 11 ++- src/processor/stackwalker.cc | 14 ++- src/processor/stackwalker_unittest_utils.h | 1 + 15 files changed, 161 insertions(+), 13 deletions(-) diff --git a/src/google_breakpad/processor/code_module.h b/src/google_breakpad/processor/code_module.h index b139907c..29b8d9c9 100644 --- a/src/google_breakpad/processor/code_module.h +++ b/src/google_breakpad/processor/code_module.h @@ -94,6 +94,9 @@ class CodeModule { // should always reflect the original values (reported in the minidump). virtual uint64_t shrink_down_delta() const = 0; virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) = 0; + + // Whether the module was unloaded from memory. + virtual bool is_unloaded() const = 0; }; } // namespace google_breakpad diff --git a/src/google_breakpad/processor/minidump.h b/src/google_breakpad/processor/minidump.h index 1bfc7d9e..bff38bf3 100644 --- a/src/google_breakpad/processor/minidump.h +++ b/src/google_breakpad/processor/minidump.h @@ -399,6 +399,7 @@ class MinidumpModule : public MinidumpObject, virtual string debug_identifier() const; virtual string version() const; virtual CodeModule* Copy() const; + virtual bool is_unloaded() const { return false; } // Getter and setter for shrink_down_delta. This is used when the address // range for a module is shrunk down due to address range conflicts with @@ -775,6 +776,7 @@ class MinidumpUnloadedModule : public MinidumpObject, string debug_identifier() const override; string version() const override; CodeModule* Copy() const override; + bool is_unloaded() const override { return true; } uint64_t shrink_down_delta() const override; void SetShrinkDownDelta(uint64_t shrink_down_delta) override; diff --git a/src/google_breakpad/processor/process_state.h b/src/google_breakpad/processor/process_state.h index 9f12b0c6..21bef42c 100644 --- a/src/google_breakpad/processor/process_state.h +++ b/src/google_breakpad/processor/process_state.h @@ -91,7 +91,7 @@ enum ExploitabilityRating { class ProcessState { public: - ProcessState() : modules_(NULL) { Clear(); } + ProcessState() : modules_(NULL), unloaded_modules_(NULL) { Clear(); } ~ProcessState(); // Resets the ProcessState to its default values @@ -111,6 +111,7 @@ class ProcessState { } const SystemInfo* system_info() const { return &system_info_; } const CodeModules* modules() const { return modules_; } + const CodeModules* unloaded_modules() const { return unloaded_modules_; } const vector >* shrunk_range_modules() const { return &shrunk_range_modules_; } @@ -177,6 +178,10 @@ class ProcessState { // ProcessState. const CodeModules *modules_; + // The modules that have been unloaded from the process represented by the + // ProcessState. + const CodeModules *unloaded_modules_; + // The modules which virtual address ranges were shrunk down due to // virtual address conflicts. vector > shrunk_range_modules_; diff --git a/src/google_breakpad/processor/stack_frame_symbolizer.h b/src/google_breakpad/processor/stack_frame_symbolizer.h index 074907cb..0bbaae0a 100644 --- a/src/google_breakpad/processor/stack_frame_symbolizer.h +++ b/src/google_breakpad/processor/stack_frame_symbolizer.h @@ -75,9 +75,11 @@ class StackFrameSymbolizer { // Encapsulate the step of resolving source line info for a stack frame. // "frame" must not be NULL. - virtual SymbolizerResult FillSourceLineInfo(const CodeModules* modules, - const SystemInfo* system_info, - StackFrame* stack_frame); + virtual SymbolizerResult FillSourceLineInfo( + const CodeModules* modules, + const CodeModules* unloaded_modules, + const SystemInfo* system_info, + StackFrame* stack_frame); virtual WindowsFrameInfo* FindWindowsFrameInfo(const StackFrame* frame); diff --git a/src/google_breakpad/processor/stackwalker.h b/src/google_breakpad/processor/stackwalker.h index a1bd3e7f..4378f75b 100644 --- a/src/google_breakpad/processor/stackwalker.h +++ b/src/google_breakpad/processor/stackwalker.h @@ -89,8 +89,10 @@ class Stackwalker { DumpContext* context, MemoryRegion* memory, const CodeModules* modules, + const CodeModules* unloaded_modules, StackFrameSymbolizer* resolver_helper); + static void set_max_frames(uint32_t max_frames) { max_frames_ = max_frames; max_frames_set_ = true; @@ -189,6 +191,11 @@ class Stackwalker { // This field is optional and may be NULL. const CodeModules* modules_; + // A list of unloaded modules, for populating frames which aren't matched + // to any loaded modules. + // This field is optional and may be NULL. + const CodeModules* unloaded_modules_; + protected: // The StackFrameSymbolizer implementation. StackFrameSymbolizer* frame_symbolizer_; diff --git a/src/processor/basic_code_module.h b/src/processor/basic_code_module.h index 0f7b3e43..35d66a60 100644 --- a/src/processor/basic_code_module.h +++ b/src/processor/basic_code_module.h @@ -62,14 +62,16 @@ class BasicCodeModule : public CodeModule { code_identifier_(that->code_identifier()), debug_file_(that->debug_file()), debug_identifier_(that->debug_identifier()), - version_(that->version()) {} + version_(that->version()), + is_unloaded_(that->is_unloaded()) {} BasicCodeModule(uint64_t base_address, uint64_t size, const string &code_file, const string &code_identifier, const string &debug_file, const string &debug_identifier, - const string &version) + const string &version, + const bool is_unloaded = false) : base_address_(base_address), size_(size), shrink_down_delta_(0), @@ -77,7 +79,8 @@ class BasicCodeModule : public CodeModule { code_identifier_(code_identifier), debug_file_(debug_file), debug_identifier_(debug_identifier), - version_(version) + version_(version), + is_unloaded_(is_unloaded) {} virtual ~BasicCodeModule() {} @@ -95,6 +98,7 @@ class BasicCodeModule : public CodeModule { virtual string debug_identifier() const { return debug_identifier_; } virtual string version() const { return version_; } virtual CodeModule* Copy() const { return new BasicCodeModule(this); } + virtual bool is_unloaded() const { return is_unloaded_; } private: uint64_t base_address_; @@ -105,6 +109,7 @@ class BasicCodeModule : public CodeModule { string debug_file_; string debug_identifier_; string version_; + bool is_unloaded_; // Disallow copy constructor and assignment operator. BasicCodeModule(const BasicCodeModule &that); diff --git a/src/processor/basic_source_line_resolver_unittest.cc b/src/processor/basic_source_line_resolver_unittest.cc index a75044c7..9fab8ca6 100644 --- a/src/processor/basic_source_line_resolver_unittest.cc +++ b/src/processor/basic_source_line_resolver_unittest.cc @@ -71,6 +71,7 @@ class TestCodeModule : public CodeModule { virtual CodeModule* Copy() const { return new TestCodeModule(code_file_); } + virtual bool is_unloaded() const { return false; } virtual uint64_t shrink_down_delta() const { return 0; } virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) {} diff --git a/src/processor/fast_source_line_resolver_unittest.cc b/src/processor/fast_source_line_resolver_unittest.cc index c7215228..87b13c52 100644 --- a/src/processor/fast_source_line_resolver_unittest.cc +++ b/src/processor/fast_source_line_resolver_unittest.cc @@ -82,6 +82,7 @@ class TestCodeModule : public CodeModule { virtual CodeModule* Copy() const { return new TestCodeModule(code_file_); } + virtual bool is_unloaded() const { return false; } virtual uint64_t shrink_down_delta() const { return 0; } virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) {} diff --git a/src/processor/microdump_processor.cc b/src/processor/microdump_processor.cc index 366e3f30..2d67f861 100644 --- a/src/processor/microdump_processor.cc +++ b/src/processor/microdump_processor.cc @@ -73,6 +73,7 @@ ProcessResult MicrodumpProcessor::Process(const string µdump_contents, microdump.GetContext(), microdump.GetMemory(), process_state->modules_, + /* unloaded_modules= */ NULL, frame_symbolizer_)); scoped_ptr stack(new CallStack()); diff --git a/src/processor/minidump_processor.cc b/src/processor/minidump_processor.cc index e80ebc38..c522adff 100644 --- a/src/processor/minidump_processor.cc +++ b/src/processor/minidump_processor.cc @@ -141,6 +141,12 @@ ProcessResult MinidumpProcessor::Process( } } + MinidumpUnloadedModuleList *unloaded_module_list = + dump->GetUnloadedModuleList(); + if (unloaded_module_list) { + process_state->unloaded_modules_ = unloaded_module_list->Copy(); + } + MinidumpMemoryList *memory_list = dump->GetMemoryList(); if (memory_list) { BPLOG(INFO) << "Found " << memory_list->region_count() @@ -262,6 +268,7 @@ ProcessResult MinidumpProcessor::Process( context, thread_memory, process_state->modules_, + process_state->unloaded_modules_, frame_symbolizer_)); scoped_ptr stack(new CallStack()); diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc index d43c1fc9..d9d974df 100644 --- a/src/processor/minidump_processor_unittest.cc +++ b/src/processor/minidump_processor_unittest.cc @@ -71,9 +71,24 @@ class MockMinidump : public Minidump { MOCK_METHOD0(GetException, MinidumpException*()); MOCK_METHOD0(GetAssertion, MinidumpAssertion*()); MOCK_METHOD0(GetModuleList, MinidumpModuleList*()); + MOCK_METHOD0(GetUnloadedModuleList, MinidumpUnloadedModuleList*()); MOCK_METHOD0(GetMemoryList, MinidumpMemoryList*()); }; +class MockMinidumpUnloadedModule : public MinidumpUnloadedModule { + public: + MockMinidumpUnloadedModule() : MinidumpUnloadedModule(NULL) {} +}; + +class MockMinidumpUnloadedModuleList : public MinidumpUnloadedModuleList { + public: + MockMinidumpUnloadedModuleList() : MinidumpUnloadedModuleList(NULL) {} + + MOCK_CONST_METHOD0(Copy, CodeModules*()); + MOCK_CONST_METHOD1(GetModuleForAddress, + const MinidumpUnloadedModule*(uint64_t)); +}; + class MockMinidumpThreadList : public MinidumpThreadList { public: MockMinidumpThreadList() : MinidumpThreadList(NULL) {} @@ -157,6 +172,8 @@ using google_breakpad::MockMinidumpMemoryList; using google_breakpad::MockMinidumpMemoryRegion; using google_breakpad::MockMinidumpThread; using google_breakpad::MockMinidumpThreadList; +using google_breakpad::MockMinidumpUnloadedModule; +using google_breakpad::MockMinidumpUnloadedModuleList; using google_breakpad::ProcessState; using google_breakpad::scoped_ptr; using google_breakpad::SymbolSupplier; @@ -320,6 +337,88 @@ class TestMinidumpContext : public MinidumpContext { class MinidumpProcessorTest : public ::testing::Test { }; +TEST_F(MinidumpProcessorTest, TestUnloadedModules) { + MockMinidump dump; + + EXPECT_CALL(dump, path()).WillRepeatedly(Return("mock minidump")); + EXPECT_CALL(dump, Read()).WillRepeatedly(Return(true)); + + MDRawHeader fake_header; + fake_header.time_date_stamp = 0; + EXPECT_CALL(dump, header()).WillRepeatedly(Return(&fake_header)); + + MDRawSystemInfo raw_system_info; + memset(&raw_system_info, 0, sizeof(raw_system_info)); + raw_system_info.processor_architecture = MD_CPU_ARCHITECTURE_X86; + raw_system_info.platform_id = MD_OS_WIN32_NT; + TestMinidumpSystemInfo dump_system_info(raw_system_info); + + EXPECT_CALL(dump, GetSystemInfo()). + WillRepeatedly(Return(&dump_system_info)); + + // No loaded modules + + MockMinidumpUnloadedModuleList unloaded_module_list; + EXPECT_CALL(dump, GetUnloadedModuleList()). + WillOnce(Return(&unloaded_module_list)); + + MockMinidumpMemoryList memory_list; + EXPECT_CALL(dump, GetMemoryList()). + WillOnce(Return(&memory_list)); + + MockMinidumpThreadList thread_list; + EXPECT_CALL(dump, GetThreadList()). + WillOnce(Return(&thread_list)); + + EXPECT_CALL(thread_list, thread_count()). + WillRepeatedly(Return(1)); + + MockMinidumpThread thread; + EXPECT_CALL(thread_list, GetThreadAtIndex(0)). + WillOnce(Return(&thread)); + + EXPECT_CALL(thread, GetThreadID(_)). + WillRepeatedly(DoAll(SetArgumentPointee<0>(1), + Return(true))); + + MDRawContextX86 thread_raw_context; + memset(&thread_raw_context, 0, + sizeof(thread_raw_context)); + thread_raw_context.context_flags = MD_CONTEXT_X86_FULL; + const uint32_t kExpectedEIP = 0xabcd1234; + thread_raw_context.eip = kExpectedEIP; + TestMinidumpContext thread_context(thread_raw_context); + EXPECT_CALL(thread, GetContext()). + WillRepeatedly(Return(&thread_context)); + + // The memory contents don't really matter here, since it won't be used. + MockMinidumpMemoryRegion thread_memory(0x1234, "xxx"); + EXPECT_CALL(thread, GetMemory()). + WillRepeatedly(Return(&thread_memory)); + EXPECT_CALL(thread, GetStartOfStackMemoryRange()). + Times(0); + EXPECT_CALL(memory_list, GetMemoryRegionForAddress(_)). + Times(0); + + EXPECT_CALL(unloaded_module_list, Copy()). + WillOnce(Return(&unloaded_module_list)); + + MockMinidumpUnloadedModule unloaded_module; + EXPECT_CALL(unloaded_module_list, GetModuleForAddress(kExpectedEIP)). + WillOnce(Return(&unloaded_module)); + + MinidumpProcessor processor(reinterpret_cast(NULL), NULL); + ProcessState state; + EXPECT_EQ(processor.Process(&dump, &state), + google_breakpad::PROCESS_OK); + + // The single frame should be populated with the unloaded module. + ASSERT_EQ(1U, state.threads()->size()); + ASSERT_EQ(1U, state.threads()->at(0)->frames()->size()); + ASSERT_EQ(kExpectedEIP, state.threads()->at(0)->frames()->at(0)->instruction); + ASSERT_EQ(&unloaded_module, state.threads()->at(0)->frames()->at(0)->module); +} + TEST_F(MinidumpProcessorTest, TestCorruptMinidumps) { MockMinidump dump; TestSymbolSupplier supplier; diff --git a/src/processor/process_state.cc b/src/processor/process_state.cc index 5a5cd7f6..90ae93d7 100644 --- a/src/processor/process_state.cc +++ b/src/processor/process_state.cc @@ -64,6 +64,7 @@ void ProcessState::Clear() { modules_with_corrupt_symbols_.clear(); delete modules_; modules_ = NULL; + unloaded_modules_ = NULL; } } // namespace google_breakpad diff --git a/src/processor/stack_frame_symbolizer.cc b/src/processor/stack_frame_symbolizer.cc index 5c8dbe5e..7a44f243 100644 --- a/src/processor/stack_frame_symbolizer.cc +++ b/src/processor/stack_frame_symbolizer.cc @@ -55,12 +55,19 @@ StackFrameSymbolizer::StackFrameSymbolizer( StackFrameSymbolizer::SymbolizerResult StackFrameSymbolizer::FillSourceLineInfo( const CodeModules* modules, + const CodeModules* unloaded_modules, const SystemInfo* system_info, StackFrame* frame) { assert(frame); - if (!modules) return kError; - const CodeModule* module = modules->GetModuleForAddress(frame->instruction); + const CodeModule* module = NULL; + if (modules) { + module = modules->GetModuleForAddress(frame->instruction); + } + if (!module && unloaded_modules) { + module = unloaded_modules->GetModuleForAddress(frame->instruction); + } + if (!module) return kError; frame->module = module; diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc index 98cb0b5b..c85ce5e8 100644 --- a/src/processor/stackwalker.cc +++ b/src/processor/stackwalker.cc @@ -72,6 +72,7 @@ Stackwalker::Stackwalker(const SystemInfo* system_info, : system_info_(system_info), memory_(memory), modules_(modules), + unloaded_modules_(NULL), frame_symbolizer_(frame_symbolizer) { assert(frame_symbolizer_); } @@ -134,8 +135,9 @@ bool Stackwalker::Walk( // Resolve the module information, if a module map was provided. StackFrameSymbolizer::SymbolizerResult symbolizer_result = - frame_symbolizer_->FillSourceLineInfo(modules_, system_info_, - frame.get()); + frame_symbolizer_->FillSourceLineInfo(modules_, unloaded_modules_, + system_info_, + frame.get()); switch (symbolizer_result) { case StackFrameSymbolizer::kInterrupt: BPLOG(INFO) << "Stack walk is interrupted."; @@ -186,13 +188,13 @@ bool Stackwalker::Walk( return true; } - // static Stackwalker* Stackwalker::StackwalkerForCPU( const SystemInfo* system_info, DumpContext* context, MemoryRegion* memory, const CodeModules* modules, + const CodeModules* unloaded_modules, StackFrameSymbolizer* frame_symbolizer) { if (!context) { BPLOG(ERROR) << "Can't choose a stackwalker implementation without context"; @@ -263,6 +265,9 @@ Stackwalker* Stackwalker::StackwalkerForCPU( BPLOG_IF(ERROR, !cpu_stackwalker) << "Unknown CPU type " << HexString(cpu) << ", can't choose a stackwalker " "implementation"; + if (cpu_stackwalker) { + cpu_stackwalker->unloaded_modules_ = unloaded_modules; + } return cpu_stackwalker; } @@ -270,7 +275,8 @@ bool Stackwalker::InstructionAddressSeemsValid(uint64_t address) { StackFrame frame; frame.instruction = address; StackFrameSymbolizer::SymbolizerResult symbolizer_result = - frame_symbolizer_->FillSourceLineInfo(modules_, system_info_, &frame); + frame_symbolizer_->FillSourceLineInfo(modules_, unloaded_modules_, + system_info_, &frame); if (!frame.module) { // not inside any loaded module diff --git a/src/processor/stackwalker_unittest_utils.h b/src/processor/stackwalker_unittest_utils.h index 1523b247..d7f34755 100644 --- a/src/processor/stackwalker_unittest_utils.h +++ b/src/processor/stackwalker_unittest_utils.h @@ -118,6 +118,7 @@ class MockCodeModule: public google_breakpad::CodeModule { google_breakpad::CodeModule *Copy() const { abort(); // Tests won't use this. } + virtual bool is_unloaded() const { return false; } virtual uint64_t shrink_down_delta() const { return 0; } virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) {}