aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorerikchen@chromium.org <erikchen@chromium.org>2015-01-27 01:20:59 +0000
committererikchen@chromium.org <erikchen@chromium.org>2015-01-27 01:20:59 +0000
commit7bebb27fb44920f189310985d96ed7801f59afbb (patch)
tree4488552decfc8604e6d4609ba5d56c81baa1e863
parentFix a source of memory corruption. (diff)
downloadbreakpad-7bebb27fb44920f189310985d96ed7801f59afbb.tar.xz
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
-rw-r--r--src/common/dwarf_cu_to_module.cc28
-rw-r--r--src/common/dwarf_cu_to_module.h3
-rw-r--r--src/common/dwarf_cu_to_module_unittest.cc9
-rw-r--r--src/common/linux/elf_symbols_to_module.cc3
-rw-r--r--src/common/module.cc9
-rw-r--r--src/common/module.h15
-rw-r--r--src/common/module_unittest.cc46
-rw-r--r--src/common/stabs_to_module.cc7
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<void*>(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<Module::Function> 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 omitted>";
+ name = "<name omitted>";
}
- func->address = low_pc_;
+
+ // Create a Module::Function based on the data we've gathered, and
+ // add it to the functions_ list.
+ scoped_ptr<Module::Function> 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<FunctionSet::iterator,bool> 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;
}