Fix more memory leaks with proper smart pointer usage

Fix more memory leaks, specifically for Module::Extern and
Module::StackFrameEntry that were outside the Module's AddressRange.

To fix this, and to prevent issues like the one fixed by
79326ebe94
in the future, switched to proper use of std::unique_ptr for Module's
Extern and StackFrameEntry functions. These should enforce ownership
correctly and make the ownership flow much more visible and clear.

Change-Id: I7c943dff3501836a5e303febedc1b312e6f0a1fe
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4129821
Reviewed-by: Joshua Peraza <jperaza@chromium.org>

src/common/dwarf_cfi_to_module.cc

@@ -33,7 +33,9 @@
 // Implementation of google_breakpad::DwarfCFIToModule.
 // See dwarf_cfi_to_module.h for details.
 
+#include <memory>
 #include <sstream>
+#include <utility>
 
 #include "common/dwarf_cfi_to_module.h"
 
@@ -151,7 +153,7 @@ bool DwarfCFIToModule::Entry(size_t offset, uint64_t address, uint64_t length,
   // need to check them here.
 
   // Get ready to collect entries.
-  entry_ = new Module::StackFrameEntry;
+  entry_ = std::make_unique<Module::StackFrameEntry>();
   entry_->address = address;
   entry_->size = length;
   entry_offset_ = offset;
@@ -258,8 +260,7 @@ bool DwarfCFIToModule::ValExpressionRule(uint64_t address, int reg,
 }
 
 bool DwarfCFIToModule::End() {
-  module_->AddStackFrameEntry(entry_);
+  module_->AddStackFrameEntry(std::move(entry_));
-  entry_ = NULL;
   return true;
 }
 

src/common/dwarf_cfi_to_module.h

@@ -43,6 +43,7 @@
 
 #include <set>
 #include <string>
+#include <memory>
 #include <vector>
 
 #include "common/module.h"
@@ -131,9 +132,9 @@ class DwarfCFIToModule: public CallFrameInfo::Handler {
   DwarfCFIToModule(Module* module, const vector<string>& register_names,
                    Reporter* reporter)
       : module_(module), register_names_(register_names), reporter_(reporter),
-        entry_(NULL), return_address_(-1), cfa_name_(".cfa"), ra_name_(".ra") {
+        return_address_(-1), cfa_name_(".cfa"), ra_name_(".ra") {
   }
-  virtual ~DwarfCFIToModule() { delete entry_; }
+  virtual ~DwarfCFIToModule() = default;
 
   virtual bool Entry(size_t offset, uint64_t address, uint64_t length,
                      uint8_t version, const string& augmentation,
@@ -170,7 +171,7 @@ class DwarfCFIToModule: public CallFrameInfo::Handler {
   Reporter* reporter_;
 
   // The current entry we're constructing.
-  Module::StackFrameEntry* entry_;
+  std::unique_ptr<Module::StackFrameEntry> entry_;
 
   // The section offset of the current frame description entry, for
   // use in error messages.

src/common/linux/elf_symbols_to_module.cc

@@ -36,6 +36,9 @@
 #include <elf.h>
 #include <string.h>
 
+#include <memory>
+#include <utility>
+
 #include "common/byte_cursor.h"
 #include "common/module.h"
 
@@ -156,7 +159,7 @@ 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(iterator->value);
+      auto ext = std::make_unique<Module::Extern>(iterator->value);
       ext->name = SymbolString(iterator->name_offset, strings);
 #if !defined(__ANDROID__)  // Android NDK doesn't provide abi::__cxa_demangle.
       int status = 0;
@@ -168,7 +171,7 @@ bool ELFSymbolsToModule(const uint8_t* symtab_section,
         free(demangled);
       }
 #endif
-      module->AddExtern(ext);
+      module->AddExtern(std::move(ext));
     }
     ++iterator;
   }

src/common/module.cc

@@ -117,12 +117,6 @@ Module::~Module() {
        it != functions_.end(); ++it) {
     delete *it;
   }
-  for (vector<StackFrameEntry*>::iterator it = stack_frame_entries_.begin();
-       it != stack_frame_entries_.end(); ++it) {
-    delete *it;
-  }
-  for (ExternSet::iterator it = externs_.begin(); it != externs_.end(); ++it)
-    delete *it;
 }
 
 void Module::SetLoadAddress(Address address) {
@@ -155,12 +149,11 @@ bool Module::AddFunction(Function* function) {
   }
   if (it_ext != externs_.end()) {
     if (enable_multiple_field_) {
-      Extern* found_ext = *it_ext;
+      Extern* found_ext = it_ext->get();
       // If the PUBLIC is for the same symbol as the FUNC, don't mark multiple.
       function->is_multiple |=
           found_ext->name != function->name || found_ext->is_multiple;
     }
-    delete *it_ext;
     externs_.erase(it_ext);
   }
 #if _DEBUG
@@ -194,27 +187,22 @@ bool Module::AddFunction(Function* function) {
   return true;
 }
 
-void Module::AddStackFrameEntry(StackFrameEntry* stack_frame_entry) {
+void Module::AddStackFrameEntry(std::unique_ptr<StackFrameEntry> stack_frame_entry) {
   if (!AddressIsInModule(stack_frame_entry->address)) {
     return;
   }
 
-  stack_frame_entries_.push_back(stack_frame_entry);
+  stack_frame_entries_.push_back(std::move(stack_frame_entry));
 }
 
-void Module::AddExtern(Extern* ext) {
+void Module::AddExtern(std::unique_ptr<Extern> ext) {
   if (!AddressIsInModule(ext->address)) {
     return;
   }
 
-  std::pair<ExternSet::iterator,bool> ret = externs_.insert(ext);
+  std::pair<ExternSet::iterator,bool> ret = externs_.emplace(std::move(ext));
-  if (!ret.second) {
+  if (!ret.second && enable_multiple_field_) {
-    if (enable_multiple_field_) {
+    (*ret.first)->is_multiple = true;
-      (*ret.first)->is_multiple = true;
-    }
-    // Free the duplicate that was not inserted because this Module
-    // now owns it.
-    delete ext;
   }
 }
 
@@ -225,7 +213,11 @@ void Module::GetFunctions(vector<Function*>* vec,
 
 void Module::GetExterns(vector<Extern*>* vec,
                         vector<Extern*>::iterator i) {
-  vec->insert(i, externs_.begin(), externs_.end());
+  auto pos = vec->insert(i, externs_.size(), nullptr);
+  for (const std::unique_ptr<Extern>& ext : externs_) {
+    *pos = ext.get();
+    ++pos;
+  }
 }
 
 Module::File* Module::FindFile(const string& name) {
@@ -267,7 +259,11 @@ void Module::GetFiles(vector<File*>* vec) {
 }
 
 void Module::GetStackFrameEntries(vector<StackFrameEntry*>* vec) const {
-  *vec = stack_frame_entries_;
+  vec->clear();
+  vec->reserve(stack_frame_entries_.size());
+  for (const auto& ent : stack_frame_entries_) {
+    vec->push_back(ent.get());
+  }
 }
 
 void Module::AssignSourceIds(
@@ -443,7 +439,7 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) {
     // Write out 'PUBLIC' records.
     for (ExternSet::const_iterator extern_it = externs_.begin();
          extern_it != externs_.end(); ++extern_it) {
-      Extern* ext = *extern_it;
+      Extern* ext = extern_it->get();
       stream << "PUBLIC " << (ext->is_multiple ? "m " : "") << hex
              << (ext->address - load_address_) << " 0 " << ext->name << dec
              << "\n";
@@ -452,10 +448,9 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) {
 
   if (symbol_data & CFI) {
     // Write out 'STACK CFI INIT' and 'STACK CFI' records.
-    vector<StackFrameEntry*>::const_iterator frame_it;
+    for (auto frame_it = stack_frame_entries_.begin();
-    for (frame_it = stack_frame_entries_.begin();
          frame_it != stack_frame_entries_.end(); ++frame_it) {
-      StackFrameEntry* entry = *frame_it;
+      StackFrameEntry* entry = frame_it->get();
       stream << "STACK CFI INIT " << hex
              << (entry->address - load_address_) << " "
              << entry->size << " " << dec;

src/common/module.h

@@ -292,7 +292,18 @@ class Module {
   };
 
   struct ExternCompare {
-    bool operator() (const Extern* lhs, const Extern* rhs) const {
+    // Defining is_transparent allows
+    // std::set<std::unique_ptr<Extern>, ExternCompare>::find() to be called
+    // with an Extern* and have set use the overloads below.
+    using is_transparent = void;
+    bool operator() (const std::unique_ptr<Extern>& lhs,
+                     const std::unique_ptr<Extern>& rhs) const {
+      return lhs->address < rhs->address;
+    }
+    bool operator() (const Extern* lhs, const std::unique_ptr<Extern>& rhs) const {
+      return lhs->address < rhs->address;
+    }
+    bool operator() (const std::unique_ptr<Extern>& lhs, const Extern* rhs) const {
       return lhs->address < rhs->address;
     }
   };
@@ -340,12 +351,12 @@ class Module {
   // Add STACK_FRAME_ENTRY to the module.
   // This module owns all StackFrameEntry objects added with this
   // function: destroying the module destroys them as well.
-  void AddStackFrameEntry(StackFrameEntry* stack_frame_entry);
+  void AddStackFrameEntry(std::unique_ptr<StackFrameEntry> stack_frame_entry);
 
   // Add PUBLIC to the module.
   // This module owns all Extern objects added with this function:
   // destroying the module destroys them as well.
-  void AddExtern(Extern* ext);
+  void AddExtern(std::unique_ptr<Extern> ext);
 
   // If this module has a file named NAME, return a pointer to it. If
   // it has none, then create one and return a pointer to the new
@@ -465,7 +476,7 @@ class Module {
   typedef set<Function*, FunctionCompare> FunctionSet;
 
   // A set containing Extern structures, sorted by address.
-  typedef set<Extern*, ExternCompare> ExternSet;
+  typedef set<std::unique_ptr<Extern>, ExternCompare> ExternSet;
 
   // The module owns all the files and functions that have been added
   // to it; destroying the module frees the Files and Functions these
@@ -477,7 +488,7 @@ class Module {
 
   // The module owns all the call frame info entries that have been
   // added to it.
-  vector<StackFrameEntry*> stack_frame_entries_;
+  vector<std::unique_ptr<StackFrameEntry>> stack_frame_entries_;
 
   // The module owns all the externs that have been added to it;
   // destroying the module frees the Externs these point to.

src/common/module_unittest.cc

@@ -36,8 +36,10 @@
 #include <string.h>
 
 #include <algorithm>
+#include <memory>
 #include <sstream>
 #include <string>
+#include <utility>
 
 #include "breakpad_googletest_includes.h"
 #include "common/module.h"
@@ -137,7 +139,7 @@ TEST(Module, WriteRelativeLoadAddress) {
   m.AddFunction(function);
 
   // Some stack information.
-  Module::StackFrameEntry* entry = new Module::StackFrameEntry();
+  auto entry = std::make_unique<Module::StackFrameEntry>();
   entry->address = 0x30f9e5c83323973dULL;
   entry->size = 0x49fc9ca7c7c13dc2ULL;
   entry->initial_rules[".cfa"] = "he was a handsome man";
@@ -145,7 +147,7 @@ TEST(Module, WriteRelativeLoadAddress) {
   entry->rule_changes[0x30f9e5c83323973eULL]["how"] =
     "do you like your blueeyed boy";
   entry->rule_changes[0x30f9e5c83323973eULL]["Mister"] = "Death";
-  m.AddStackFrameEntry(entry);
+  m.AddStackFrameEntry(std::move(entry));
 
   // Set the load address.  Doing this after adding all the data to
   // the module must work fine.
@@ -242,7 +244,7 @@ TEST(Module, WriteNoCFI) {
   m.AddFunction(function);
 
   // Some stack information.
-  Module::StackFrameEntry* entry = new Module::StackFrameEntry();
+  auto entry = std::make_unique<Module::StackFrameEntry>();
   entry->address = 0x30f9e5c83323973dULL;
   entry->size = 0x49fc9ca7c7c13dc2ULL;
   entry->initial_rules[".cfa"] = "he was a handsome man";
@@ -250,7 +252,7 @@ TEST(Module, WriteNoCFI) {
   entry->rule_changes[0x30f9e5c83323973eULL]["how"] =
     "do you like your blueeyed boy";
   entry->rule_changes[0x30f9e5c83323973eULL]["Mister"] = "Death";
-  m.AddStackFrameEntry(entry);
+  m.AddStackFrameEntry(std::move(entry));
 
   // Set the load address.  Doing this after adding all the data to
   // the module must work fine.
@@ -321,18 +323,18 @@ TEST(Module, WriteOutOfRangeAddresses) {
 
   // Add three stack frames (one lower, one in, and one higher than the allowed
   // address range).  Only the middle frame should be captured.
-  Module::StackFrameEntry* entry1 = new Module::StackFrameEntry();
+  auto entry1 = std::make_unique<Module::StackFrameEntry>();
   entry1->address = 0x1000ULL;
   entry1->size = 0x100ULL;
-  m.AddStackFrameEntry(entry1);
+  m.AddStackFrameEntry(std::move(entry1));
-  Module::StackFrameEntry* entry2 = new Module::StackFrameEntry();
+  auto entry2 = std::make_unique<Module::StackFrameEntry>();
   entry2->address = 0x2000ULL;
   entry2->size = 0x100ULL;
-  m.AddStackFrameEntry(entry2);
+  m.AddStackFrameEntry(std::move(entry2));
-  Module::StackFrameEntry* entry3 = new Module::StackFrameEntry();
+  auto entry3 = std::make_unique<Module::StackFrameEntry>();
   entry3->address = 0x3000ULL;
   entry3->size = 0x100ULL;
-  m.AddStackFrameEntry(entry3);
+  m.AddStackFrameEntry(std::move(entry3));
 
   // Add a function outside the allowed range.
   Module::File* file = m.FindFile("file_name.cc");
@@ -346,9 +348,9 @@ TEST(Module, WriteOutOfRangeAddresses) {
   m.AddFunction(function);
 
   // Add an extern outside the allowed range.
-  Module::Extern* extern1 = new Module::Extern(0x5000ULL);
+  auto extern1 = std::make_unique<Module::Extern>(0x5000ULL);
   extern1->name = "_xyz";
-  m.AddExtern(extern1);
+  m.AddExtern(std::move(extern1));
 
   m.Write(s, ALL_SYMBOL_DATA);
 
@@ -357,10 +359,7 @@ TEST(Module, WriteOutOfRangeAddresses) {
                s.str().c_str());
 
   // Cleanup - Prevent Memory Leak errors.
-  delete (extern1);
   delete (function);
-  delete (entry3);
-  delete (entry1);
 }
 
 TEST(Module, ConstructAddFrames) {
@@ -368,22 +367,22 @@ TEST(Module, ConstructAddFrames) {
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
 
   // First STACK CFI entry, with no initial rules or deltas.
-  Module::StackFrameEntry* entry1 = new Module::StackFrameEntry();
+  auto entry1 = std::make_unique<Module::StackFrameEntry>();
   entry1->address = 0xddb5f41285aa7757ULL;
   entry1->size = 0x1486493370dc5073ULL;
-  m.AddStackFrameEntry(entry1);
+  m.AddStackFrameEntry(std::move(entry1));
 
   // Second STACK CFI entry, with initial rules but no deltas.
-  Module::StackFrameEntry* entry2 = new Module::StackFrameEntry();
+  auto entry2 = std::make_unique<Module::StackFrameEntry>();
   entry2->address = 0x8064f3af5e067e38ULL;
   entry2->size = 0x0de2a5ee55509407ULL;
   entry2->initial_rules[".cfa"] = "I think that I shall never see";
   entry2->initial_rules["stromboli"] = "a poem lovely as a tree";
   entry2->initial_rules["cannoli"] = "a tree whose hungry mouth is prest";
-  m.AddStackFrameEntry(entry2);
+  m.AddStackFrameEntry(std::move(entry2));
 
   // Third STACK CFI entry, with initial rules and deltas.
-  Module::StackFrameEntry* entry3 = new Module::StackFrameEntry();
+  auto entry3 = std::make_unique<Module::StackFrameEntry>();
   entry3->address = 0x5e8d0db0a7075c6cULL;
   entry3->size = 0x1c7edb12a7aea229ULL;
   entry3->initial_rules[".cfa"] = "Whose woods are these";
@@ -395,7 +394,7 @@ TEST(Module, ConstructAddFrames) {
     "his house is in";
   entry3->rule_changes[0x36682fad3763ffffULL][".cfa"] =
     "I think I know";
-  m.AddStackFrameEntry(entry3);
+  m.AddStackFrameEntry(std::move(entry3));
 
   // Check that Write writes STACK CFI records properly.
   m.Write(s, ALL_SYMBOL_DATA);
@@ -541,13 +540,13 @@ TEST(Module, ConstructExterns) {
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
 
   // Two externs.
-  Module::Extern* extern1 = new Module::Extern(0xffff);
+  auto extern1 = std::make_unique<Module::Extern>(0xffff);
   extern1->name = "_abc";
-  Module::Extern* extern2 = new Module::Extern(0xaaaa);
+  auto extern2 = std::make_unique<Module::Extern>(0xaaaa);
   extern2->name = "_xyz";
 
-  m.AddExtern(extern1);
+  m.AddExtern(std::move(extern1));
-  m.AddExtern(extern2);
+  m.AddExtern(std::move(extern2));
 
   m.Write(s, ALL_SYMBOL_DATA);
   string contents = s.str();
@@ -566,13 +565,13 @@ TEST(Module, ConstructDuplicateExterns) {
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
 
   // Two externs.
-  Module::Extern* extern1 = new Module::Extern(0xffff);
+  auto extern1 = std::make_unique<Module::Extern>(0xffff);
   extern1->name = "_xyz";
-  Module::Extern* extern2 = new Module::Extern(0xffff);
+  auto extern2 = std::make_unique<Module::Extern>(0xffff);
   extern2->name = "_abc";
 
-  m.AddExtern(extern1);
+  m.AddExtern(std::move(extern1));
-  m.AddExtern(extern2);
+  m.AddExtern(std::move(extern2));
 
   m.Write(s, ALL_SYMBOL_DATA);
   string contents = s.str();
@@ -589,13 +588,13 @@ TEST(Module, ConstructDuplicateExternsMultiple) {
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID, "", true);
 
   // Two externs.
-  Module::Extern* extern1 = new Module::Extern(0xffff);
+  auto extern1 = std::make_unique<Module::Extern>(0xffff);
   extern1->name = "_xyz";
-  Module::Extern* extern2 = new Module::Extern(0xffff);
+  auto extern2 = std::make_unique<Module::Extern>(0xffff);
   extern2->name = "_abc";
 
-  m.AddExtern(extern1);
+  m.AddExtern(std::move(extern1));
-  m.AddExtern(extern2);
+  m.AddExtern(std::move(extern2));
 
   m.Write(s, ALL_SYMBOL_DATA);
   string contents = s.str();
@@ -613,13 +612,13 @@ TEST(Module, ConstructFunctionsAndExternsWithSameAddress) {
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
 
   // Two externs.
-  Module::Extern* extern1 = new Module::Extern(0xabc0);
+  auto extern1 = std::make_unique<Module::Extern>(0xabc0);
   extern1->name = "abc";
-  Module::Extern* extern2 = new Module::Extern(0xfff0);
+  auto extern2 = std::make_unique<Module::Extern>(0xfff0);
   extern2->name = "xyz";
 
-  m.AddExtern(extern1);
+  m.AddExtern(std::move(extern1));
-  m.AddExtern(extern2);
+  m.AddExtern(std::move(extern2));
 
   Module::Function* function = new Module::Function("_xyz", 0xfff0);
   Module::Range range(0xfff0, 0x10);
@@ -644,13 +643,13 @@ TEST(Module, ConstructFunctionsAndExternsWithSameAddressMultiple) {
   Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID, "", true);
 
   // Two externs.
-  Module::Extern* extern1 = new Module::Extern(0xabc0);
+  auto extern1 = std::make_unique<Module::Extern>(0xabc0);
   extern1->name = "abc";
-  Module::Extern* extern2 = new Module::Extern(0xfff0);
+  auto extern2 = std::make_unique<Module::Extern>(0xfff0);
   extern2->name = "xyz";
 
-  m.AddExtern(extern1);
+  m.AddExtern(std::move(extern1));
-  m.AddExtern(extern2);
+  m.AddExtern(std::move(extern2));
 
   Module::Function* function = new Module::Function("_xyz", 0xfff0);
   Module::Range range(0xfff0, 0x10);
@@ -676,17 +675,17 @@ TEST(Module, ConstructFunctionsAndThumbExternsWithSameAddress) {
   Module m(MODULE_NAME, MODULE_OS, "arm", MODULE_ID);
 
   // Two THUMB externs.
-  Module::Extern* thumb_extern1 = new Module::Extern(0xabc1);
+  auto thumb_extern1 = std::make_unique<Module::Extern>(0xabc1);
   thumb_extern1->name = "thumb_abc";
-  Module::Extern* thumb_extern2 = new Module::Extern(0xfff1);
+  auto thumb_extern2 = std::make_unique<Module::Extern>(0xfff1);
   thumb_extern2->name = "thumb_xyz";
 
-  Module::Extern* arm_extern1 = new Module::Extern(0xcc00);
+  auto arm_extern1 = std::make_unique<Module::Extern>(0xcc00);
   arm_extern1->name = "arm_func";
 
-  m.AddExtern(thumb_extern1);
+  m.AddExtern(std::move(thumb_extern1));
-  m.AddExtern(thumb_extern2);
+  m.AddExtern(std::move(thumb_extern2));
-  m.AddExtern(arm_extern1);
+  m.AddExtern(std::move(arm_extern1));
 
   // The corresponding function from the DWARF debug data have the actual
   // address.

src/common/stabs_to_module.cc

@@ -36,6 +36,8 @@
 #include <stdio.h>
 
 #include <algorithm>
+#include <memory>
+#include <utility>
 
 #include "common/stabs_to_module.h"
 #include "common/using_std_string.h"
@@ -132,7 +134,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(address);
+  auto ext = std::make_unique<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) {
@@ -142,7 +144,7 @@ bool StabsToModule::Extern(const string& name, uint64_t address) {
   } else {
     ext->name = name;
   }
-  module_->AddExtern(ext);
+  module_->AddExtern(std::move(ext));
   return true;
 }
 

src/tools/mac/dump_syms/dump_syms_tool.cc

@@ -36,6 +36,8 @@
 
 #include <algorithm>
 #include <iostream>
+#include <memory>
+#include <utility>
 #include <vector>
 
 #include "common/mac/dump_syms.h"
@@ -108,7 +110,8 @@ static void CopyCFIDataBetweenModules(Module* to_module,
     // If the entry does not overlap, then it is safe to copy to |to_module|.
     if (to_it == to_data.end() || (from_entry->address < (*to_it)->address &&
             from_entry_end < (*to_it)->address)) {
-      to_module->AddStackFrameEntry(new Module::StackFrameEntry(*from_entry));
+      to_module->AddStackFrameEntry(
+          std::make_unique<Module::StackFrameEntry>(*from_entry));
     }
   }
 }