diff --git a/src/google_breakpad/processor/basic_source_line_resolver.h b/src/google_breakpad/processor/basic_source_line_resolver.h index 6cf99705..f77b3bbb 100644 --- a/src/google_breakpad/processor/basic_source_line_resolver.h +++ b/src/google_breakpad/processor/basic_source_line_resolver.h @@ -55,6 +55,7 @@ class BasicSourceLineResolver : public SourceLineResolverBase { using SourceLineResolverBase::LoadModule; using SourceLineResolverBase::LoadModuleUsingMapBuffer; using SourceLineResolverBase::LoadModuleUsingMemoryBuffer; + using SourceLineResolverBase::ShouldDeleteMemoryBufferAfterLoadModule; using SourceLineResolverBase::UnloadModule; using SourceLineResolverBase::HasModule; using SourceLineResolverBase::FillSourceLineInfo; @@ -73,16 +74,6 @@ class BasicSourceLineResolver : public SourceLineResolverBase { // Module implements SourceLineResolverBase::Module interface. class Module; - // Helper methods to manage C-String format symbol data. - // See "google_breakpad/processor/source_line_resolver_base.h" for more - // comments about these helper methods. - virtual void DeleteDataAfterLoad(char *symbol_data); - // No-op helper methods. - virtual void DeleteDataUnload(const CodeModule *module) { } - virtual void ClearLocalMemory() { } - virtual void StoreDataBeforeLoad(const CodeModule *module, - char *symbol_data) { } - // Disallow unwanted copy ctor and assignment operator BasicSourceLineResolver(const BasicSourceLineResolver&); void operator=(const BasicSourceLineResolver&); diff --git a/src/google_breakpad/processor/fast_source_line_resolver.h b/src/google_breakpad/processor/fast_source_line_resolver.h index 4bb74f1a..60f6dfce 100644 --- a/src/google_breakpad/processor/fast_source_line_resolver.h +++ b/src/google_breakpad/processor/fast_source_line_resolver.h @@ -84,18 +84,10 @@ class FastSourceLineResolver : public SourceLineResolverBase { // Deserialize raw memory data to construct a WindowsFrameInfo object. static WindowsFrameInfo CopyWFI(const char *raw_memory); - // Helper methods to manage C-String format symbol data. - // See "google_breakpad/processor/source_line_resolver_base.h" for more - // comments about these helper methods. - virtual void StoreDataBeforeLoad(const CodeModule *module, char *symbol_data); - virtual void DeleteDataUnload(const CodeModule *module); - virtual void ClearLocalMemory(); - // No-op helper method. - virtual void DeleteDataAfterLoad(char *symbol_data) { } - - // Store memory data allocated in LoadModule and LoadModuleUsingMapBuffer. - typedef std::map MemoryMap; - MemoryMap memory_chunks_; + // FastSourceLineResolver requires the memory buffer stays alive during the + // lifetime of a corresponding module, therefore it needs to redefine this + // virtual method. + virtual bool ShouldDeleteMemoryBufferAfterLoadModule(); // Disallow unwanted copy ctor and assignment operator FastSourceLineResolver(const FastSourceLineResolver&); diff --git a/src/google_breakpad/processor/network_source_line_resolver.h b/src/google_breakpad/processor/network_source_line_resolver.h index f60ff701..138b2f56 100644 --- a/src/google_breakpad/processor/network_source_line_resolver.h +++ b/src/google_breakpad/processor/network_source_line_resolver.h @@ -84,6 +84,10 @@ class NetworkSourceLineResolver : public SourceLineResolverInterface, virtual bool LoadModuleUsingMemoryBuffer(const CodeModule *module, char *memory_buffer); + // It doesn't matter whether returns true or false, since no memory buffer + // will be allocated in GetCStringSymbolData(). + virtual bool ShouldDeleteMemoryBufferAfterLoadModule() { return true; } + void UnloadModule(const CodeModule *module); virtual bool HasModule(const CodeModule *module); @@ -112,6 +116,11 @@ class NetworkSourceLineResolver : public SourceLineResolverInterface, string *symbol_file, char **symbol_data); + // Delete the data buffer allocated in GetCStringSymbolData(). + // Since the above GetCStringSymbolData() won't allocate any memory at all, + // this method is no-op. + virtual void FreeSymbolData(const CodeModule *module) { } + private: int wait_milliseconds_; // if false, some part of our network setup failed. diff --git a/src/google_breakpad/processor/source_line_resolver_base.h b/src/google_breakpad/processor/source_line_resolver_base.h index 8113e2ed..d950a736 100644 --- a/src/google_breakpad/processor/source_line_resolver_base.h +++ b/src/google_breakpad/processor/source_line_resolver_base.h @@ -73,35 +73,13 @@ class SourceLineResolverBase : public SourceLineResolverInterface { const string &map_buffer); virtual bool LoadModuleUsingMemoryBuffer(const CodeModule *module, char *memory_buffer); + virtual bool ShouldDeleteMemoryBufferAfterLoadModule(); virtual void UnloadModule(const CodeModule *module); virtual bool HasModule(const CodeModule *module); virtual void FillSourceLineInfo(StackFrame *frame); virtual WindowsFrameInfo *FindWindowsFrameInfo(const StackFrame *frame); virtual CFIFrameInfo *FindCFIFrameInfo(const StackFrame *frame); - // Helper methods to manage C-String format symbol data. - // These methods are defined as no-op by default. - // - // StoreDataBeforeLoad() will be called in LoadModule() and - // LoadModuleUsingMapBuffer() to let subclass decide whether or how to store - // the dynamicly allocated memory data, before passing the data to - // LoadModuleUsingMemoryBuffer() which actually loads the module. - virtual void StoreDataBeforeLoad(const CodeModule *module, char *symbol_data); - - // DeleteDataAfterLoad() will be called at the end of - // LoadModuleUsingMemoryBuffer() to let subclass decide whether to delete the - // allocated memory data or not (which depends on whether the subclass has - // ownership of the data or not). - virtual void DeleteDataAfterLoad(char *symbol_data); - - // DeleteDataUnload() will be called in UnloadModule() to let subclass clean - // up dynamicly allocated data associated with the module, if there is any. - virtual void DeleteDataUnload(const CodeModule *module); - - // ClearLocalMemory() will be called in destructor to let subclass clean up - // all local memory data it owns, if there is any. - virtual void ClearLocalMemory(); - // Nested structs and classes. struct Line; struct Function; @@ -113,10 +91,14 @@ class SourceLineResolverBase : public SourceLineResolverInterface { class Module; class AutoFileCloser; - // All of the modules we've loaded + // All of the modules that are loaded. typedef map ModuleMap; ModuleMap *modules_; + // All of heap-allocated buffers that are owned locally by resolver. + typedef std::map MemoryMap; + MemoryMap *memory_buffers_; + // Creates a concrete module at run-time. ModuleFactory *module_factory_; diff --git a/src/google_breakpad/processor/source_line_resolver_interface.h b/src/google_breakpad/processor/source_line_resolver_interface.h index bd6a12d6..103f979e 100644 --- a/src/google_breakpad/processor/source_line_resolver_interface.h +++ b/src/google_breakpad/processor/source_line_resolver_interface.h @@ -67,9 +67,15 @@ class SourceLineResolverInterface { // Add an interface to load symbol using C-String data insteading string. // This is useful in the optimization design for avoiding unnecessary copying // of symbol data, in order to improve memory efficiency. + // LoadModuleUsingMemoryBuffer() does NOT take ownership of memory_buffer. virtual bool LoadModuleUsingMemoryBuffer(const CodeModule *module, char *memory_buffer) = 0; + // Return true if the memory buffer should be deleted immediately after + // LoadModuleUsingMemoryBuffer(). Return false if the memory buffer has to be + // alive during the lifetime of the corresponding Module. + virtual bool ShouldDeleteMemoryBufferAfterLoadModule() = 0; + // Request that the specified module be unloaded from this resolver. // A resolver may choose to ignore such a request. virtual void UnloadModule(const CodeModule *module) = 0; @@ -79,7 +85,7 @@ class SourceLineResolverInterface { // Fills in the function_base, function_name, source_file_name, // and source_line fields of the StackFrame. The instruction and - // module_name fields must already be filled in. + // module_name fields must already be filled in. virtual void FillSourceLineInfo(StackFrame *frame) = 0; // If Windows stack walking information is available covering @@ -87,7 +93,7 @@ class SourceLineResolverInterface { // describing it. If the information is not available, returns NULL. // A NULL return value does not indicate an error. The caller takes // ownership of any returned WindowsFrameInfo object. - virtual WindowsFrameInfo *FindWindowsFrameInfo(const StackFrame *frame) = 0; + virtual WindowsFrameInfo *FindWindowsFrameInfo(const StackFrame *frame) = 0; // If CFI stack walking information is available covering ADDRESS, // return a CFIFrameInfo structure describing it. If the information diff --git a/src/google_breakpad/processor/symbol_supplier.h b/src/google_breakpad/processor/symbol_supplier.h index 4a41688e..26f5d7fa 100644 --- a/src/google_breakpad/processor/symbol_supplier.h +++ b/src/google_breakpad/processor/symbol_supplier.h @@ -76,14 +76,21 @@ class SymbolSupplier { string *symbol_file, string *symbol_data) = 0; - // Same as above, except places symbol data into symbol_data as C-string in - // dynamically allocated memory. Using C-string as type of symbol data enables - // passing data by pointer, and thus avoids unncessary copying of data (to - // improve memory efficiency). + // Same as above, except allocates data buffer on heap and then places the + // symbol data into the buffer as C-string. + // SymbolSupplier is responsible for deleting the data buffer. After the call + // to GetCStringSymbolData(), the caller should call FreeSymbolData(const + // Module *module) once the data buffer is no longer needed. + // If symbol_data is not NULL, symbol supplier won't return FOUND unless it + // returns a valid buffer in symbol_data, e.g., returns INTERRUPT on memory + // allocation failure. virtual SymbolResult GetCStringSymbolData(const CodeModule *module, const SystemInfo *system_info, string *symbol_file, char **symbol_data) = 0; + + // Frees the data buffer allocated for the module in GetCStringSymbolData. + virtual void FreeSymbolData(const CodeModule *module) = 0; }; } // namespace google_breakpad diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc index a795c4ae..ff57140f 100644 --- a/src/processor/basic_source_line_resolver.cc +++ b/src/processor/basic_source_line_resolver.cc @@ -60,11 +60,6 @@ static const char *kWhitespace = " \r\n"; BasicSourceLineResolver::BasicSourceLineResolver() : SourceLineResolverBase(new BasicModuleFactory) { } -void BasicSourceLineResolver::DeleteDataAfterLoad(char *symbol_data) { - // Always delete allocated memory after loading symbol. - delete symbol_data; -} - bool BasicSourceLineResolver::Module::LoadMapFromMemory(char *memory_buffer) { linked_ptr cur_func; int line_number = 0; diff --git a/src/processor/exploitability_unittest.cc b/src/processor/exploitability_unittest.cc index b23c2de7..4de6f1d6 100644 --- a/src/processor/exploitability_unittest.cc +++ b/src/processor/exploitability_unittest.cc @@ -98,6 +98,7 @@ class TestSymbolSupplier : public SymbolSupplier { string *symbol_file, char **symbol_data); + virtual void FreeSymbolData(const CodeModule *module) { } // When set to true, causes the SymbolSupplier to return INTERRUPT void set_interrupt(bool interrupt) { interrupt_ = interrupt; } diff --git a/src/processor/fast_source_line_resolver.cc b/src/processor/fast_source_line_resolver.cc index 5f6cc074..45c1f0f0 100644 --- a/src/processor/fast_source_line_resolver.cc +++ b/src/processor/fast_source_line_resolver.cc @@ -54,26 +54,8 @@ namespace google_breakpad { FastSourceLineResolver::FastSourceLineResolver() : SourceLineResolverBase(new FastModuleFactory) { } -void FastSourceLineResolver::ClearLocalMemory() { - for (MemoryMap::iterator it = memory_chunks_.begin(); - it != memory_chunks_.end(); - ++it) { - delete it->second; - } -} - -void FastSourceLineResolver::DeleteDataUnload(const CodeModule *module) { - if (module) { - MemoryMap::iterator iter = memory_chunks_.find(module->code_file()); - if (iter != memory_chunks_.end()) { - delete iter->second; - } - } -} - -void FastSourceLineResolver::StoreDataBeforeLoad(const CodeModule *module, - char *symbol_data) { - memory_chunks_.insert(make_pair(module->code_file(), symbol_data)); +bool FastSourceLineResolver::ShouldDeleteMemoryBufferAfterLoadModule() { + return false; } void FastSourceLineResolver::Module::LookupAddress(StackFrame *frame) const { diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc index ac0368d5..fcac48ff 100644 --- a/src/processor/minidump_processor_unittest.cc +++ b/src/processor/minidump_processor_unittest.cc @@ -36,6 +36,7 @@ #include #include #include +#include #include "breakpad_googletest_includes.h" #include "google_breakpad/processor/basic_source_line_resolver.h" @@ -118,11 +119,14 @@ class TestSymbolSupplier : public SymbolSupplier { string *symbol_file, char **symbol_data); + virtual void FreeSymbolData(const CodeModule *module); + // When set to true, causes the SymbolSupplier to return INTERRUPT void set_interrupt(bool interrupt) { interrupt_ = interrupt; } private: bool interrupt_; + map memory_buffers_; }; SymbolSupplier::SymbolResult TestSymbolSupplier::GetSymbolFile( @@ -181,13 +185,27 @@ SymbolSupplier::SymbolResult TestSymbolSupplier::GetCStringSymbolData( &symbol_data_string); if (s == FOUND) { unsigned int size = symbol_data_string.size() + 1; - *symbol_data = reinterpret_cast(operator new(size)); + *symbol_data = new char[size]; + if (*symbol_data == NULL) { + BPLOG(ERROR) << "Memory allocation failed for module: " + << module->code_file() << " size: " << size; + return INTERRUPT; + } strcpy(*symbol_data, symbol_data_string.c_str()); + memory_buffers_.insert(make_pair(module->code_file(), *symbol_data)); } return s; } +void TestSymbolSupplier::FreeSymbolData(const CodeModule *module) { + map::iterator it = memory_buffers_.find(module->code_file()); + if (it != memory_buffers_.end()) { + delete [] it->second; + memory_buffers_.erase(it); + } +} + // A mock symbol supplier that always returns NOT_FOUND; one current // use for testing the processor's caching of symbol lookups. class MockSymbolSupplier : public SymbolSupplier { @@ -204,6 +222,7 @@ class MockSymbolSupplier : public SymbolSupplier { const SystemInfo*, string*, char**)); + MOCK_METHOD1(FreeSymbolData, void(const CodeModule*)); }; class MinidumpProcessorTest : public ::testing::Test { diff --git a/src/processor/network_source_line_server_unittest.cc b/src/processor/network_source_line_server_unittest.cc index 50eb8a04..c45f19cc 100644 --- a/src/processor/network_source_line_server_unittest.cc +++ b/src/processor/network_source_line_server_unittest.cc @@ -93,6 +93,7 @@ public: const SystemInfo *system_info, string *symbol_file, char **symbol_data)); + MOCK_METHOD1(FreeSymbolData, void(const CodeModule *module)); }; class MockSourceLineResolver : public SourceLineResolverInterface { @@ -106,6 +107,7 @@ class MockSourceLineResolver : public SourceLineResolverInterface { const string &map_buffer)); MOCK_METHOD2(LoadModuleUsingMemoryBuffer, bool(const CodeModule *module, char *memory_buffer)); + MOCK_METHOD0(ShouldDeleteMemoryBufferAfterLoadModule, bool()); MOCK_METHOD1(UnloadModule, void(const CodeModule *module)); MOCK_METHOD1(HasModule, bool(const CodeModule *module)); MOCK_METHOD1(FillSourceLineInfo, void(StackFrame *frame)); diff --git a/src/processor/simple_symbol_supplier.cc b/src/processor/simple_symbol_supplier.cc index df77d72c..76820e12 100644 --- a/src/processor/simple_symbol_supplier.cc +++ b/src/processor/simple_symbol_supplier.cc @@ -107,13 +107,34 @@ SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetCStringSymbolData( if (s == FOUND) { unsigned int size = symbol_data_string.size() + 1; - *symbol_data = reinterpret_cast(operator new(size)); + *symbol_data = new char[size]; + if (*symbol_data == NULL) { + BPLOG(ERROR) << "Memory allocation for size " << size << " failed"; + return INTERRUPT; + } memcpy(*symbol_data, symbol_data_string.c_str(), size - 1); (*symbol_data)[size - 1] = '\0'; + memory_buffers_.insert(make_pair(module->code_file(), *symbol_data)); } return s; } +void SimpleSymbolSupplier::FreeSymbolData(const CodeModule *module) { + if (!module) { + BPLOG(INFO) << "Cannot free symbol data buffer for NULL module"; + return; + } + + map::iterator it = memory_buffers_.find(module->code_file()); + if (it == memory_buffers_.end()) { + BPLOG(INFO) << "Cannot find symbol data buffer for module " + << module->code_file(); + return; + } + delete [] it->second; + memory_buffers_.erase(it); +} + SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFileAtPathFromRoot( const CodeModule *module, const SystemInfo *system_info, const string &root_path, string *symbol_file) { diff --git a/src/processor/simple_symbol_supplier.h b/src/processor/simple_symbol_supplier.h index 753bd0cd..e1c16195 100644 --- a/src/processor/simple_symbol_supplier.h +++ b/src/processor/simple_symbol_supplier.h @@ -76,6 +76,7 @@ #ifndef PROCESSOR_SIMPLE_SYMBOL_SUPPLIER_H__ #define PROCESSOR_SIMPLE_SYMBOL_SUPPLIER_H__ +#include #include #include @@ -83,6 +84,7 @@ namespace google_breakpad { +using std::map; using std::string; using std::vector; @@ -111,11 +113,16 @@ class SimpleSymbolSupplier : public SymbolSupplier { string *symbol_file, string *symbol_data); + // Allocates data buffer on heap and writes symbol data into buffer. + // Symbol supplier ALWAYS takes ownership of the data buffer. virtual SymbolResult GetCStringSymbolData(const CodeModule *module, const SystemInfo *system_info, string *symbol_file, char **symbol_data); + // Free the data buffer allocated in the above GetCStringSymbolData(); + virtual void FreeSymbolData(const CodeModule *module); + protected: SymbolResult GetSymbolFileAtPathFromRoot(const CodeModule *module, const SystemInfo *system_info, @@ -123,6 +130,7 @@ class SimpleSymbolSupplier : public SymbolSupplier { string *symbol_file); private: + map memory_buffers_; vector paths_; }; diff --git a/src/processor/source_line_resolver_base.cc b/src/processor/source_line_resolver_base.cc index 14f6ec6d..b8362dcf 100644 --- a/src/processor/source_line_resolver_base.cc +++ b/src/processor/source_line_resolver_base.cc @@ -53,6 +53,7 @@ namespace google_breakpad { SourceLineResolverBase::SourceLineResolverBase( ModuleFactory *module_factory) : modules_(new ModuleMap), + memory_buffers_(new MemoryMap), module_factory_(module_factory) { } @@ -65,20 +66,17 @@ SourceLineResolverBase::~SourceLineResolverBase() { } // Delete the map of modules. delete modules_; + + MemoryMap::iterator iter = memory_buffers_->begin(); + for (; iter != memory_buffers_->end(); ++iter) { + delete [] iter->second; + } + // Delete the map of memory buffers. + delete memory_buffers_; + delete module_factory_; - - // Helper method to be specified by subclasses. - ClearLocalMemory(); } -// Helper methods to be specified by subclasses. -void SourceLineResolverBase::StoreDataBeforeLoad(const CodeModule *module, - char *symbol_data) { } -void SourceLineResolverBase::DeleteDataAfterLoad(char *symbol_data) { } -void SourceLineResolverBase::DeleteDataUnload(const CodeModule *module) { } -void SourceLineResolverBase::ClearLocalMemory() { } - - bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data, const string &map_file) { if (symbol_data == NULL) { @@ -100,7 +98,7 @@ bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data, // Allocate memory for file contents, plus a null terminator // since we may use strtok() on the contents. - *symbol_data = reinterpret_cast(operator new(file_size + 1)); + *symbol_data = new char[file_size + 1]; if (*symbol_data == NULL) { BPLOG(ERROR) << "Could not allocate memory for " << map_file; @@ -115,7 +113,7 @@ bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data, int error_code = ErrnoString(&error_string); BPLOG(ERROR) << "Could not open " << map_file << ", error " << error_code << ": " << error_string; - delete (*symbol_data); + delete [] (*symbol_data); *symbol_data = NULL; return false; } @@ -131,7 +129,7 @@ bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data, int error_code = ErrnoString(&error_string); BPLOG(ERROR) << "Could not slurp " << map_file << ", error " << error_code << ": " << error_string; - delete (*symbol_data); + delete [] (*symbol_data); *symbol_data = NULL; return false; } @@ -161,59 +159,80 @@ bool SourceLineResolverBase::LoadModule(const CodeModule *module, BPLOG(INFO) << "Read symbol file " << map_file << " succeeded"; - // Invoke helper method, let the concrete subclass decides its own action. - StoreDataBeforeLoad(module, memory_buffer); + bool load_result = LoadModuleUsingMemoryBuffer(module, memory_buffer); - return LoadModuleUsingMemoryBuffer(module, memory_buffer); + if (load_result && !ShouldDeleteMemoryBufferAfterLoadModule()) { + // memory_buffer has to stay alive as long as the module. + memory_buffers_->insert(make_pair(module->code_file(), memory_buffer)); + } else { + delete [] memory_buffer; + } + + return load_result; } bool SourceLineResolverBase::LoadModuleUsingMapBuffer( const CodeModule *module, const string &map_buffer) { - char *memory_buffer = reinterpret_cast( - operator new(map_buffer.size() + 1)); - if (memory_buffer == NULL) + if (module == NULL) + return false; + + // Make sure we don't already have a module with the given name. + if (modules_->find(module->code_file()) != modules_->end()) { + BPLOG(INFO) << "Symbols for module " << module->code_file() + << " already loaded"; + return false; + } + + char *memory_buffer = new char[map_buffer.size() + 1]; + if (memory_buffer == NULL) { + BPLOG(ERROR) << "Could not allocate memory for " << module->code_file(); + return false; + } + + // Can't use strcpy, as the data may contain '\0's before the end. + memcpy(memory_buffer, map_buffer.c_str(), map_buffer.size()); + memory_buffer[map_buffer.size()] = '\0'; + + bool load_result = LoadModuleUsingMemoryBuffer(module, memory_buffer); + + if (load_result && !ShouldDeleteMemoryBufferAfterLoadModule()) { + // memory_buffer has to stay alive as long as the module. + memory_buffers_->insert(make_pair(module->code_file(), memory_buffer)); + } else { + delete [] memory_buffer; + } + + return load_result; +} + +bool SourceLineResolverBase::LoadModuleUsingMemoryBuffer( + const CodeModule *module, char *memory_buffer) { + if (!module) return false; - // Can't use strcpy, as the data may contain '\0's before the end. - memcpy(memory_buffer, map_buffer.c_str(), map_buffer.size()); - memory_buffer[map_buffer.size()] = '\0'; - - // Invoke helper method, let the concrete subclass decides its own action. - StoreDataBeforeLoad(module, memory_buffer); - - return LoadModuleUsingMemoryBuffer(module, memory_buffer); -} - -bool SourceLineResolverBase::LoadModuleUsingMemoryBuffer( - const CodeModule *module, char *memory_buffer) { - if (!module) { - // Invoke helper method, let the concrete subclass decides its own action. - DeleteDataAfterLoad(memory_buffer); - return false; - } - // Make sure we don't already have a module with the given name. if (modules_->find(module->code_file()) != modules_->end()) { BPLOG(INFO) << "Symbols for module " << module->code_file() << " already loaded"; - DeleteDataAfterLoad(memory_buffer); return false; } BPLOG(INFO) << "Loading symbols for module " << module->code_file() - << " from buffer"; + << " from memory buffer"; Module *basic_module = module_factory_->CreateModule(module->code_file()); // Ownership of memory is NOT transfered to Module::LoadMapFromMemory(). if (!basic_module->LoadMapFromMemory(memory_buffer)) { delete basic_module; - DeleteDataAfterLoad(memory_buffer); return false; } modules_->insert(make_pair(module->code_file(), basic_module)); - DeleteDataAfterLoad(memory_buffer); + return true; +} + +bool SourceLineResolverBase::ShouldDeleteMemoryBufferAfterLoadModule() { return true; } @@ -228,7 +247,16 @@ void SourceLineResolverBase::UnloadModule(const CodeModule *code_module) { modules_->erase(iter); } - DeleteDataUnload(code_module); + if (ShouldDeleteMemoryBufferAfterLoadModule()) { + // No-op. Because we never store any memory buffers. + } else { + // There may be a buffer stored locally, we need to find and delete it. + MemoryMap::iterator iter = memory_buffers_->find(code_module->code_file()); + if (iter != memory_buffers_->end()) { + delete [] iter->second; + memory_buffers_->erase(iter); + } + } } bool SourceLineResolverBase::HasModule(const CodeModule *module) { diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc index 296697cb..58de27dc 100644 --- a/src/processor/stackwalker.cc +++ b/src/processor/stackwalker.cc @@ -116,6 +116,9 @@ bool Stackwalker::Walk(CallStack *stack) { case SymbolSupplier::INTERRUPT: return false; } + // Inform symbol supplier to free the unused data memory buffer. + if (resolver_->ShouldDeleteMemoryBufferAfterLoadModule()) + supplier_->FreeSymbolData(module); } resolver_->FillSourceLineInfo(frame.get()); } diff --git a/src/processor/stackwalker_unittest_utils.h b/src/processor/stackwalker_unittest_utils.h index 2da71bc0..d2e29f72 100644 --- a/src/processor/stackwalker_unittest_utils.h +++ b/src/processor/stackwalker_unittest_utils.h @@ -174,6 +174,7 @@ class MockSymbolSupplier: public google_breakpad::SymbolSupplier { const SystemInfo *system_info, std::string *symbol_file, char **symbol_data)); + MOCK_METHOD1(FreeSymbolData, void(const CodeModule *module)); }; #endif // PROCESSOR_STACKWALKER_UNITTEST_UTILS_H_ diff --git a/src/tools/mac/crash_report/on_demand_symbol_supplier.h b/src/tools/mac/crash_report/on_demand_symbol_supplier.h index 2bad776d..28002c6b 100644 --- a/src/tools/mac/crash_report/on_demand_symbol_supplier.h +++ b/src/tools/mac/crash_report/on_demand_symbol_supplier.h @@ -61,10 +61,16 @@ class OnDemandSymbolSupplier : public SymbolSupplier { const SystemInfo *system_info, string *symbol_file, string *symbol_data); + // Allocates data buffer on heap, and takes the ownership of + // the data buffer. virtual SymbolResult GetCStringSymbolData(const CodeModule *module, const SystemInfo *system_info, string *symbol_file, char **symbol_data); + + // Delete the data buffer allocated for module in GetCStringSymbolData(). + virtual void FreeSymbolData(const CodeModule *module); + protected: // Search directory string search_dir_; @@ -74,6 +80,9 @@ class OnDemandSymbolSupplier : public SymbolSupplier { // and the path to that module's symbol file. map module_file_map_; + // Map of allocated data buffers, keyed by module->code_file(). + map memory_buffers_; + // Return the name for |module| This will be the value used as the key // to the |module_file_map_|. string GetNameForModule(const CodeModule *module); diff --git a/src/tools/mac/crash_report/on_demand_symbol_supplier.mm b/src/tools/mac/crash_report/on_demand_symbol_supplier.mm index e1db9203..f71aebd9 100644 --- a/src/tools/mac/crash_report/on_demand_symbol_supplier.mm +++ b/src/tools/mac/crash_report/on_demand_symbol_supplier.mm @@ -32,6 +32,7 @@ #include #include #include +#include #include "google_breakpad/processor/basic_source_line_resolver.h" #include "google_breakpad/processor/minidump.h" @@ -49,7 +50,7 @@ using google_breakpad::PathnameStripper; using google_breakpad::SymbolSupplier; using google_breakpad::SystemInfo; -OnDemandSymbolSupplier::OnDemandSymbolSupplier(const string &search_dir, +OnDemandSymbolSupplier::OnDemandSymbolSupplier(const string &search_dir, const string &symbol_search_dir) : search_dir_(search_dir) { NSFileManager *mgr = [NSFileManager defaultManager]; @@ -169,10 +170,27 @@ OnDemandSymbolSupplier::GetCStringSymbolData(const CodeModule *module, system_info, symbol_file, &symbol_data_string); - strcpy(*symbol_data, symbol_data_string.c_str()); + if (result == FOUND) { + unsigned int size = symbol_data_string.size() + 1; + *symbol_data = new char[size]; + if (*symbol_data == NULL) { + // Should return INTERRUPT on memory allocation failure. + return INTERRUPT; + } + strcpy(*symbol_data, symbol_data_string.c_str()); + memory_buffers_.insert(make_pair(module->code_file(), *symbol_data); + } return result; } +void OnDemandSymbolSupplier::FreeSymbolData(const CodeModule *module) { + map::iterator it = memory_buffers_.find(module->code_file()); + if (it != memory_buffers_.end()) { + delete [] it->second; + memory_buffers_.erase(it); + } +} + string OnDemandSymbolSupplier::GetLocalModulePath(const CodeModule *module) { NSFileManager *mgr = [NSFileManager defaultManager]; const char *moduleStr = module->code_file().c_str(); @@ -281,9 +299,9 @@ bool OnDemandSymbolSupplier::GenerateSymbolFile(const CodeModule *module, } else { printf("Unable to open %s (%d)\n", name.c_str(), errno); result = false; - } + } } else { - printf("Architecture %s not available for %s\n", + printf("Architecture %s not available for %s\n", system_info->cpu.c_str(), name.c_str()); result = false; }