From 24f5931c5e0120982c0cbf1896641e3ef2bdd52f Mon Sep 17 00:00:00 2001 From: Ivan Penkov Date: Mon, 20 Jun 2016 11:14:47 -0700 Subject: Server-side workaround to handle overlapping modules. This change is resolving an issue that was caused by the combination of: - Android system libraries being relro packed in N+. - Breakpad dealing with relro packed libraries in a hack way. This is a fix for http://crbug/611824. I also found an use-after-free issue (bug in Minidump::SeekToStreamType). I disallowed the MinidumpStreamInfo copy and assign constructors and the compiler detected another similar issue in Minidump::Print. Then I disabled the copy and assign constructors for most classes in minidump.h (just in case). There are a couple of classes where I couldn't disallow them (since assign is used). This will require a small refactor so I left it out of this CL. R=mark@chromium.org Review URL: https://codereview.chromium.org/2060663002 . --- src/processor/basic_code_module.h | 33 +++++++++------- src/processor/basic_code_modules.cc | 44 +++++++++++++++++---- src/processor/basic_code_modules.h | 9 +++++ .../basic_source_line_resolver_unittest.cc | 4 +- .../fast_source_line_resolver_unittest.cc | 4 +- src/processor/microdump.cc | 4 ++ src/processor/microdump_processor_unittest.cc | 8 ++-- src/processor/minidump.cc | 45 ++++++++++++++++++++-- src/processor/minidump_processor.cc | 14 ++++++- src/processor/range_map-inl.h | 5 +++ src/processor/range_map.h | 1 + src/processor/stackwalker_unittest_utils.h | 23 ++++++++--- 12 files changed, 158 insertions(+), 36 deletions(-) (limited to 'src/processor') diff --git a/src/processor/basic_code_module.h b/src/processor/basic_code_module.h index 3fe782bb..0f7b3e43 100644 --- a/src/processor/basic_code_module.h +++ b/src/processor/basic_code_module.h @@ -57,6 +57,7 @@ class BasicCodeModule : public CodeModule { explicit BasicCodeModule(const CodeModule *that) : base_address_(that->base_address()), size_(that->size()), + shrink_down_delta_(that->shrink_down_delta()), code_file_(that->code_file()), code_identifier_(that->code_identifier()), debug_file_(that->debug_file()), @@ -64,18 +65,19 @@ class BasicCodeModule : public CodeModule { version_(that->version()) {} 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) - : base_address_(base_address), - size_(size), - code_file_(code_file), - code_identifier_(code_identifier), - debug_file_(debug_file), - debug_identifier_(debug_identifier), - version_(version) + const string &code_file, + const string &code_identifier, + const string &debug_file, + const string &debug_identifier, + const string &version) + : base_address_(base_address), + size_(size), + shrink_down_delta_(0), + code_file_(code_file), + code_identifier_(code_identifier), + debug_file_(debug_file), + debug_identifier_(debug_identifier), + version_(version) {} virtual ~BasicCodeModule() {} @@ -83,16 +85,21 @@ class BasicCodeModule : public CodeModule { // members. virtual uint64_t base_address() const { return base_address_; } virtual uint64_t size() const { return size_; } + virtual uint64_t shrink_down_delta() const { return shrink_down_delta_; } + virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) { + shrink_down_delta_ = shrink_down_delta; + } virtual string code_file() const { return code_file_; } virtual string code_identifier() const { return code_identifier_; } virtual string debug_file() const { return debug_file_; } virtual string debug_identifier() const { return debug_identifier_; } virtual string version() const { return version_; } - virtual const CodeModule* Copy() const { return new BasicCodeModule(this); } + virtual CodeModule* Copy() const { return new BasicCodeModule(this); } private: uint64_t base_address_; uint64_t size_; + uint64_t shrink_down_delta_; string code_file_; string code_identifier_; string debug_file_; diff --git a/src/processor/basic_code_modules.cc b/src/processor/basic_code_modules.cc index 00dc6fcb..48d97167 100644 --- a/src/processor/basic_code_modules.cc +++ b/src/processor/basic_code_modules.cc @@ -38,6 +38,8 @@ #include +#include + #include "google_breakpad/processor/code_module.h" #include "processor/linked_ptr.h" #include "processor/logging.h" @@ -45,31 +47,50 @@ namespace google_breakpad { +using std::vector; + BasicCodeModules::BasicCodeModules(const CodeModules *that) : main_address_(0), map_() { BPLOG_IF(ERROR, !that) << "BasicCodeModules::BasicCodeModules requires " "|that|"; assert(that); + map_.SetEnableShrinkDown(that->IsModuleShrinkEnabled()); + const CodeModule *main_module = that->GetMainModule(); if (main_module) main_address_ = main_module->base_address(); unsigned int count = that->module_count(); - for (unsigned int module_sequence = 0; - module_sequence < count; - ++module_sequence) { + for (unsigned int i = 0; i < count; ++i) { // Make a copy of the module and insert it into the map. Use // GetModuleAtIndex because ordering is unimportant when slurping the // entire list, and GetModuleAtIndex may be faster than // GetModuleAtSequence. - linked_ptr module( - that->GetModuleAtIndex(module_sequence)->Copy()); + linked_ptr module(that->GetModuleAtIndex(i)->Copy()); if (!map_.StoreRange(module->base_address(), module->size(), module)) { - BPLOG(ERROR) << "Module " << module->code_file() << - " could not be stored"; + BPLOG(ERROR) << "Module " << module->code_file() + << " could not be stored"; + } + } + + // Report modules with shrunk ranges. + for (unsigned int i = 0; i < count; ++i) { + linked_ptr module(that->GetModuleAtIndex(i)->Copy()); + uint64_t delta = 0; + if (map_.RetrieveRange(module->base_address() + module->size() - 1, + &module, NULL /* base */, &delta, NULL /* size */) && + delta > 0) { + BPLOG(INFO) << "The range for module " << module->code_file() + << " was shrunk down by " << HexString(delta) << " bytes."; + linked_ptr shrunk_range_module(module->Copy()); + shrunk_range_module->SetShrinkDownDelta(delta); + shrunk_range_modules_.push_back(shrunk_range_module); } } + + // TODO(ivanpe): Report modules with conflicting ranges. The list of such + // modules should be copied from |that|. } BasicCodeModules::BasicCodeModules() : main_address_(0), map_() { } @@ -122,4 +143,13 @@ const CodeModules* BasicCodeModules::Copy() const { return new BasicCodeModules(this); } +vector > +BasicCodeModules::GetShrunkRangeModules() const { + return shrunk_range_modules_; +} + +bool BasicCodeModules::IsModuleShrinkEnabled() const { + return map_.IsShrinkDownEnabled(); +} + } // namespace google_breakpad diff --git a/src/processor/basic_code_modules.h b/src/processor/basic_code_modules.h index 97579b4d..50f8a03d 100644 --- a/src/processor/basic_code_modules.h +++ b/src/processor/basic_code_modules.h @@ -43,6 +43,8 @@ #include +#include + #include "google_breakpad/processor/code_modules.h" #include "processor/linked_ptr.h" #include "processor/range_map.h" @@ -67,6 +69,9 @@ class BasicCodeModules : public CodeModules { virtual const CodeModule* GetModuleAtSequence(unsigned int sequence) const; virtual const CodeModule* GetModuleAtIndex(unsigned int index) const; virtual const CodeModules* Copy() const; + virtual std::vector > + GetShrunkRangeModules() const; + virtual bool IsModuleShrinkEnabled() const; protected: BasicCodeModules(); @@ -78,6 +83,10 @@ class BasicCodeModules : public CodeModules { // address range. RangeMap > map_; + // A vector of all CodeModules that were shrunk downs due to + // address range conflicts. + std::vector > shrunk_range_modules_; + private: // Disallow copy constructor and assignment operator. BasicCodeModules(const BasicCodeModules &that); diff --git a/src/processor/basic_source_line_resolver_unittest.cc b/src/processor/basic_source_line_resolver_unittest.cc index 7d4cd5c5..a75044c7 100644 --- a/src/processor/basic_source_line_resolver_unittest.cc +++ b/src/processor/basic_source_line_resolver_unittest.cc @@ -68,9 +68,11 @@ class TestCodeModule : public CodeModule { virtual string debug_file() const { return ""; } virtual string debug_identifier() const { return ""; } virtual string version() const { return ""; } - virtual const CodeModule* Copy() const { + virtual CodeModule* Copy() const { return new TestCodeModule(code_file_); } + virtual uint64_t shrink_down_delta() const { return 0; } + virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) {} private: string code_file_; diff --git a/src/processor/fast_source_line_resolver_unittest.cc b/src/processor/fast_source_line_resolver_unittest.cc index 72632f84..c7215228 100644 --- a/src/processor/fast_source_line_resolver_unittest.cc +++ b/src/processor/fast_source_line_resolver_unittest.cc @@ -79,9 +79,11 @@ class TestCodeModule : public CodeModule { virtual string debug_file() const { return ""; } virtual string debug_identifier() const { return ""; } virtual string version() const { return ""; } - virtual const CodeModule* Copy() const { + virtual CodeModule* Copy() const { return new TestCodeModule(code_file_); } + virtual uint64_t shrink_down_delta() const { return 0; } + virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) {} private: string code_file_; diff --git a/src/processor/microdump.cc b/src/processor/microdump.cc index 9073fe10..4af62f56 100644 --- a/src/processor/microdump.cc +++ b/src/processor/microdump.cc @@ -110,6 +110,9 @@ void MicrodumpModules::Add(const CodeModule* module) { } } +void MicrodumpModules::SetEnableModuleShrink(bool is_enabled) { + map_.SetEnableShrinkDown(is_enabled); +} // // MicrodumpContext @@ -262,6 +265,7 @@ Microdump::Microdump(const string& contents) } else if (os_id == "A") { system_info_->os = "Android"; system_info_->os_short = "android"; + modules_->SetEnableModuleShrink(true); } // OS line also contains release and version for future use. diff --git a/src/processor/microdump_processor_unittest.cc b/src/processor/microdump_processor_unittest.cc index 898b65b2..af897f7d 100644 --- a/src/processor/microdump_processor_unittest.cc +++ b/src/processor/microdump_processor_unittest.cc @@ -201,7 +201,7 @@ TEST_F(MicrodumpProcessorTest, TestProcessX86) { AnalyzeDump("microdump-x86.dmp", false /* omit_symbols */, 4 /* expected_cpu_count */, &state); - ASSERT_EQ(105U, state.modules()->module_count()); + ASSERT_EQ(124U, state.modules()->module_count()); ASSERT_EQ("x86", state.system_info()->cpu); ASSERT_EQ("asus/WW_Z00A/Z00A:5.0/LRX21V/2.19.40.22_20150627_5104_user:user/" "release-keys", state.system_info()->os_version); @@ -216,11 +216,11 @@ TEST_F(MicrodumpProcessorTest, TestProcessMultiple) { ProcessState state; AnalyzeDump("microdump-multiple.dmp", false /* omit_symbols */, 6 /* expected_cpu_count */, &state); - ASSERT_EQ(133U, state.modules()->module_count()); + ASSERT_EQ(156U, state.modules()->module_count()); ASSERT_EQ("arm", state.system_info()->cpu); ASSERT_EQ("lge/p1_tmo_us/p1:6.0/MRA58K/1603210524c8d:user/release-keys", state.system_info()->os_version); - ASSERT_EQ(2U, state.threads()->at(0)->frames()->size()); + ASSERT_EQ(5U, state.threads()->at(0)->frames()->size()); } TEST_F(MicrodumpProcessorTest, TestProcessMips) { @@ -249,7 +249,7 @@ TEST_F(MicrodumpProcessorTest, TestProcessMips64) { AnalyzeDump("microdump-mips64.dmp", false /* omit_symbols */, 1 /* expected_cpu_count */, &state); - ASSERT_EQ(7U, state.modules()->module_count()); + ASSERT_EQ(8U, state.modules()->module_count()); ASSERT_EQ("mips64", state.system_info()->cpu); ASSERT_EQ("3.10.0-gf185e20 #112 PREEMPT Mon Oct 5 11:12:49 PDT 2015", state.system_info()->os_version); diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc index 81c3d11a..f94a409a 100644 --- a/src/processor/minidump.cc +++ b/src/processor/minidump.cc @@ -2109,11 +2109,21 @@ string MinidumpModule::version() const { } -const CodeModule* MinidumpModule::Copy() const { +CodeModule* MinidumpModule::Copy() const { return new BasicCodeModule(this); } +uint64_t MinidumpModule::shrink_down_delta() const { + return 0; +} + +void MinidumpModule::SetShrinkDownDelta(uint64_t shrink_down_delta) { + // Not implemented + assert(false); +} + + const uint8_t* MinidumpModule::GetCVRecord(uint32_t* size) { if (!module_valid_) { BPLOG(ERROR) << "Invalid MinidumpModule for GetCVRecord"; @@ -2497,6 +2507,7 @@ MinidumpModuleList::MinidumpModuleList(Minidump* minidump) range_map_(new RangeMap()), modules_(NULL), module_count_(0) { + range_map_->SetEnableShrinkDown(minidump_->IsAndroid()); } @@ -2709,7 +2720,7 @@ const MinidumpModule* MinidumpModuleList::GetModuleAtSequence( } unsigned int module_index; - if (!range_map_->RetrieveRangeAtIndex(sequence, &module_index, + if (!range_map_->RetrieveRangeAtIndex(sequence, &module_index, NULL /* base */, NULL /* delta */, NULL /* size */)) { BPLOG(ERROR) << "MinidumpModuleList has no module at sequence " << sequence; @@ -2741,6 +2752,14 @@ const CodeModules* MinidumpModuleList::Copy() const { return new BasicCodeModules(this); } +vector > +MinidumpModuleList::GetShrunkRangeModules() const { + return vector >(); +} + +bool MinidumpModuleList::IsModuleShrinkEnabled() const { + return range_map_->IsShrinkDownEnabled(); +} void MinidumpModuleList::Print() { if (!valid_) { @@ -4532,6 +4551,24 @@ MinidumpLinuxMapsList *Minidump::GetLinuxMapsList() { return GetStream(&linux_maps_list); } +bool Minidump::IsAndroid() { + // Save the current stream position + off_t saved_position = Tell(); + if (saved_position == -1) { + return false; + } + const MDRawSystemInfo* system_info = + GetSystemInfo() ? GetSystemInfo()->system_info() : NULL; + + // Restore position and return + if (!SeekSet(saved_position)) { + BPLOG(ERROR) << "Couldn't seek back to saved position"; + return false; + } + + return system_info && system_info->platform_id == MD_OS_ANDROID; +} + static const char* get_stream_name(uint32_t stream_type) { switch (stream_type) { case MD_UNUSED_STREAM: @@ -4645,7 +4682,7 @@ void Minidump::Print() { iterator != stream_map_->end(); ++iterator) { uint32_t stream_type = iterator->first; - MinidumpStreamInfo info = iterator->second; + const MinidumpStreamInfo& info = iterator->second; printf(" stream type 0x%x (%s) at index %d\n", stream_type, get_stream_name(stream_type), info.stream_index); @@ -4802,7 +4839,7 @@ bool Minidump::SeekToStreamType(uint32_t stream_type, return false; } - MinidumpStreamInfo info = iterator->second; + const MinidumpStreamInfo& info = iterator->second; if (info.stream_index >= header_.stream_count) { BPLOG(ERROR) << "SeekToStreamType: type " << stream_type << " out of range: " << diff --git a/src/processor/minidump_processor.cc b/src/processor/minidump_processor.cc index 06a8916d..3ff19518 100644 --- a/src/processor/minidump_processor.cc +++ b/src/processor/minidump_processor.cc @@ -126,8 +126,20 @@ ProcessResult MinidumpProcessor::Process( // Put a copy of the module list into ProcessState object. This is not // necessarily a MinidumpModuleList, but it adheres to the CodeModules // interface, which is all that ProcessState needs to expose. - if (module_list) + if (module_list) { process_state->modules_ = module_list->Copy(); + process_state->shrunk_range_modules_ = + process_state->modules_->GetShrunkRangeModules(); + for (unsigned int i = 0; + i < process_state->shrunk_range_modules_.size(); + i++) { + linked_ptr module = + process_state->shrunk_range_modules_[i]; + BPLOG(INFO) << "The range for module " << module->code_file() + << " was shrunk down by " << HexString( + module->shrink_down_delta()) << " bytes. "; + } + } MinidumpMemoryList *memory_list = dump->GetMemoryList(); if (memory_list) { diff --git a/src/processor/range_map-inl.h b/src/processor/range_map-inl.h index 082733e1..9fe74c50 100644 --- a/src/processor/range_map-inl.h +++ b/src/processor/range_map-inl.h @@ -52,6 +52,11 @@ void RangeMap::SetEnableShrinkDown( enable_shrink_down_ = enable_shrink_down; } +template +bool RangeMap::IsShrinkDownEnabled() const { + return enable_shrink_down_; +} + template bool RangeMap::StoreRange(const AddressType &base, const AddressType &size, diff --git a/src/processor/range_map.h b/src/processor/range_map.h index 985d992f..d90a6732 100644 --- a/src/processor/range_map.h +++ b/src/processor/range_map.h @@ -60,6 +60,7 @@ class RangeMap { // will be shrunk down by moving its start position to a higher address so // that it does not overlap anymore. void SetEnableShrinkDown(bool enable_shrink_down); + bool IsShrinkDownEnabled() const; // Inserts a range into the map. Returns false for a parameter error, // or if the location of the range would conflict with a range already diff --git a/src/processor/stackwalker_unittest_utils.h b/src/processor/stackwalker_unittest_utils.h index 73ceb199..ee22a8fe 100644 --- a/src/processor/stackwalker_unittest_utils.h +++ b/src/processor/stackwalker_unittest_utils.h @@ -48,6 +48,7 @@ #include "google_breakpad/processor/memory_region.h" #include "google_breakpad/processor/symbol_supplier.h" #include "google_breakpad/processor/system_info.h" +#include "processor/linked_ptr.h" class MockMemoryRegion: public google_breakpad::MemoryRegion { public: @@ -114,9 +115,11 @@ class MockCodeModule: public google_breakpad::CodeModule { string debug_file() const { return code_file_; } string debug_identifier() const { return code_file_; } string version() const { return version_; } - const google_breakpad::CodeModule *Copy() const { + google_breakpad::CodeModule *Copy() const { abort(); // Tests won't use this. } + virtual uint64_t shrink_down_delta() const { return 0; } + virtual void SetShrinkDownDelta(uint64_t shrink_down_delta) {} private: uint64_t base_address_; @@ -126,11 +129,11 @@ class MockCodeModule: public google_breakpad::CodeModule { }; class MockCodeModules: public google_breakpad::CodeModules { - public: + public: typedef google_breakpad::CodeModule CodeModule; typedef google_breakpad::CodeModules CodeModules; - void Add(const MockCodeModule *module) { + void Add(const MockCodeModule *module) { modules_.push_back(module); } @@ -157,9 +160,19 @@ class MockCodeModules: public google_breakpad::CodeModules { return modules_.at(index); } - const CodeModules *Copy() const { abort(); } // Tests won't use this. + CodeModules *Copy() const { abort(); } // Tests won't use this + + virtual std::vector > + GetShrunkRangeModules() const { + return std::vector >(); + } + + // Returns true, if module address range shrink is enabled. + bool IsModuleShrinkEnabled() const { + return false; + } - private: + private: typedef std::vector ModuleVector; ModuleVector modules_; }; -- cgit v1.2.1