Restrict ownership of symbol data buffers to symbol supplier.

git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@721 4c0a9323-5329-0410-9bdc-e9ce6186880e

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<Function> cur_func;
   int line_number = 0;

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; }
 

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 {

src/processor/minidump_processor_unittest.cc

@@ -36,6 +36,7 @@
 #include <iostream>
 #include <fstream>
 #include <map>
+#include <utility>
 
 #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<string, char *> 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<char*>(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<string, char *>::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 {

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));

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<char*>(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<string, char *>::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) {

src/processor/simple_symbol_supplier.h

@@ -76,6 +76,7 @@
 #ifndef PROCESSOR_SIMPLE_SYMBOL_SUPPLIER_H__
 #define PROCESSOR_SIMPLE_SYMBOL_SUPPLIER_H__
 
+#include <map>
 #include <string>
 #include <vector>
 
@@ -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<string, char *> memory_buffers_;
   vector<string> paths_;
 };
 

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<char*>(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<char*>(
-      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) {

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());
       }

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_