From 7bebb27fb44920f189310985d96ed7801f59afbb Mon Sep 17 00:00:00 2001 From: "erikchen@chromium.org" Date: Tue, 27 Jan 2015 01:20:59 +0000 Subject: Fix some fragile code that is likely to cause future memory corruption problems. - The ordering of keys in stl containers cannot change. Make the relevant members const to guarantee this assumption. - Add handling and logging for demangle errors. - Fix a potential double-delete bug if a function passed to AddFunction() is already present. BUG=chromium:449214 R=mark@chromium.org Review URL: https://breakpad.appspot.com/10704002/ git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1415 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/common/dwarf_cu_to_module.cc | 28 ++++++++++++++----- src/common/dwarf_cu_to_module.h | 3 ++ src/common/dwarf_cu_to_module_unittest.cc | 9 +++--- src/common/linux/elf_symbols_to_module.cc | 3 +- src/common/module.cc | 9 ++---- src/common/module.h | 15 +++++++--- src/common/module_unittest.cc | 46 ++++++++++++------------------- src/common/stabs_to_module.cc | 7 ++--- 8 files changed, 62 insertions(+), 58 deletions(-) diff --git a/src/common/dwarf_cu_to_module.cc b/src/common/dwarf_cu_to_module.cc index 4bd7bdd5..aaac058b 100644 --- a/src/common/dwarf_cu_to_module.cc +++ b/src/common/dwarf_cu_to_module.cc @@ -351,9 +351,15 @@ void DwarfCUToModule::GenericDIEHandler::ProcessAttributeString( break; case dwarf2reader::DW_AT_MIPS_linkage_name: { char* demangled = NULL; + int status = -1; #if !defined(__ANDROID__) - demangled = abi::__cxa_demangle(data.c_str(), NULL, NULL, NULL); + demangled = abi::__cxa_demangle(data.c_str(), NULL, NULL, &status); #endif + if (status != 0) { + cu_context_->reporter->DemangleError(data, status); + demangled_name_ = ""; + break; + } if (demangled) { demangled_name_ = AddStringToPool(demangled); free(reinterpret_cast(demangled)); @@ -534,18 +540,19 @@ void DwarfCUToModule::FuncHandler::Finish() { // functions that were never used), but all the ones we're // interested in cover a non-empty range of bytes. if (low_pc_ < high_pc_) { - // Create a Module::Function based on the data we've gathered, and - // add it to the functions_ list. - scoped_ptr func(new Module::Function); // Malformed DWARF may omit the name, but all Module::Functions must // have names. + string name; if (!name_.empty()) { - func->name = name_; + name = name_; } else { cu_context_->reporter->UnnamedFunction(offset_); - func->name = ""; + name = ""; } - func->address = low_pc_; + + // Create a Module::Function based on the data we've gathered, and + // add it to the functions_ list. + scoped_ptr func(new Module::Function(name, low_pc_)); func->size = high_pc_ - low_pc_; func->parameter_size = 0; if (func->address) { @@ -667,6 +674,13 @@ void DwarfCUToModule::WarningReporter::UnnamedFunction(uint64 offset) { filename_.c_str(), offset); } +void DwarfCUToModule::WarningReporter::DemangleError( + const string &input, int error) { + CUHeading(); + fprintf(stderr, "%s: warning: failed to demangle %s with error %d\n", + filename_.c_str(), input.c_str(), error); +} + void DwarfCUToModule::WarningReporter::UnhandledInterCUReference( uint64 offset, uint64 target) { CUHeading(); diff --git a/src/common/dwarf_cu_to_module.h b/src/common/dwarf_cu_to_module.h index ab95485f..fd9c380d 100644 --- a/src/common/dwarf_cu_to_module.h +++ b/src/common/dwarf_cu_to_module.h @@ -199,6 +199,9 @@ class DwarfCUToModule: public dwarf2reader::RootDIEHandler { // link. virtual void UnnamedFunction(uint64 offset); + // __cxa_demangle() failed to demangle INPUT. + virtual void DemangleError(const string &input, int error); + // The DW_FORM_ref_addr at OFFSET to TARGET was not handled because // FilePrivate did not retain the inter-CU specification data. virtual void UnhandledInterCUReference(uint64 offset, uint64 target); diff --git a/src/common/dwarf_cu_to_module_unittest.cc b/src/common/dwarf_cu_to_module_unittest.cc index 5f61a58e..9ff842c8 100644 --- a/src/common/dwarf_cu_to_module_unittest.cc +++ b/src/common/dwarf_cu_to_module_unittest.cc @@ -81,6 +81,7 @@ class MockWarningReporter: public DwarfCUToModule::WarningReporter { MOCK_METHOD1(UncoveredFunction, void(const Module::Function &function)); MOCK_METHOD1(UncoveredLine, void(const Module::Line &line)); MOCK_METHOD1(UnnamedFunction, void(uint64 offset)); + MOCK_METHOD2(DemangleError, void(const string &input, int error)); MOCK_METHOD2(UnhandledInterCUReference, void(uint64 offset, uint64 target)); }; @@ -1712,16 +1713,14 @@ TEST_F(CUErrors, BadCURootDIETag) { // produce) output, so their results need to be checked by hand. struct Reporter: public Test { Reporter() - : reporter("filename", 0x123456789abcdef0ULL) { + : reporter("filename", 0x123456789abcdef0ULL), + function("function name", 0x19c45c30770c1eb0ULL), + file("source file name") { reporter.SetCUName("compilation-unit-name"); - function.name = "function name"; - function.address = 0x19c45c30770c1eb0ULL; function.size = 0x89808a5bdfa0a6a3ULL; function.parameter_size = 0x6a329f18683dcd51ULL; - file.name = "source file name"; - line.address = 0x3606ac6267aebeccULL; line.size = 0x5de482229f32556aULL; line.file = &file; diff --git a/src/common/linux/elf_symbols_to_module.cc b/src/common/linux/elf_symbols_to_module.cc index 82d53dd1..96c64fb6 100644 --- a/src/common/linux/elf_symbols_to_module.cc +++ b/src/common/linux/elf_symbols_to_module.cc @@ -155,9 +155,8 @@ bool ELFSymbolsToModule(const uint8_t *symtab_section, while(!iterator->at_end) { if (ELF32_ST_TYPE(iterator->info) == STT_FUNC && iterator->shndx != SHN_UNDEF) { - Module::Extern *ext = new Module::Extern; + Module::Extern *ext = new Module::Extern(iterator->value); ext->name = SymbolString(iterator->name_offset, strings); - ext->address = iterator->value; module->AddExtern(ext); } ++iterator; diff --git a/src/common/module.cc b/src/common/module.cc index 9a8e64cf..f682e24a 100644 --- a/src/common/module.cc +++ b/src/common/module.cc @@ -80,7 +80,7 @@ void Module::AddFunction(Function *function) { // callers try to add one. assert(!function->name.empty()); std::pair ret = functions_.insert(function); - if (!ret.second) { + if (!ret.second && (*ret.first != function)) { // Free the duplicate that was not inserted because this Module // now owns it. delete function; @@ -98,9 +98,7 @@ void Module::AddStackFrameEntry(StackFrameEntry *stack_frame_entry) { } void Module::AddExtern(Extern *ext) { - Function func; - func.name = ext->name; - func.address = ext->address; + Function func(ext->name, ext->address); // Since parsing debug section and public info are not necessarily // mutually exclusive, check if the symbol has already been read @@ -141,8 +139,7 @@ Module::File *Module::FindFile(const string &name) { FileByNameMap::iterator destiny = files_.lower_bound(&name); if (destiny == files_.end() || *destiny->first != name) { // Repeated string comparison, boo hoo. - File *file = new File; - file->name = name; + File *file = new File(name); file->source_id = -1; destiny = files_.insert(destiny, FileByNameMap::value_type(&file->name, file)); diff --git a/src/common/module.h b/src/common/module.h index 440298f0..65b5595d 100644 --- a/src/common/module.h +++ b/src/common/module.h @@ -74,8 +74,10 @@ class Module { // A source file. struct File { + explicit File(const string &name_input) : name(name_input), source_id(0) {} + // The name of the source file. - string name; + const string name; // The file's source id. The Write member function clears this // field and assigns source ids a fresh, so any value placed here @@ -85,6 +87,9 @@ class Module { // A function. struct Function { + Function(const string &name_input, const Address &address_input) : + name(name_input), address(address_input), size(0), parameter_size(0) {} + // For sorting by address. (Not style-guide compliant, but it's // stupid not to put this in the struct.) static bool CompareByAddress(const Function *x, const Function *y) { @@ -92,10 +97,11 @@ class Module { } // The function's name. - string name; + const string name; // The start address and length of the function's code. - Address address, size; + const Address address; + Address size; // The function's parameter size. Address parameter_size; @@ -120,7 +126,8 @@ class Module { // An exported symbol. struct Extern { - Address address; + explicit Extern(const Address &address_input) : address(address_input) {} + const Address address; string name; }; diff --git a/src/common/module_unittest.cc b/src/common/module_unittest.cc index 5c0c6975..0df2d35c 100644 --- a/src/common/module_unittest.cc +++ b/src/common/module_unittest.cc @@ -54,9 +54,7 @@ static Module::Function *generate_duplicate_function(const string &name) { const Module::Address DUP_SIZE = 0x200b26e605f99071LL; const Module::Address DUP_PARAMETER_SIZE = 0xf14ac4fed48c4a99LL; - Module::Function *function = new(Module::Function); - function->name = name; - function->address = DUP_ADDRESS; + Module::Function *function = new Module::Function(name, DUP_ADDRESS); function->size = DUP_SIZE; function->parameter_size = DUP_PARAMETER_SIZE; return function; @@ -81,9 +79,8 @@ TEST(Write, OneLineFunc) { Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID); Module::File *file = m.FindFile("file_name.cc"); - Module::Function *function = new(Module::Function); - function->name = "function_name"; - function->address = 0xe165bf8023b9d9abLL; + Module::Function *function = new Module::Function( + "function_name", 0xe165bf8023b9d9abLL); function->size = 0x1e4bb0eb1cbf5b09LL; function->parameter_size = 0x772beee89114358aLL; Module::Line line = { 0xe165bf8023b9d9abLL, 0x1e4bb0eb1cbf5b09LL, @@ -110,9 +107,8 @@ TEST(Write, RelativeLoadAddress) { Module::File *file2 = m.FindFile("filename-a.cc"); // A function. - Module::Function *function = new(Module::Function); - function->name = "A_FLIBBERTIJIBBET::a_will_o_the_wisp(a clown)"; - function->address = 0xbec774ea5dd935f3LL; + Module::Function *function = new Module::Function( + "A_FLIBBERTIJIBBET::a_will_o_the_wisp(a clown)", 0xbec774ea5dd935f3LL); function->size = 0x2922088f98d3f6fcLL; function->parameter_size = 0xe5e9aa008bd5f0d0LL; @@ -168,9 +164,8 @@ TEST(Write, OmitUnusedFiles) { Module::File *file3 = m.FindFile("filename3"); // Create a function. - Module::Function *function = new(Module::Function); - function->name = "function_name"; - function->address = 0x9b926d464f0b9384LL; + Module::Function *function = new Module::Function( + "function_name", 0x9b926d464f0b9384LL); function->size = 0x4f524a4ba795e6a6LL; function->parameter_size = 0xbbe8133a6641c9b7LL; @@ -217,9 +212,8 @@ TEST(Write, NoCFI) { Module::File *file1 = m.FindFile("filename.cc"); // A function. - Module::Function *function = new(Module::Function); - function->name = "A_FLIBBERTIJIBBET::a_will_o_the_wisp(a clown)"; - function->address = 0xbec774ea5dd935f3LL; + Module::Function *function = new Module::Function( + "A_FLIBBERTIJIBBET::a_will_o_the_wisp(a clown)", 0xbec774ea5dd935f3LL); function->size = 0x2922088f98d3f6fcLL; function->parameter_size = 0xe5e9aa008bd5f0d0LL; @@ -260,15 +254,13 @@ TEST(Construct, AddFunctions) { Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID); // Two functions. - Module::Function *function1 = new(Module::Function); - function1->name = "_without_form"; - function1->address = 0xd35024aa7ca7da5cLL; + Module::Function *function1 = new Module::Function( + "_without_form", 0xd35024aa7ca7da5cLL); function1->size = 0x200b26e605f99071LL; function1->parameter_size = 0xf14ac4fed48c4a99LL; - Module::Function *function2 = new(Module::Function); - function2->name = "_and_void"; - function2->address = 0x2987743d0b35b13fLL; + Module::Function *function2 = new Module::Function( + "_and_void", 0x2987743d0b35b13fLL); function2->size = 0xb369db048deb3010LL; function2->parameter_size = 0x938e556cb5a79988LL; @@ -443,11 +435,9 @@ TEST(Construct, Externs) { Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID); // Two externs. - Module::Extern *extern1 = new(Module::Extern); - extern1->address = 0xffff; + Module::Extern *extern1 = new Module::Extern(0xffff); extern1->name = "_abc"; - Module::Extern *extern2 = new(Module::Extern); - extern2->address = 0xaaaa; + Module::Extern *extern2 = new Module::Extern(0xaaaa); extern2->name = "_xyz"; m.AddExtern(extern1); @@ -470,11 +460,9 @@ TEST(Construct, DuplicateExterns) { Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID); // Two externs. - Module::Extern *extern1 = new(Module::Extern); - extern1->address = 0xffff; + Module::Extern *extern1 = new Module::Extern(0xffff); extern1->name = "_xyz"; - Module::Extern *extern2 = new(Module::Extern); - extern2->address = 0xffff; + Module::Extern *extern2 = new Module::Extern(0xffff); extern2->name = "_abc"; m.AddExtern(extern1); diff --git a/src/common/stabs_to_module.cc b/src/common/stabs_to_module.cc index e59aebdb..0a83cf21 100644 --- a/src/common/stabs_to_module.cc +++ b/src/common/stabs_to_module.cc @@ -90,9 +90,7 @@ bool StabsToModule::EndCompilationUnit(uint64_t address) { bool StabsToModule::StartFunction(const string &name, uint64_t address) { assert(!current_function_); - Module::Function *f = new Module::Function; - f->name = Demangle(name); - f->address = address; + Module::Function *f = new Module::Function(Demangle(name), address); f->size = 0; // We compute this in StabsToModule::Finalize(). f->parameter_size = 0; // We don't provide this information. current_function_ = f; @@ -133,7 +131,7 @@ bool StabsToModule::Line(uint64_t address, const char *name, int number) { } bool StabsToModule::Extern(const string &name, uint64_t address) { - Module::Extern *ext = new Module::Extern; + Module::Extern *ext = new Module::Extern(address); // Older libstdc++ demangle implementations can crash on unexpected // input, so be careful about what gets passed in. if (name.compare(0, 3, "__Z") == 0) { @@ -143,7 +141,6 @@ bool StabsToModule::Extern(const string &name, uint64_t address) { } else { ext->name = name; } - ext->address = address; module_->AddExtern(ext); return true; } -- cgit v1.2.1