Fix memory leaks and ambiguous ownership. This introduces many shared…

… pointers to ParseAPI interfaces; I have tried to allow generic options wherever possible as long as they're convertible to shared_ptr so that existing client code continues to work. It is now actually safe and mostly non-leaky to delete BPatch_addressSpaces, images, etc.
dyninst · Jan 27, 2017 · cd1cfb3 · cd1cfb3
1 parent 919f5ac
commit cd1cfb3
Show file tree

Hide file tree

Showing 59 changed files with 419 additions and 501 deletions.
diff --git a/dyninstAPI/CMakeLists.txt b/dyninstAPI/CMakeLists.txt
@@ -192,7 +192,7 @@ set (SRC_LIST ${SRC_LIST} src/cpuid-x86.S)
 set_source_files_properties(src/cpuid-x86.S PROPERTIES LANGUAGE C)
 endif ()
 
-dyninst_library(dyninstAPI common instructionAPI stackwalk pcontrol patchAPI parseAPI symtabAPI)
+dyninst_library(dyninstAPI common instructionAPI stackwalk pcontrol patchAPI parseAPI symtabAPI symLite)
 
 target_link_private_libraries(dyninstAPI ${Boost_LIBRARIES})
 if (UNIX)

diff --git a/dyninstAPI/src/BPatch.C b/dyninstAPI/src/BPatch.C
@@ -51,6 +51,7 @@
 #include "instPoint.h"
 #include "hybridAnalysis.h"
 #include "BPatch_object.h"
+#include "os.h"
 
 // ProcControlAPI interface
 #include "dynProcess.h"

diff --git a/dyninstAPI/src/BPatch_binaryEdit.C b/dyninstAPI/src/BPatch_binaryEdit.C
@@ -185,7 +185,7 @@ BPatch_binaryEdit::~BPatch_binaryEdit()
      delete (*i).second;
   }
   llBinEdits.clear();
-  origBinEdit = NULL;
+
 
   assert(BPatch::bpatch != NULL);
 }

diff --git a/dyninstAPI/src/BPatch_object.C b/dyninstAPI/src/BPatch_object.C
@@ -183,7 +183,7 @@ std::string BPatch_object::Region::format() {
 
 Dyninst::ParseAPI::CodeObject *Dyninst::ParseAPI::convert(const BPatch_object *o) {
    if (!o->obj) return NULL;
-   return o->obj->parse_img()->codeObject();
+   return o->obj->parse_img()->codeObject().get();
 }
 
 Dyninst::PatchAPI::PatchObject *Dyninst::PatchAPI::convert(const BPatch_object *o) {

diff --git a/dyninstAPI/src/Parsing.h b/dyninstAPI/src/Parsing.h
@@ -54,7 +54,7 @@ namespace ParseAPI {
 class DynCFGFactory : public Dyninst::ParseAPI::CFGFactory {
   public:
     DynCFGFactory(image * im);
-    ~DynCFGFactory() {};
+    virtual ~DynCFGFactory() {};
 
     ParseAPI::Function * mkfunc(Address addr, FuncSource src, std::string name,
             ParseAPI::CodeObject * obj, ParseAPI::CodeRegion * reg,

diff --git a/dyninstAPI/src/Relocation/DynCFGMaker.C b/dyninstAPI/src/Relocation/DynCFGMaker.C
@@ -94,13 +94,13 @@ PatchEdge* DynCFGMaker::makeEdge(ParseAPI::Edge* e,
   mapped_object* moS = NULL;
   mapped_object* moT = NULL;
   if (!s) {
-      if (e->src()->obj() == o->co()) 
+      if (e->src()->obj() == o->co().get())
           moS = SCAST_MO(o);
       else 
           moS = SCAST_MO(o)->as()->findObject(e->src()->obj());
   }
   if (!t) {
-      if (e->trg()->obj() == o->co()) 
+      if (e->trg()->obj() == o->co().get())
           moT = SCAST_MO(o);
       else 
           moT = SCAST_MO(o)->as()->findObject(e->trg()->obj());

diff --git a/dyninstAPI/src/Relocation/DynObject.C b/dyninstAPI/src/Relocation/DynObject.C
@@ -41,7 +41,7 @@ using Dyninst::PatchAPI::InstancePtr;
 using Dyninst::PatchAPI::DynCFGMakerPtr;
 using Dyninst::PatchAPI::DynCFGMaker;
 
-DynObject::DynObject(ParseAPI::CodeObject* co, AddressSpace* as, Address base)
+DynObject::DynObject(boost::shared_ptr<ParseAPI::CodeObject> co, AddressSpace* as, Address base)
    : PatchObject(co, base, 
                  new DynCFGMaker, 
                  new DynPatchCallback),

diff --git a/dyninstAPI/src/Relocation/DynObject.h b/dyninstAPI/src/Relocation/DynObject.h
@@ -42,12 +42,12 @@ namespace PatchAPI {
 class DynObject : public PatchObject {
 
   public:
-    static DynObject* create(ParseAPI::CodeObject* co,
+    static DynObject* create(boost::shared_ptr<ParseAPI::CodeObject> co,
                              AddressSpace* as,
                              Address base) {
       return (new DynObject(co, as, base));
     }
-    DynObject(ParseAPI::CodeObject* co, AddressSpace* as, Address base);
+    DynObject(boost::shared_ptr<ParseAPI::CodeObject> co, AddressSpace* as, Address base);
     DynObject(const DynObject *par_obj, AddressSpace* child, Address base);
     ~DynObject();
 

diff --git a/dyninstAPI/src/addressSpace.C b/dyninstAPI/src/addressSpace.C
@@ -28,6 +28,9 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include "os.h"
+
+
 #include "addressSpace.h"
 #include "codeRange.h"
 #include "dynProcess.h"
@@ -63,6 +66,10 @@
 
 #include <boost/bind.hpp>
 
+#include "dynThread.h"
+#include "pcEventHandler.h"
+
+
 // Implementations of non-virtual functions in the address space
 // class.
 
@@ -88,8 +95,7 @@ AddressSpace::AddressSpace () :
     memEmulator_(NULL),
     emulateMem_(false),
     emulatePC_(false),
-    delayRelocation_(false),
-    patcher_(NULL)
+    delayRelocation_(false)
 {
 #if 0
    // Disabled for now; used by defensive mode
@@ -103,10 +109,8 @@ AddressSpace::AddressSpace () :
 }
 
 AddressSpace::~AddressSpace() {
-    if (memEmulator_)
-      delete memEmulator_;
-    if (mgr_)
-       static_cast<DynAddrSpace*>(mgr_->as())->removeAddrSpace(this);
+    cerr << "Destroying AddressSpace " << hex << this << dec << " for " << getAOut()->fileName() << endl;
+    deleteAddressSpace();
 }
 
 PCProcess *AddressSpace::proc() {
@@ -252,7 +256,6 @@ void AddressSpace::deleteAddressSpace() {
 
    // bool heapInitialized_
    // inferiorHeap heap_
-
    heapInitialized_ = false;
    heap_.clear();
    for (unsigned i = 0; i < mapped_objects.size(); i++) 
@@ -877,7 +880,7 @@ mapped_object *AddressSpace::findObject(Address addr) const {
 
 mapped_object *AddressSpace::findObject(const ParseAPI::CodeObject *co) const {
    mapped_object *obj = 
-      findObject(static_cast<ParseAPI::SymtabCodeSource*>(co->cs())->
+      findObject(static_cast<ParseAPI::SymtabCodeSource*>(co->cs().get())->
                  getSymtabObject()->file());
    return obj;
 }
@@ -2321,3 +2324,12 @@ bool uninstrument(Dyninst::PatchAPI::Instance::Ptr inst) {
    return true;
 
 }
+
+
+unsigned AddressSpace::getAddressWidth() const {
+    if( mapped_objects.size() > 0 ) {
+        return mapped_objects[0]->parse_img()->codeObject()->cs()->getAddressWidth();
+    }
+    // We can call this before we've attached...best effort guess
+    return sizeof(Address);
+}
diff --git a/dyninstAPI/src/addressSpace.h b/dyninstAPI/src/addressSpace.h
@@ -192,7 +192,7 @@ class AddressSpace : public InstructionSource {
     virtual bool isValidAddress(const Address) const;
     virtual void *getPtrToInstruction(const Address) const;
     virtual void *getPtrToData(const Address a) const { return getPtrToInstruction(a); }
-    virtual unsigned getAddressWidth() const = 0;
+
     bool usesDataLoadAddress() const; // OS-specific
     virtual bool isCode(const Address) const;
     virtual bool isData(const Address) const;
@@ -373,7 +373,11 @@ class AddressSpace : public InstructionSource {
     //////////////////////////////////////////////////////
     // Callbacks for higher level code (like BPatch) to learn about new 
     //  functions and InstPoints.
- private:
+    /* AddressSpace pure virtual implementation */
+    unsigned getAddressWidth() const;
+
+
+private:
     BPatch_function *(*new_func_cb)(AddressSpace *a, Dyninst::PatchAPI::PatchFunction *f);
     BPatch_point *(*new_instp_cb)(AddressSpace *a, Dyninst::PatchAPI::PatchFunction *f, 
                                   Dyninst::PatchAPI::Point *ip, 
@@ -565,14 +569,14 @@ class AddressSpace : public InstructionSource {
   public:
     Dyninst::PatchAPI::PatchMgrPtr mgr() const { assert(mgr_); return mgr_; }
     void setMgr(Dyninst::PatchAPI::PatchMgrPtr m) { mgr_ = m; }
-    void setPatcher(Dyninst::PatchAPI::Patcher* p) { patcher_ = p; }
+    void setPatcher(Dyninst::PatchAPI::Patcher::Ptr p) { patcher_ = p; }
     void initPatchAPI();
     void addMappedObject(mapped_object* obj);
-    Dyninst::PatchAPI::Patcher* patcher() { return patcher_; }
+    Dyninst::PatchAPI::Patcher::Ptr patcher() { return patcher_; }
     static bool patch(AddressSpace*);
   protected:
     Dyninst::PatchAPI::PatchMgrPtr mgr_;
-    Dyninst::PatchAPI::Patcher* patcher_;
+    Dyninst::PatchAPI::Patcher::Ptr patcher_;
 };
 
 

diff --git a/dyninstAPI/src/binaryEdit.C b/dyninstAPI/src/binaryEdit.C
@@ -255,10 +255,6 @@ Architecture BinaryEdit::getArch() const {
   return mapped_objects[0]->parse_img()->codeObject()->cs()->getArch();
 }
 
-unsigned BinaryEdit::getAddressWidth() const {
-  assert(!mapped_objects.empty());
-  return mapped_objects[0]->parse_img()->codeObject()->cs()->getAddressWidth();
-}
 Address BinaryEdit::offset() const {
     fprintf(stderr,"error BinaryEdit::offset() unimpl\n");
     return 0;
@@ -290,26 +286,27 @@ BinaryEdit::BinaryEdit() :
 
 BinaryEdit::~BinaryEdit() 
 {
-}
-
-void BinaryEdit::deleteBinaryEdit() {
-    deleteAddressSpace();
     highWaterMark_ = 0;
     lowWaterMark_ = 0;
 
-    // TODO: is this cleanup necessary?
-    depRelocation *rel;
-    while (dependentRelocations.size() > 0) {
-        rel = dependentRelocations[0];
-        dependentRelocations.erase(dependentRelocations.begin());
-        delete rel;
+    for(auto i = dependentRelocations.begin();
+            i != dependentRelocations.end();
+            ++i)
+    {
+        delete (*i);
+    }
+    for (auto mt = trackerFreeList.begin();
+            mt != trackerFreeList.end();
+            ++mt)
+    {
+        delete (*mt);
     }
     delete memoryTracker_;
 }
 
-BinaryEdit *BinaryEdit::openFile(const std::string &file, 
+BinaryEdit *BinaryEdit::openFile(const std::string &file,
                                  PatchMgrPtr mgr, 
-                                 Dyninst::PatchAPI::Patcher *patch,
+                                 Dyninst::PatchAPI::Patcher::Ptr patch,
                                  const std::string &member) {
     if (!OS::executableExists(file)) {
         startup_printf("%s[%d]:  failed to read file %s\n", FILE__, __LINE__, file.c_str());
@@ -762,13 +759,11 @@ bool BinaryEdit::createMemoryBackingStore(mapped_object *obj) {
       {
          continue;
       }
-      else {
-         newTracker = new memoryTracker(regs[i]->getMemOffset(),
-                                        regs[i]->getMemSize(),
-                                        regs[i]->getPtrToRawData());
-
-      }
+     newTracker = new memoryTracker(regs[i]->getMemOffset(),
+                                    regs[i]->getMemSize(),
+                                    regs[i]->getPtrToRawData());
       newTracker->alloced = false;
+       trackerFreeList.push_back(newTracker);
       if (!memoryTracker_)
          memoryTracker_ = new codeRangeTree();
       memoryTracker_->insert(newTracker);

diff --git a/dyninstAPI/src/binaryEdit.h b/dyninstAPI/src/binaryEdit.h
@@ -100,8 +100,6 @@ class BinaryEdit : public AddressSpace {
     virtual void inferiorFree(Address item);
     virtual bool inferiorRealloc(Address item, unsigned newSize);
 
-    /* AddressSpace pure virtual implementation */
-    unsigned getAddressWidth() const;
     Address offset() const;
     Address length() const;
     Architecture getArch() const;
@@ -138,13 +136,10 @@ class BinaryEdit : public AddressSpace {
     BinaryEdit();
     ~BinaryEdit();
 
-    // Same usage pattern as process
-    void deleteBinaryEdit();
-
     // And the "open" factory method.
     static BinaryEdit *openFile(const std::string &file,
                                 Dyninst::PatchAPI::PatchMgrPtr mgr = Dyninst::PatchAPI::PatchMgrPtr(),
-                                Dyninst::PatchAPI::Patcher *patch = NULL,
+                                Dyninst::PatchAPI::Patcher::Ptr patch = Dyninst::PatchAPI::Patcher::Ptr(),
                                 const std::string &member = "");
 
     bool writeFile(const std::string &newFileName);
@@ -191,7 +186,7 @@ class BinaryEdit : public AddressSpace {
 
    virtual void addTrap(Address from, Address to, codeGen &gen);
    virtual void removeTrap(Address /*from*/) {};
-    static bool getResolvedLibraryPath(const std::string &filename, std::vector<std::string> &paths);
+    static bool getResolvedLibraryPath(const std::string &filename, set<std::string> &paths);
 
  private:
     Address highWaterMark_;
@@ -218,7 +213,7 @@ class BinaryEdit : public AddressSpace {
    bool doStaticBinarySpecialCases();
 
     codeRangeTree* memoryTracker_;
-
+    std::vector<memoryTracker*> trackerFreeList;
     mapped_object * addSharedObject(const std::string *fullPath);
 
     std::vector<depRelocation *> dependentRelocations;
@@ -234,6 +229,7 @@ class BinaryEdit : public AddressSpace {
 
     // Symbols that other people (e.g., functions) want us to add
     std::vector<SymtabAPI::Symbol *> newDyninstSyms_;
+    bool isCompatibleBinary(std::string pathname);
 
 };
 
@@ -252,16 +248,16 @@ class memoryTracker : public codeRange {
  public:
     memoryTracker(Address a, unsigned s) :
         alloced(false),  dirty(false), a_(a), s_(s) {
-        b_ = malloc(s_);
+        b_ = calloc(s_, 1);
     }
 
     memoryTracker(Address a, unsigned s, void *b) :
     alloced(false), dirty(false), a_(a), s_(s)
         {
-            b_ = malloc(s_);
+            b_ = calloc(s_, 1);
             memcpy(b_, b, s_);
         }
-    ~memoryTracker() { free(b_); }
+    virtual ~memoryTracker() { free(b_); }
 
     Address get_address() const { return a_; }
     unsigned get_size() const { return s_; }

diff --git a/dyninstAPI/src/codeRange.C b/dyninstAPI/src/codeRange.C
@@ -498,7 +498,7 @@ void codeRange::print_range(Address) {
    if (func_ptr && !mapped_ptr)
       mapped_ptr = func_ptr->obj();
    if (mapped_ptr && !img_ptr)
-      img_ptr = mapped_ptr->parse_img();
+      img_ptr = mapped_ptr->parse_img().get();
 
    fprintf(stderr, "[");
 

diff --git a/dyninstAPI/src/dynProcess.C b/dyninstAPI/src/dynProcess.C
@@ -1503,15 +1503,6 @@ int PCProcess::incrementThreadIndex() {
     return ret;
 }
 
-unsigned PCProcess::getAddressWidth() const {
-    if( mapped_objects.size() > 0 ) {
-        return mapped_objects[0]->parse_img()->codeObject()->cs()->getAddressWidth();
-    }
-
-    // We can call this before we've attached...best effort guess
-    return sizeof(Address);
-}
-
 PCEventHandler * PCProcess::getPCEventHandler() const {
     return eventHandler_;
 }

diff --git a/dyninstAPI/src/dynProcess.h b/dyninstAPI/src/dynProcess.h
@@ -41,14 +41,11 @@
 #include <set>
 
 #include "addressSpace.h"
-#include "dynThread.h"
-#include "pcEventHandler.h"
 #include "inst.h"
 #include "codeRange.h"
 #include "infHeap.h"
 #include "ast.h"
 #include "syscallNotification.h"
-#include "os.h"
 #include "baseTramp.h"
 
 #include "Symtab.h"
@@ -59,6 +56,8 @@
 #include "stackwalk/h/walker.h"
 #include "stackwalk/h/framestepper.h"
 #include "stackwalk/h/symlookup.h"
+#include "pcEventHandler.h"
+#include "frame.h"
 
 #define RPC_LEAVE_AS_IS 0
 #define RPC_RUN_WHEN_DONE 1
@@ -154,7 +153,7 @@ class PCProcess : public AddressSpace {
     bool removeThread(dynthread_t tid);
 
     int getPid() const;
-    unsigned getAddressWidth() const;
+
     bool wasRunningWhenAttached() const;
     bool wasCreatedViaAttach() const;
     bool wasCreatedViaFork() const;