From ab7a83b8891f50785d4ccad632f2486b69d36652 Mon Sep 17 00:00:00 2001 From: Pankaj Date: Wed, 13 Apr 2022 12:07:59 +0530 Subject: [PATCH] Review comments addressed. --- bftengine/src/bcstatetransfer/BCStateTran.cpp | 12 ++++++------ bftengine/src/bcstatetransfer/RVBManager.cpp | 4 ++-- .../src/bcstatetransfer/RangeValidationTree.cpp | 4 ++-- bftengine/src/bftengine/NullStateTransfer.cpp | 2 +- bftengine/src/bftengine/ReplicaImp.cpp | 6 ++++-- bftengine/src/bftengine/SeqNumInfo.cpp | 9 ++++++--- bftengine/src/bftengine/messages/PrePrepareMsg.cpp | 4 ++-- bftengine/src/bftengine/messages/SignedShareMsgs.cpp | 3 ++- bftengine/src/bftengine/messages/ViewChangeMsg.cpp | 2 +- bftengine/tests/messages/PrePrepareMsg_test.cpp | 4 ++-- .../strategy/MangledPreProcessResultMsgStrategy.cpp | 4 ++-- .../strategy/ShufflePrePrepareMsgStrategy.cpp | 4 ++-- util/include/cryptopp_digest_creator.hpp | 10 +++++----- util/include/digest_creator.hpp | 10 +++++----- util/include/digest_holder.hpp | 6 +++--- util/include/evp_hash.hpp | 3 --- util/include/openssl_digest_creator.ipp | 7 ++----- util/src/cryptopp_digest_creator.cpp | 10 +++++----- 18 files changed, 52 insertions(+), 52 deletions(-) diff --git a/bftengine/src/bcstatetransfer/BCStateTran.cpp b/bftengine/src/bcstatetransfer/BCStateTran.cpp index 51b2e821fe..da4008f979 100644 --- a/bftengine/src/bcstatetransfer/BCStateTran.cpp +++ b/bftengine/src/bcstatetransfer/BCStateTran.cpp @@ -3880,17 +3880,17 @@ void BCStateTran::checkStoredCheckpoints(uint64_t firstStoredCheckpoint, uint64_ void BCStateTran::computeDigestOfPage( const uint32_t pageId, const uint64_t checkpointNumber, const char *page, uint32_t pageSize, Digest &outDigest) { DigestGenerator digestGenerator; - digestGenerator.updateDigest(reinterpret_cast(&pageId), sizeof(pageId)); - digestGenerator.updateDigest(reinterpret_cast(&checkpointNumber), sizeof(checkpointNumber)); + digestGenerator.update(reinterpret_cast(&pageId), sizeof(pageId)); + digestGenerator.update(reinterpret_cast(&checkpointNumber), sizeof(checkpointNumber)); if (checkpointNumber > 0) { - digestGenerator.updateDigest(page, pageSize); + digestGenerator.update(page, pageSize); } digestGenerator.writeDigest(reinterpret_cast(&outDigest)); } void BCStateTran::computeDigestOfPagesDescriptor(const DataStore::ResPagesDescriptor *pagesDesc, Digest &outDigest) { DigestGenerator digestGenerator; - digestGenerator.updateDigest(reinterpret_cast(pagesDesc), pagesDesc->size()); + digestGenerator.update(reinterpret_cast(pagesDesc), pagesDesc->size()); digestGenerator.writeDigest(reinterpret_cast(&outDigest)); } @@ -3901,8 +3901,8 @@ void BCStateTran::computeDigestOfBlockImpl(const uint64_t blockNum, ConcordAssertGT(blockNum, 0); ConcordAssertGT(blockSize, 0); DigestGenerator digestGenerator; - digestGenerator.updateDigest(reinterpret_cast(&blockNum), sizeof(blockNum)); - digestGenerator.updateDigest(block, blockSize); + digestGenerator.update(reinterpret_cast(&blockNum), sizeof(blockNum)); + digestGenerator.update(block, blockSize); digestGenerator.writeDigest(outDigest); } diff --git a/bftengine/src/bcstatetransfer/RVBManager.cpp b/bftengine/src/bcstatetransfer/RVBManager.cpp index bd79b9538a..332a0486bc 100644 --- a/bftengine/src/bcstatetransfer/RVBManager.cpp +++ b/bftengine/src/bcstatetransfer/RVBManager.cpp @@ -630,8 +630,8 @@ void RVBManager::computeDigestOfBlock(const uint64_t block_id, ConcordAssertGT(block_id, 0); ConcordAssertGT(block_size, 0); DigestGenerator digest_generator; - digest_generator.updateDigest(reinterpret_cast(&block_id), sizeof(block_id)); - digest_generator.updateDigest(block, block_size); + digest_generator.update(reinterpret_cast(&block_id), sizeof(block_id)); + digest_generator.update(block, block_size); digest_generator.writeDigest(out_digest); } diff --git a/bftengine/src/bcstatetransfer/RangeValidationTree.cpp b/bftengine/src/bcstatetransfer/RangeValidationTree.cpp index 695a7e6af6..6ada47d8b7 100644 --- a/bftengine/src/bcstatetransfer/RangeValidationTree.cpp +++ b/bftengine/src/bcstatetransfer/RangeValidationTree.cpp @@ -152,8 +152,8 @@ const shared_ptr RVBNode::computeNodeInitialValue(NodeInfo& node_info, c ConcordAssertGT(node_info.id(), 0); DigestGenerator digest_generator; - digest_generator.updateDigest(reinterpret_cast(&node_info.id_data_), sizeof(node_info.id_data_)); - digest_generator.updateDigest(data, data_size); + digest_generator.update(reinterpret_cast(&node_info.id_data_), sizeof(node_info.id_data_)); + digest_generator.update(data, data_size); // TODO - Use default_delete in case memleak is reported by ASAN static std::shared_ptr out_digest_buff(new char[NodeVal::kDigestContextOutputSize]); digest_generator.writeDigest(out_digest_buff.get()); diff --git a/bftengine/src/bftengine/NullStateTransfer.cpp b/bftengine/src/bftengine/NullStateTransfer.cpp index c93b0226ad..19c23184b5 100644 --- a/bftengine/src/bftengine/NullStateTransfer.cpp +++ b/bftengine/src/bftengine/NullStateTransfer.cpp @@ -64,7 +64,7 @@ void NullStateTransfer::getDigestOfCheckpoint(uint64_t checkpointNumber, Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest((char*)&checkpointNumber, sizeof(checkpointNumber), (char*)&d, sizeof(d)); + digestGenerator.compute((char*)&checkpointNumber, sizeof(checkpointNumber), (char*)&d, sizeof(d)); memset(outStateDigest, 0, sizeOfDigestBuffer); memset(outResPagesDigest, 0, sizeOfDigestBuffer); diff --git a/bftengine/src/bftengine/ReplicaImp.cpp b/bftengine/src/bftengine/ReplicaImp.cpp index 404d83ddc7..c31f6d78c2 100644 --- a/bftengine/src/bftengine/ReplicaImp.cpp +++ b/bftengine/src/bftengine/ReplicaImp.cpp @@ -1340,7 +1340,8 @@ void ReplicaImp::sendPartialProof(SeqNumInfo &seqNumInfo) { else commitSigner = CryptoManager::instance().thresholdSignerForOptimisticCommit(seqNum); - Digest tmpDigest, digestHelper; + Digest tmpDigest; + Digest digestHelper; digestHelper.calcCombination(ppDigest, getCurrentView(), seqNum, tmpDigest); const auto &span_context = pp->spanContext::type>(); @@ -4117,7 +4118,8 @@ ReplicaImp::ReplicaImp(const LoadedReplicaData &ld, else commitSigner = CryptoManager::instance().thresholdSignerForOptimisticCommit(seqNum); - Digest tmpDigest, digestHelper; + Digest tmpDigest; + Digest digestHelper; digestHelper.calcCombination(ppDigest, getCurrentView(), seqNum, tmpDigest); PartialCommitProofMsg *p = new PartialCommitProofMsg( diff --git a/bftengine/src/bftengine/SeqNumInfo.cpp b/bftengine/src/bftengine/SeqNumInfo.cpp index e354a8170a..8db861c2ca 100644 --- a/bftengine/src/bftengine/SeqNumInfo.cpp +++ b/bftengine/src/bftengine/SeqNumInfo.cpp @@ -102,7 +102,8 @@ bool SeqNumInfo::addMsg(PrePrepareMsg* m, bool directAdd, bool isTimeCorrect) { isTimeCorrect_ = isTimeCorrect; // set expected - Digest tmpDigest, digestHelper; + Digest tmpDigest; + Digest digestHelper; digestHelper.calcCombination(m->digestOfRequests(), m->viewNumber(), m->seqNumber(), tmpDigest); if (!directAdd) prepareSigCollector->setExpected(m->seqNumber(), m->viewNumber(), tmpDigest); @@ -128,7 +129,8 @@ bool SeqNumInfo::addSelfMsg(PrePrepareMsg* m, bool directAdd) { primary = true; // set expected - Digest tmpDigest, digestHelper; + Digest tmpDigest; + Digest digestHelper; digestHelper.calcCombination(m->digestOfRequests(), m->viewNumber(), m->seqNumber(), tmpDigest); if (!directAdd) prepareSigCollector->setExpected(m->seqNumber(), m->viewNumber(), tmpDigest); @@ -194,7 +196,8 @@ bool SeqNumInfo::addSelfCommitPartialMsgAndDigest(CommitPartialMsg* m, Digest& c ConcordAssert(replica->getReplicasInfo().myId() == m->senderId()); ConcordAssert(!forcedCompleted); - Digest tmpDigest, digestHelper; + Digest tmpDigest; + Digest digestHelper; digestHelper.calcCombination(commitDigest, m->viewNumber(), m->seqNumber(), tmpDigest); bool r; if (!directAdd) { diff --git a/bftengine/src/bftengine/messages/PrePrepareMsg.cpp b/bftengine/src/bftengine/messages/PrePrepareMsg.cpp index a93a6dd446..f7ac8a07fe 100644 --- a/bftengine/src/bftengine/messages/PrePrepareMsg.cpp +++ b/bftengine/src/bftengine/messages/PrePrepareMsg.cpp @@ -59,7 +59,7 @@ void PrePrepareMsg::calculateDigestOfRequests(Digest& digest) const { tasks.push_back(threadPool.async( [&sigOrDigestOfRequest, &digestBuffer, local_id](auto* request, auto requestLength) { DigestGenerator digestGenerator; - digestGenerator.computeDigest( + digestGenerator.compute( request, requestLength, digestBuffer.get() + local_id * sizeof(Digest), sizeof(Digest)); sigOrDigestOfRequest[local_id].first = digestBuffer.get() + local_id * sizeof(Digest); sigOrDigestOfRequest[local_id].second = sizeof(Digest); @@ -80,7 +80,7 @@ void PrePrepareMsg::calculateDigestOfRequests(Digest& digest) const { // compute and set digest DigestGenerator digestGenerator; - digestGenerator.computeDigest(sigOrDig.c_str(), sigOrDig.size(), (char*)&digest, sizeof(Digest)); + digestGenerator.compute(sigOrDig.c_str(), sigOrDig.size(), (char*)&digest, sizeof(Digest)); } catch (std::out_of_range& ex) { throw std::runtime_error(__PRETTY_FUNCTION__ + std::string(": digest threadpool")); } diff --git a/bftengine/src/bftengine/messages/SignedShareMsgs.cpp b/bftengine/src/bftengine/messages/SignedShareMsgs.cpp index 026032a282..dc67447088 100644 --- a/bftengine/src/bftengine/messages/SignedShareMsgs.cpp +++ b/bftengine/src/bftengine/messages/SignedShareMsgs.cpp @@ -47,7 +47,8 @@ SignedShareBase* SignedShareBase::create(int16_t type, m->b()->epochNum = EpochManager::instance().getSelfEpochNumber(); m->b()->thresSigLength = (uint16_t)sigLen; - Digest tmpDigest, digestHelper; + Digest tmpDigest; + Digest digestHelper; digestHelper.calcCombination(digest, v, s, tmpDigest); auto position = m->body() + sizeof(Header); diff --git a/bftengine/src/bftengine/messages/ViewChangeMsg.cpp b/bftengine/src/bftengine/messages/ViewChangeMsg.cpp index e044a41446..40de987b2e 100644 --- a/bftengine/src/bftengine/messages/ViewChangeMsg.cpp +++ b/bftengine/src/bftengine/messages/ViewChangeMsg.cpp @@ -52,7 +52,7 @@ void ViewChangeMsg::getMsgDigest(Digest& outDigest) const { auto bodySize = getBodySize(); bodySize += b()->sizeOfAllComplaints; DigestGenerator digestGenerator; - digestGenerator.computeDigest(body(), bodySize, (char*)outDigest.content(), sizeof(Digest)); + digestGenerator.compute(body(), bodySize, (char*)outDigest.content(), sizeof(Digest)); } uint32_t ViewChangeMsg::getBodySize() const { diff --git a/bftengine/tests/messages/PrePrepareMsg_test.cpp b/bftengine/tests/messages/PrePrepareMsg_test.cpp index 8b890963f6..df671aeb04 100644 --- a/bftengine/tests/messages/PrePrepareMsg_test.cpp +++ b/bftengine/tests/messages/PrePrepareMsg_test.cpp @@ -139,7 +139,7 @@ TEST_F(PrePrepareMsgTestFixture, finalize_and_validate) { msg.addRequest(crm->body(), crm->size()); Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest(crm->body(), crm->size(), (char*)&d, sizeof(Digest)); + digestGenerator.compute(crm->body(), crm->size(), (char*)&d, sizeof(Digest)); dv.push_back({d.content(), sizeof(Digest)}); } EXPECT_NO_THROW(msg.finishAddingRequests()); // create the digest @@ -151,7 +151,7 @@ TEST_F(PrePrepareMsgTestFixture, finalize_and_validate) { } Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest(dod.c_str(), dod.size(), (char*)&d, sizeof(Digest)); + digestGenerator.compute(dod.c_str(), dod.size(), (char*)&d, sizeof(Digest)); EXPECT_EQ(d, msg.digestOfRequests()); EXPECT_NO_THROW(msg.validate(replicaInfo)); // validate the same digest } diff --git a/tests/simpleKVBC/TesterReplica/strategy/MangledPreProcessResultMsgStrategy.cpp b/tests/simpleKVBC/TesterReplica/strategy/MangledPreProcessResultMsgStrategy.cpp index 7828386c2f..1021043607 100644 --- a/tests/simpleKVBC/TesterReplica/strategy/MangledPreProcessResultMsgStrategy.cpp +++ b/tests/simpleKVBC/TesterReplica/strategy/MangledPreProcessResultMsgStrategy.cpp @@ -57,7 +57,7 @@ bool concord::kvbc::strategy::MangledPreProcessResultMsgStrategy::changeMessage( } else { Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest(req.body(), req.size(), reinterpret_cast(&d), sizeof(Digest)); + digestGenerator.compute(req.body(), req.size(), reinterpret_cast(&d), sizeof(Digest)); sigOrDigestOfRequest[idx].append(d.content(), sizeof(Digest)); } idx++; @@ -71,7 +71,7 @@ bool concord::kvbc::strategy::MangledPreProcessResultMsgStrategy::changeMessage( Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest(sigOrDig.c_str(), sigOrDig.size(), reinterpret_cast(&d), sizeof(Digest)); + digestGenerator.compute(sigOrDig.c_str(), sigOrDig.size(), reinterpret_cast(&d), sizeof(Digest)); nmsg.digestOfRequests() = d; LOG_INFO(logger_, "Finally the PrePrepare Message with correlation id : " diff --git a/tests/simpleKVBC/TesterReplica/strategy/ShufflePrePrepareMsgStrategy.cpp b/tests/simpleKVBC/TesterReplica/strategy/ShufflePrePrepareMsgStrategy.cpp index 5c30ae8c20..2277184d6b 100644 --- a/tests/simpleKVBC/TesterReplica/strategy/ShufflePrePrepareMsgStrategy.cpp +++ b/tests/simpleKVBC/TesterReplica/strategy/ShufflePrePrepareMsgStrategy.cpp @@ -73,7 +73,7 @@ bool ShufflePrePrepareMsgStrategy::changeMessage(std::shared_ptr& m } else { Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest(req.body(), req.size(), reinterpret_cast(&d), sizeof(Digest)); + digestGenerator.compute(req.body(), req.size(), reinterpret_cast(&d), sizeof(Digest)); if (idx == swapIdx) { sigOrDigestOfRequest[idx + 1].append(d.content(), sizeof(Digest)); } else if (idx == (swapIdx + 1)) { @@ -98,7 +98,7 @@ bool ShufflePrePrepareMsgStrategy::changeMessage(std::shared_ptr& m Digest d; DigestGenerator digestGenerator; - digestGenerator.computeDigest(sigOrDig.c_str(), sigOrDig.size(), reinterpret_cast(&d), sizeof(Digest)); + digestGenerator.compute(sigOrDig.c_str(), sigOrDig.size(), reinterpret_cast(&d), sizeof(Digest)); nmsg.digestOfRequests() = d; LOG_INFO(logger_, "Finally the PrePrepare Message with correlation id : " diff --git a/util/include/cryptopp_digest_creator.hpp b/util/include/cryptopp_digest_creator.hpp index c5e4e1739c..b1a5c8c628 100644 --- a/util/include/cryptopp_digest_creator.hpp +++ b/util/include/cryptopp_digest_creator.hpp @@ -31,13 +31,13 @@ class CryptoppDigestCreator : public DigestCreator { CryptoppDigestCreator(); virtual ~CryptoppDigestCreator(); - void updateDigest(const char* data, const size_t len) override; + void update(const char* data, size_t len) override; void writeDigest(char* outDigest) override; size_t digestLength() const override; - bool computeDigest(const char* input, - const size_t inputLength, - char* outBufferForDigest, - const size_t lengthOfBufferForDigest) override; + bool compute(const char* input, + size_t inputLength, + char* outBufferForDigest, + size_t lengthOfBufferForDigest) override; private: void* internalState_; diff --git a/util/include/digest_creator.hpp b/util/include/digest_creator.hpp index c923e66d98..bfff275e21 100644 --- a/util/include/digest_creator.hpp +++ b/util/include/digest_creator.hpp @@ -21,12 +21,12 @@ class DigestCreator { public: virtual ~DigestCreator() = default; - virtual void updateDigest(const char* data, const size_t len) = 0; + virtual void update(const char* data, size_t len) = 0; virtual void writeDigest(char* outDigest) = 0; virtual size_t digestLength() const = 0; - virtual bool computeDigest(const char* input, - const size_t inputLength, - char* outBufferForDigest, - const size_t lengthOfBufferForDigest) = 0; + virtual bool compute(const char* input, + size_t inputLength, + char* outBufferForDigest, + size_t lengthOfBufferForDigest) = 0; }; } // namespace concord::util::digest diff --git a/util/include/digest_holder.hpp b/util/include/digest_holder.hpp index 05c85a5579..e2e74ed1d1 100644 --- a/util/include/digest_holder.hpp +++ b/util/include/digest_holder.hpp @@ -29,11 +29,11 @@ class DigestHolder { DigestHolder(const char* other) { std::memcpy(d, other, DIGEST_SIZE); } DigestHolder(char* buf, size_t len) { CREATOR digestCreator; - digestCreator.computeDigest(buf, len, (char*)d, DIGEST_SIZE); + digestCreator.compute(buf, len, (char*)d, DIGEST_SIZE); } DigestHolder(const DigestHolder& other) { std::memcpy(d, other.d, DIGEST_SIZE); } - char* content() const { return (char*)d; } // @TODO: Can be replaced by getForUpdate(). + char* content() const { return (char*)d; } void makeZero() { std::memset(d, 0, DIGEST_SIZE); } std::string toString() const { return concordUtils::bufferToHex(d, DIGEST_SIZE, false); } void print() { printf("digest=[%s]", toString().c_str()); } @@ -73,7 +73,7 @@ class DigestHolder { void digestOfDigest(const DigestHolder& inDigest, DigestHolder& outDigest) { CREATOR digestCreator; - digestCreator.computeDigest(inDigest.d, sizeof(DigestHolder), outDigest.d, sizeof(DigestHolder)); + digestCreator.compute(inDigest.d, sizeof(DigestHolder), outDigest.d, sizeof(DigestHolder)); } void calcCombination(const DigestHolder& inDigest, int64_t inDataA, int64_t inDataB, DigestHolder& outDigest) { diff --git a/util/include/evp_hash.hpp b/util/include/evp_hash.hpp index 908dad3660..26927bfa14 100644 --- a/util/include/evp_hash.hpp +++ b/util/include/evp_hash.hpp @@ -45,9 +45,6 @@ class EVPHash { }; EVPHash& operator=(EVPHash&& other) noexcept { - if (nullptr != ctx_) { // Prevent memory leak. - EVP_MD_CTX_destroy(ctx_); - } *this = EVPHash{std::move(other)}; return *this; } diff --git a/util/include/openssl_digest_creator.ipp b/util/include/openssl_digest_creator.ipp index c9659f582d..3ccb62de67 100644 --- a/util/include/openssl_digest_creator.ipp +++ b/util/include/openssl_digest_creator.ipp @@ -40,7 +40,7 @@ class OpenSSLDigestCreator : public DigestCreator { } } - void updateDigest(const char* data, const size_t len) { + void update(const char* data, size_t len) { ConcordAssert(nullptr != data); init(); @@ -57,10 +57,7 @@ class OpenSSLDigestCreator : public DigestCreator { size_t digestLength() const { return hash_ctx_.SIZE_IN_BYTES; } - bool computeDigest(const char* input, - const size_t inputLength, - char* outBufferForDigest, - const size_t lengthOfBufferForDigest) { + bool compute(const char* input, size_t inputLength, char* outBufferForDigest, size_t lengthOfBufferForDigest) { ConcordAssert(nullptr != input); ConcordAssert(nullptr != outBufferForDigest); diff --git a/util/src/cryptopp_digest_creator.cpp b/util/src/cryptopp_digest_creator.cpp index 4aba292623..7d9dc88b00 100644 --- a/util/src/cryptopp_digest_creator.cpp +++ b/util/src/cryptopp_digest_creator.cpp @@ -35,10 +35,10 @@ CryptoppDigestCreator::CryptoppDigestCreator() { size_t CryptoppDigestCreator::digestLength() const { return DigestType::DIGESTSIZE; } -bool CryptoppDigestCreator::computeDigest(const char* input, - const size_t inputLength, - char* outBufferForDigest, - const size_t lengthOfBufferForDigest) { +bool CryptoppDigestCreator::compute(const char* input, + size_t inputLength, + char* outBufferForDigest, + size_t lengthOfBufferForDigest) { DigestType dig; const size_t size = dig.DigestSize(); @@ -57,7 +57,7 @@ bool CryptoppDigestCreator::computeDigest(const char* input, return true; } -void CryptoppDigestCreator::updateDigest(const char* data, const size_t len) { +void CryptoppDigestCreator::update(const char* data, size_t len) { ConcordAssert(nullptr != internalState_); DigestType* p = (DigestType*)internalState_;