vmware / concord-bft

Concord byzantine fault tolerant state machine replication library
378 stars 149 forks source link

Memory leak at PartialProofsSet::tryToCreateFullProof #501

Closed cloudnoize closed 1 year ago

cloudnoize commented 4 years ago

Describe the bug At PartialProofsSet::tryToCreateFullProof() the line - IThresholdAccumulator* acc = thresholdAccumulator->clone() creates a new object by calling - virtual IThresholdAccumulator clone() { return new BlsThresholdAccumulator(this); } (file BlsThresholdAccumulator.h)

The pointer is being passed to AsynchProofCreationJob for async execution, but I don't see any code that deletes it.

Expected behavior delete the created object by using std ::unique_ptr

cloudnoize commented 4 years ago

Actually the issue is even more profound - The job that wraps the IThresholdAccumulator, AsynchProofCreationJob , doesn't get deleted after its execution as well.

When deleting AsynchProofCreationJob or the IThresholdAccumulator, the replica crashed on PartialProofsSet::resetAndFree().

`#1 0x000055e7e1d5404e in bftEngine::impl::PartialProofsSet::resetAndFree (this=0x55e7e26a53c0) at /concord-bft/bftengine/src/bftengine/messages/PartialProofsSet.cpp:45

2 0x000055e7e1d5c3e2 in bftEngine::impl::SeqNumInfo::resetAndFree (this=0x55e7e269cfc8) at /concord-bft/bftengine/src/bftengine/SeqNumInfo.cpp:48

3 0x000055e7e1cb0448 in bftEngine::impl::SeqNumInfo::reset (i=...) at /concord-bft/bftengine/src/bftengine/SeqNumInfo.hpp:187

4 0x000055e7e1cb80eb in bftEngine::impl::SequenceWithActiveWindow<(unsigned short)300, (unsigned short)1, long, bftEngine::impl::SeqNumInfo, bftEngine::impl::SeqNumInfo>::advanceActiveWindow (this=0x55e7e269c9d0,

newFirstIndexOfActiveWindow=151) at /concord-bft/bftengine/src/bftengine/SequenceWithActiveWindow.hpp:104

5 0x000055e7e1ca31a0 in bftEngine::impl::ReplicaImp::onSeqNumIsStable (this=0x55e7e268be60, newStableSeqNum=150, hasStateInformation=true, oldSeqNum=false) at /concord-bft/bftengine/src/bftengine/ReplicaImp.cpp:2179

6 0x000055e7e1ca21b2 in bftEngine::impl::ReplicaImp::sendCheckpointIfNeeded (this=0x55e7e268be60) at /concord-bft/bftengine/src/bftengine/ReplicaImp.cpp:2060

7 0x000055e7e1cad5c4 in bftEngine::impl::ReplicaImp::executeRequestsInPrePrepareMsg (this=0x55e7e268be60, ppMsg=0x7fcca8018520, recoverFromErrorInRequestsExecution=false) at /concord-bft/bftengine/src/bftengine/ReplicaImp.cpp:3157

8 0x000055e7e1cae0d8 in bftEngine::impl::ReplicaImp::executeNextCommittedRequests (this=0x55e7e268be60, requestMissingInfo=false) at /concord-bft/bftengine/src/bftengine/ReplicaImp.cpp:3206

9 0x000055e7e1c9902e in bftEngine::impl::ReplicaImp::onMessage (this=0x55e7e268be60, msg=0x7fcca8018590) at /concord-bft/bftengine/src/bftengine/ReplicaImp.cpp:758

10 0x000055e7e1cb3cbd in bftEngine::impl::ReplicaImp::messageHandler (this=0x55e7e268be60, msg=0x7fcca8018590) at /concord-bft/bftengine/src/bftengine/ReplicaImp.cpp:97`

teoparvanov commented 1 year ago

This code has been refactored, and therefore the issue is no longer applicable.