net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix

theStack · theStack · commit 1ad8ea2b7313 · 2020-04-28T19:27:22.000+02:00
diff --git a/src/bloom.cpp b/src/bloom.cpp
@@ -31,8 +31,6 @@ CBloomFilter::CBloomFilter(const unsigned int nElements, const double nFPRate, c
      * Again, we ignore filter parameters which will create a bloom filter with more hash functions than the protocol limits
      * See https://en.wikipedia.org/wiki/Bloom_filter for an explanation of these formulas
      */
-    isFull(false),
-    isEmpty(true),
     nHashFuncs(std::min((unsigned int)(vData.size() * 8 / nElements * LN2), MAX_HASH_FUNCS)),
     nTweak(nTweakIn),
     nFlags(nFlagsIn)
@@ -47,15 +45,14 @@ inline unsigned int CBloomFilter::Hash(unsigned int nHashNum, const std::vector<
 
 void CBloomFilter::insert(const std::vector<unsigned char>& vKey)
 {
-    if (isFull)
+    if (vData.empty()) // Avoid divide-by-zero (CVE-2013-5700)
         return;
     for (unsigned int i = 0; i < nHashFuncs; i++)
     {
         unsigned int nIndex = Hash(i, vKey);
         // Sets bit nIndex of vData
         vData[nIndex >> 3] |= (1 << (7 & nIndex));
     }
-    isEmpty = false;
 }
 
 void CBloomFilter::insert(const COutPoint& outpoint)
@@ -74,10 +71,8 @@ void CBloomFilter::insert(const uint256& hash)
 
 bool CBloomFilter::contains(const std::vector<unsigned char>& vKey) const
 {
-    if (isFull)
+    if (vData.empty()) // Avoid divide-by-zero (CVE-2013-5700)
         return true;
-    if (isEmpty)
-        return false;
     for (unsigned int i = 0; i < nHashFuncs; i++)
     {
         unsigned int nIndex = Hash(i, vKey);
@@ -112,10 +107,8 @@ bool CBloomFilter::IsRelevantAndUpdate(const CTransaction& tx)
     bool fFound = false;
     // Match if the filter contains the hash of tx
     //  for finding tx when they appear in a block
-    if (isFull)
+    if (vData.empty()) // zero-size = "match-all" filter
         return true;
-    if (isEmpty)
-        return false;
     const uint256& hash = tx.GetHash();
     if (contains(hash))
         fFound = true;
@@ -177,19 +170,6 @@ bool CBloomFilter::IsRelevantAndUpdate(const CTransaction& tx)
     return false;
 }
 
-void CBloomFilter::UpdateEmptyFull()
-{
-    bool full = true;
-    bool empty = true;
-    for (unsigned int i = 0; i < vData.size(); i++)
-    {
-        full &= vData[i] == 0xff;
-        empty &= vData[i] == 0;
-    }
-    isFull = full;
-    isEmpty = empty;
-}
-
 CRollingBloomFilter::CRollingBloomFilter(const unsigned int nElements, const double fpRate)
 {
     double logFpRate = log(fpRate);
diff --git a/src/bloom.h b/src/bloom.h
@@ -45,8 +45,6 @@ class CBloomFilter
 {
 private:
     std::vector<unsigned char> vData;
-    bool isFull;
-    bool isEmpty;
     unsigned int nHashFuncs;
     unsigned int nTweak;
     unsigned char nFlags;
@@ -64,7 +62,7 @@ class CBloomFilter
      * nFlags should be one of the BLOOM_UPDATE_* enums (not _MASK)
      */
     CBloomFilter(const unsigned int nElements, const double nFPRate, const unsigned int nTweak, unsigned char nFlagsIn);
-    CBloomFilter() : isFull(true), isEmpty(false), nHashFuncs(0), nTweak(0), nFlags(0) {}
+    CBloomFilter() : nHashFuncs(0), nTweak(0), nFlags(0) {}
 
     ADD_SERIALIZE_METHODS;
 
@@ -90,9 +88,6 @@ class CBloomFilter
 
     //! Also adds any outputs which match the filter to the filter (to match their spending txes)
     bool IsRelevantAndUpdate(const CTransaction& tx);
-
-    //! Checks for empty and full filters to avoid wasting cpu
-    void UpdateEmptyFull();
 };
 
 /**
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -3193,7 +3193,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
         {
             LOCK(pfrom->m_tx_relay->cs_filter);
             pfrom->m_tx_relay->pfilter.reset(new CBloomFilter(filter));
-            pfrom->m_tx_relay->pfilter->UpdateEmptyFull();
             pfrom->m_tx_relay->fRelayTxes = true;
         }
         return true;
diff --git a/src/test/fuzz/bloom_filter.cpp b/src/test/fuzz/bloom_filter.cpp
@@ -25,7 +25,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         fuzzed_data_provider.ConsumeIntegral<unsigned int>(),
         static_cast<unsigned char>(fuzzed_data_provider.PickValueInArray({BLOOM_UPDATE_NONE, BLOOM_UPDATE_ALL, BLOOM_UPDATE_P2PUBKEY_ONLY, BLOOM_UPDATE_MASK}))};
     while (fuzzed_data_provider.remaining_bytes() > 0) {
-        switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 4)) {
+        switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 3)) {
         case 0: {
             const std::vector<unsigned char> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);
             (void)bloom_filter.contains(b);
@@ -65,9 +65,6 @@ void test_one_input(const std::vector<uint8_t>& buffer)
             (void)bloom_filter.IsRelevantAndUpdate(tx);
             break;
         }
-        case 4:
-            bloom_filter.UpdateEmptyFull();
-            break;
         }
         (void)bloom_filter.IsWithinSizeConstraints();
     }

Original file line number	Diff line number	Diff line change
`@@ -3193,7 +3193,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec`
`3193`	`3193`	`{`
`3194`	`3194`	`LOCK(pfrom->m_tx_relay->cs_filter);`
`3195`	`3195`	`pfrom->m_tx_relay->pfilter.reset(new CBloomFilter(filter));`
`3196`		`- pfrom->m_tx_relay->pfilter->UpdateEmptyFull();`
`3197`	`3196`	`pfrom->m_tx_relay->fRelayTxes = true;`
`3198`	`3197`	`}`
`3199`	`3198`	`return true;`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)`
`25`	`25`	`fuzzed_data_provider.ConsumeIntegral<unsigned int>(),`
`26`	`26`	`static_cast<unsigned char>(fuzzed_data_provider.PickValueInArray({BLOOM_UPDATE_NONE, BLOOM_UPDATE_ALL, BLOOM_UPDATE_P2PUBKEY_ONLY, BLOOM_UPDATE_MASK}))};`
`27`	`27`	`while (fuzzed_data_provider.remaining_bytes() > 0) {`
`28`		`- switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 4)) {`
	`28`	`+ switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 3)) {`
`29`	`29`	`case 0: {`
`30`	`30`	`const std::vector<unsigned char> b = ConsumeRandomLengthByteVector(fuzzed_data_provider);`
`31`	`31`	`(void)bloom_filter.contains(b);`
`@@ -65,9 +65,6 @@ void test_one_input(const std::vector<uint8_t>& buffer)`
`65`	`65`	`(void)bloom_filter.IsRelevantAndUpdate(tx);`
`66`	`66`	`break;`
`67`	`67`	`}`
`68`		`- case 4:`
`69`		`- bloom_filter.UpdateEmptyFull();`
`70`		`- break;`
`71`	`68`	`}`
`72`	`69`	`(void)bloom_filter.IsWithinSizeConstraints();`
`73`	`70`	`}`