Fixed memory leaks following ProcessTxBaseMsg refactoring

HorizenOfficial · Mar 30, 2021 · 8a4f8ed · 8a4f8ed
1 parent 8982926
commit 8a4f8ed
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 44 deletions.
diff --git a/src/gtest/test_processTxBaseMsg.cpp b/src/gtest/test_processTxBaseMsg.cpp
@@ -144,6 +144,9 @@ class ProcessTxBaseMsgTestSuite : public ::testing::Test
         UnloadBlockIndex();
         mapRelay.clear();
 
+        delete pcoinsTip;
+        pcoinsTip = nullptr;
+
         delete pChainStateDb;
         pChainStateDb = nullptr;
     }

diff --git a/src/main.cpp b/src/main.cpp
@@ -5600,40 +5600,10 @@ struct TxBaseMsg_DataToProcess
     NodeId sourceNodeId;
 
     // optional data, only for txes which has not been processed yet, just to inform sender
-    CTransactionBase* pTxBase; //owning pointer
+    std::shared_ptr<const CTransactionBase> pTxBase; //owning pointer
     CNodeInterface* pSourceNode; //non-owning pointer. Null if source node already got its answer and we do not need to send any message to it
 
     TxBaseMsg_DataToProcess(): txBaseHash(), sourceNodeId(-1), pTxBase(nullptr), pSourceNode(nullptr) {};
-
-    TxBaseMsg_DataToProcess(const TxBaseMsg_DataToProcess& rhs)
-    {
-        this->txBaseHash = rhs.txBaseHash;
-        this->sourceNodeId = rhs.sourceNodeId;
-        this->pTxBase = rhs.pTxBase == nullptr? nullptr: rhs.pTxBase->clone();
-        this->pSourceNode = rhs.pSourceNode;
-    }
-
-    TxBaseMsg_DataToProcess& operator=(const TxBaseMsg_DataToProcess& rhs)
-    {
-        if (this != &rhs)
-        {
-            this->txBaseHash = rhs.txBaseHash;
-            this->sourceNodeId = rhs.sourceNodeId;
-            this->pTxBase = rhs.pTxBase == nullptr? nullptr: rhs.pTxBase->clone();
-            this->pSourceNode = rhs.pSourceNode;
-        }
-        return *this;
-    }
-
-    ~TxBaseMsg_DataToProcess()
-    {
-        // Delete pTxBase since it's owning ptr
-        delete pTxBase;
-        pTxBase = nullptr;
-
-        // No delete, since it's non-owning pointer
-        pSourceNode = nullptr;
-    };
 };
 static std::vector<TxBaseMsg_DataToProcess> processTxBaseMsg_WorkQueue;
 
@@ -5665,10 +5635,10 @@ void addTxBaseMsgToProcess(const CTransactionBase& txBase, CNodeInterface* pfrom
     }
 
     TxBaseMsg_DataToProcess dataToAdd;
-    dataToAdd.txBaseHash = txBase.GetHash();
+    dataToAdd.txBaseHash   = txBase.GetHash();
     dataToAdd.sourceNodeId = pfrom->GetId();
-    dataToAdd.pTxBase = txBase.clone();
-    dataToAdd.pSourceNode = pfrom;
+    dataToAdd.pTxBase      = txBase.MakeShared();
+    dataToAdd.pSourceNode  = pfrom;
     processTxBaseMsg_WorkQueue.push_back(dataToAdd);
     return;
 }
@@ -5681,10 +5651,10 @@ void ProcessTxBaseMsg(const processMempoolTx& mempoolProcess)
 
     while (!processTxBaseMsg_WorkQueue.empty())
     {
-        const uint256& hashToProcess = processTxBaseMsg_WorkQueue.at(0).txBaseHash;
-        NodeId sourceNodeId = processTxBaseMsg_WorkQueue.at(0).sourceNodeId; //just an int, better copy than reference
+        const uint256& hashToProcess        = processTxBaseMsg_WorkQueue.at(0).txBaseHash;
+        NodeId sourceNodeId                 = processTxBaseMsg_WorkQueue.at(0).sourceNodeId;
         const CTransactionBase& txToProcess = *processTxBaseMsg_WorkQueue.at(0).pTxBase;
-        CNodeInterface* pSourceNode =  processTxBaseMsg_WorkQueue.at(0).pSourceNode;
+        CNodeInterface* pSourceNode         =  processTxBaseMsg_WorkQueue.at(0).pSourceNode;
 
         if (setMisbehaving.count(sourceNodeId))
         {
@@ -5715,10 +5685,10 @@ void ProcessTxBaseMsg(const processMempoolTx& mempoolProcess)
             for(const uint256& orphanHash: unlockedOrphansIt->second)
             {
                 TxBaseMsg_DataToProcess dataToAdd;
-                dataToAdd.txBaseHash = orphanHash;
+                dataToAdd.txBaseHash   = orphanHash;
                 dataToAdd.sourceNodeId = mapOrphanTransactions.at(orphanHash).fromPeer;
-                dataToAdd.pTxBase = mapOrphanTransactions.at(orphanHash).tx->clone(); //Clone currently necessary since ptr ownership is assumed
-                dataToAdd.pSourceNode = nullptr;
+                dataToAdd.pTxBase      = mapOrphanTransactions.at(orphanHash).tx;
+                dataToAdd.pSourceNode  = nullptr;
 
                 processTxBaseMsg_WorkQueue.push_back(dataToAdd);
             }

diff --git a/src/primitives/certificate.h b/src/primitives/certificate.h
@@ -69,7 +69,6 @@ class CScCertificate : public CTransactionBase
     CScCertificate(int versionIn = SC_CERT_VERSION);
     CScCertificate(const CScCertificate& tx);
     CScCertificate& operator=(const CScCertificate& tx);
-    CScCertificate* clone() const override final { return new CScCertificate(*this); };
     ~CScCertificate() = default;
 
     /** Convert a CMutableScCertificate into a CScCertificate.  */

diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
@@ -710,8 +710,6 @@ class CTransactionBase
     CTransactionBase& operator=(const CTransactionBase& tx);
 
     explicit CTransactionBase(const CMutableTransactionBase& mutTxBase);
-    virtual CTransactionBase* clone() const = 0;
-
     virtual ~CTransactionBase() = default;
 
     template <typename Stream>
@@ -856,7 +854,6 @@ class CTransaction : public CTransactionBase
     CTransaction(int nVersionIn = TRANSPARENT_TX_VERSION);
     CTransaction& operator=(const CTransaction& tx);
     CTransaction(const CTransaction& tx);
-    CTransaction* clone() const override final { return new CTransaction(*this); };
     ~CTransaction() = default;
 
     /** Convert a CMutableTransaction into a CTransaction. */