Clean up memoryTracker usage in binaryEdit (#877)

* Store memoryTracker_ as a value instead of pointer This provides cleaner semantics for its usage and ensures that the it's default-constructed correctly. * Code cleanup * Give codeRange a virtual destructor This will ensure that memoryTracker objects removed from a codeTree will correctly get destroyed. * Simplify inferiorFree * Free any remaining memoryTrackers on destruction
dyninst · Nov 28, 2021 · 94252b8 · 94252b8
1 parent 91ede1f
commit 94252b8
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 37 deletions.
diff --git a/dyninstAPI/src/binaryEdit.C b/dyninstAPI/src/binaryEdit.C
@@ -60,7 +60,7 @@ bool BinaryEdit::readTextSpace(const void *inOther,
 
     // Look up this address in the code range tree of memory
     codeRange *range = NULL;
-    if (!memoryTracker_->find(addr, range))
+    if (!memoryTracker_.find(addr, range))
         return false;
     assert(addr >= range->get_address());
 
@@ -86,7 +86,7 @@ bool BinaryEdit::writeTextSpace(void *inOther,
     while (to_do) {
        // Look up this address in the code range tree of memory
        codeRange *range = NULL;
-       if (!memoryTracker_->find(addr, range)) {
+       if (!memoryTracker_.find(addr, range)) {
           return false;
        }
 
@@ -197,10 +197,7 @@ Address BinaryEdit::inferiorMalloc(unsigned size,
         if (ret) {
 	  memoryTracker *newTracker = new memoryTracker(ret, size);
 	  newTracker->alloced = true;
-	  if (!memoryTracker_)
-	    memoryTracker_ = new codeRangeTree();
-	  memoryTracker_->insert(newTracker);
-
+	  memoryTracker_.insert(newTracker);
 	  break;
 	}
     }
@@ -212,17 +209,11 @@ void BinaryEdit::inferiorFree(Address item)
 {
   inferiorFreeInternal(item);
 
-  codeRange *obj;
-  if(!memoryTracker_->find(item, obj))
-  {
-    // Warn the user?
-    return;
-  }
-
-
-  delete obj;
-
-  memoryTracker_->remove(item);
+  codeRange *obj{};
+  if(memoryTracker_.find(item, obj)) delete obj;
+
+  // Remove it from the tree
+  memoryTracker_.remove(item);
 }
 
 bool BinaryEdit::inferiorRealloc(Address item, unsigned newsize)
@@ -234,17 +225,17 @@ bool BinaryEdit::inferiorRealloc(Address item, unsigned newsize)
   maxAllocedAddr();
 
   codeRange *obj;
-  result = memoryTracker_->find(item, obj);
+  result = memoryTracker_.find(item, obj);
   assert(result);
 
-  memoryTracker_->remove(item);
+  memoryTracker_.remove(item);
 
   memoryTracker *mem_track = dynamic_cast<memoryTracker *>(obj);
   assert(mem_track);
 
   mem_track->realloc(newsize);
 
-  memoryTracker_->insert(obj);
+  memoryTracker_.insert(obj);
   return true;
 }
 
@@ -276,7 +267,7 @@ BinaryEdit::BinaryEdit() :
    highWaterMark_(0),
    lowWaterMark_(0),
    isDirty_(false),
-   memoryTracker_(NULL),
+   memoryTracker_{},
    mobj(NULL),
    multithread_capable_(false),
    writing_(false)
@@ -294,7 +285,12 @@ BinaryEdit::~BinaryEdit()
     for(auto *rel : dependentRelocations) {
         delete rel;
     }
-    delete memoryTracker_;
+
+    std::vector<codeRange*> x;
+    memoryTracker_.elements(x);
+    for(auto const *c : x) {
+    	delete c;
+    }
 }
 
 BinaryEdit *BinaryEdit::openFile(const std::string &file, 
@@ -528,7 +524,7 @@ bool BinaryEdit::writeFile(const std::string &newFileName)
       // Now, we need to copy in the memory of the new segments
       for (unsigned i = 0; i < oldSegs.size(); i++) {
          codeRange *segRange = NULL;
-         if (!memoryTracker_->find(oldSegs[i]->getMemOffset(), segRange)) {
+         if (!memoryTracker_.find(oldSegs[i]->getMemOffset(), segRange)) {
 #if 0
             // Looks like BSS
             if (newSegs[i].name == ".bss")
@@ -556,7 +552,7 @@ bool BinaryEdit::writeFile(const std::string &newFileName)
       void *newSectionPtr = calloc(highWaterMark_ - lowWaterMark_, 1);
 
       std::vector<codeRange *> writes;
-      memoryTracker_->elements(writes);
+      memoryTracker_.elements(writes);
 
       for (unsigned i = 0; i < writes.size(); i++) {
          assert(newSectionPtr);
@@ -747,24 +743,16 @@ bool BinaryEdit::createMemoryBackingStore(mapped_object *obj) {
     symObj->getAllRegions(regs);
 
    for (unsigned i = 0; i < regs.size(); i++) {
-      memoryTracker *newTracker = NULL;
       if (regs[i]->getRegionType() == Region::RT_BSS || (regs[i]->getMemSize() == 0))
       {
          continue;
       }
-      else {
-         newTracker = new memoryTracker(regs[i]->getMemOffset(),
-                                        regs[i]->getMemSize(),
-                                        regs[i]->getPtrToRawData());
-
-      }
+      auto *newTracker = new memoryTracker(regs[i]->getMemOffset(),
+                                           regs[i]->getMemSize(),
+                                           regs[i]->getPtrToRawData());
       newTracker->alloced = false;
-      if (!memoryTracker_)
-         memoryTracker_ = new codeRangeTree();
-      memoryTracker_->insert(newTracker);
+      memoryTracker_.insert(newTracker);
    }
-
-
    return true;
 }
 

diff --git a/dyninstAPI/src/binaryEdit.h b/dyninstAPI/src/binaryEdit.h
@@ -211,7 +211,7 @@ class BinaryEdit : public AddressSpace {
    /* Function specific to rewritting static binaries */
    bool doStaticBinarySpecialCases();
 
-    codeRangeTree* memoryTracker_;
+    codeRangeTree memoryTracker_;
 
     mapped_object * addSharedObject(const std::string *fullPath);
 

diff --git a/dyninstAPI/src/codeRange.h b/dyninstAPI/src/codeRange.h
@@ -102,6 +102,8 @@ class codeRange : public patchTarget {
     void print_range(Address addr = 0);
 
     friend ostream &operator<<(ostream &s, const codeRange &c);
+
+    virtual ~codeRange() = default;
 };
 
 class codeRangeTree {