Fix dangling pointer in forward_ref_die_to_func

Bug: google-breakpad:843
Change-Id: I14358b239604e1faeb5a8c4c4734102571dbed09
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/2951787
Reviewed-by: Mike Frysinger <vapier@chromium.org>

src/common/dwarf_cu_to_module.cc

@@ -269,6 +269,9 @@ struct DwarfCUToModule::CUContext {
   //
   // Destroying this destroys all the functions this vector points to.
   vector<Module::Function*> functions;
+
+  // A map of function pointers to the its forward specification DIE's offset.
+  map<Module::Function*, uint64_t> spec_function_offsets;
 };
 
 // Information about the context of a particular DIE. This is for
@@ -714,6 +717,9 @@ void DwarfCUToModule::FuncHandler::Finish() {
         cu_context_->file_context->file_private_
             ->forward_ref_die_to_func[forward_ref_die_offset_] =
             cu_context_->functions.back();
+
+        cu_context_->spec_function_offsets[cu_context_->functions.back()] =
+            forward_ref_die_offset_;
       }
     }
   } else if (inline_) {
@@ -1313,9 +1319,15 @@ void DwarfCUToModule::Finish() {
   AssignLinesToFunctions();
 
   // Add our functions, which now have source lines assigned to them,
-  // to module_.
+  // to module_, and remove duplicate functions.
-  cu_context_->file_context->module_->AddFunctions(functions->begin(),
+  for (Module::Function* func : *functions)
-                                                   functions->end());
+    if (!cu_context_->file_context->module_->AddFunction(func)) {
+      auto iter = cu_context_->spec_function_offsets.find(func);
+      if (iter != cu_context_->spec_function_offsets.end())
+        cu_context_->file_context->file_private_->forward_ref_die_to_func.erase(
+            iter->second);
+      delete func;
+    }
 
   // Ownership of the function objects has shifted from cu_context to
   // the Module.

src/common/module.cc

@@ -80,13 +80,13 @@ void Module::SetAddressRanges(const vector<Range>& ranges) {
   address_ranges_ = ranges;
 }
 
-void Module::AddFunction(Function* function) {
+bool Module::AddFunction(Function* function) {
   // FUNC lines must not hold an empty name, so catch the problem early if
   // callers try to add one.
   assert(!function->name.empty());
 
   if (!AddressIsInModule(function->address)) {
-    return;
+    return false;
   }
 
   // FUNCs are better than PUBLICs as they come with sizes, so remove an extern
@@ -120,14 +120,9 @@ void Module::AddFunction(Function* function) {
   if (!ret.second && (*ret.first != function)) {
     // Free the duplicate that was not inserted because this Module
     // now owns it.
-    delete function;
+    return false;
   }
-}
+  return true;
-
-void Module::AddFunctions(vector<Function*>::iterator begin,
-                          vector<Function*>::iterator end) {
-  for (vector<Function*>::iterator it = begin; it != end; ++it)
-    AddFunction(*it);
 }
 
 void Module::AddStackFrameEntry(StackFrameEntry* stack_frame_entry) {

src/common/module.h

@@ -216,13 +216,8 @@ class Module {
   // Add FUNCTION to the module. FUNCTION's name must not be empty.
   // This module owns all Function objects added with this function:
   // destroying the module destroys them as well.
-  void AddFunction(Function* function);
+  // Return false if the function is duplicate and needs to be freed.
-
+  bool AddFunction(Function* function);
-  // Add all the functions in [BEGIN,END) to the module.
-  // This module owns all Function objects added with this function:
-  // destroying the module destroys them as well.
-  void AddFunctions(vector<Function*>::iterator begin,
-                    vector<Function*>::iterator end);
 
   // Add STACK_FRAME_ENTRY to the module.
   // This module owns all StackFrameEntry objects added with this

src/common/module_unittest.cc

@@ -265,7 +265,7 @@ TEST(Write, NoCFI) {
                contents.c_str());
 }
 
-TEST(Construct, AddFunctions) {
+TEST(Construct, AddFunction) {
   stringstream s;
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
 
@@ -287,7 +287,8 @@ TEST(Construct, AddFunctions) {
   vec.push_back(function1);
   vec.push_back(function2);
 
-  m.AddFunctions(vec.begin(), vec.end());
+  for (Module::Function* func: vec)
+    m.AddFunction(func);
 
   m.Write(s, ALL_SYMBOL_DATA);
   string contents = s.str();

src/common/stabs_to_module.cc

@@ -192,7 +192,8 @@ void StabsToModule::Finalize() {
   }
   // Now that everything has a size, add our functions to the module, and
   // dispose of our private list.
-  module_->AddFunctions(functions_.begin(), functions_.end());
+  for (Module::Function* func: functions_)
+    module_->AddFunction(func);
   functions_.clear();
 }