merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize

src/Makefile.test.include

@@ -263,7 +263,6 @@ test_fuzz_fuzz_SOURCES = \
   test/fuzz/crypto_hkdf_hmac_sha256_l32.cpp \
   test/fuzz/crypto_poly1305.cpp \
   test/fuzz/cuckoocache.cpp \
-  test/fuzz/data_stream.cpp \
   test/fuzz/decode_tx.cpp \
   test/fuzz/descriptor_parse.cpp \
   test/fuzz/deserialize.cpp \

src/addrman.cpp

@@ -389,7 +389,12 @@ void CAddrMan::Unserialize(Stream& s_)
         LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions or invalid addresses\n", nLostUnk, nLost);
     }
 
-    Check();
+    const int check_code{ForceCheckAddrman()};
+    if (check_code != 0) {
+        throw std::ios_base::failure(strprintf(
+            "Corrupt data. Consistency check failed with code %s",
+            check_code));
+    }
 }
 
 // explicit instantiation
@@ -764,14 +769,26 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const
     }
 }
 
-int CAddrMan::Check_() const
+void CAddrMan::Check() const
 {
     AssertLockHeld(cs);
-    LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size());
 
     // Run consistency checks 1 in m_consistency_check_ratio times if enabled
-    if (m_consistency_check_ratio == 0) return 0;
-    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0;
+    if (m_consistency_check_ratio == 0) return;
+    if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return;
+
+    const int err{ForceCheckAddrman()};
+    if (err) {
+        LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
+        assert(false);
+    }
+}
+
+int CAddrMan::ForceCheckAddrman() const
+{
+    AssertLockHeld(cs);
+
+    LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size());
 
     std::unordered_set<int> setTried;
     std::unordered_map<int, int> mapNew;

src/addrman.h

@@ -406,20 +406,12 @@ private:
     //! Return a random to-be-evicted tried table address.
     CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    //! Consistency check
-    void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs)
-    {
-        AssertLockHeld(cs);
+    //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected.
+    void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-        const int err = Check_();
-        if (err) {
-            LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
-            assert(false);
-        }
-    }
-
-    //! Perform consistency check. Returns an error code or zero.
-    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    //! Perform consistency check, regardless of m_consistency_check_ratio.
+    //! @returns an error code or zero.
+    int ForceCheckAddrman() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /**
      * Return all or many randomly selected addresses, optionally by network.

src/test/fuzz/addrman.cpp

@@ -23,6 +23,17 @@ void initialize_addrman()
     SelectParams(CBaseChainParams::REGTEST);
 }
 
+FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider);
+    CAddrMan addr_man(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    try {
+        ReadFromStream(addr_man, data_stream);
+    } catch (const std::exception&) {
+    }
+}
+
 class CAddrManDeterministic : public CAddrMan
 {
 public:

src/test/fuzz/data_stream.cpp

@@ -1,30 +0,0 @@
-// Copyright (c) 2020 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#include <addrdb.h>
-#include <addrman.h>
-#include <net.h>
-#include <test/fuzz/FuzzedDataProvider.h>
-#include <test/fuzz/fuzz.h>
-#include <test/fuzz/util.h>
-#include <test/util/setup_common.h>
-
-#include <cstdint>
-#include <vector>
-
-void initialize_data_stream_addr_man()
-{
-    static const auto testing_setup = MakeNoLogFileContext<>();
-}
-
-FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man)
-{
-    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
-    CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider);
-    CAddrMan addr_man(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
-    try {
-        ReadFromStream(addr_man, data_stream);
-    } catch (const std::exception&) {
-    }
-}

test/functional/feature_addrman.py

@@ -19,6 +19,7 @@ def serialize_addrman(
     format=1,
     lowest_compatible=3,
     net_magic="regtest",
+    bucket_key=1,
     len_new=None,
     len_tried=None,
     mock_checksum=None,
@@ -29,7 +30,7 @@ def serialize_addrman(
     r = MAGIC_BYTES[net_magic]
     r += struct.pack("B", format)
     r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible)
-    r += ser_uint256(1)
+    r += ser_uint256(bucket_key)
     r += struct.pack("i", len_new or len(new))
     r += struct.pack("i", len_tried or len(tried))
     ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
@@ -119,6 +120,14 @@ class AddrmanTest(BitcoinTestFramework):
             match=ErrorMatch.FULL_REGEX,
         )
 
+        self.log.info("Check that corrupt addrman cannot be read (failed check)")
+        self.stop_node(0)
+        write_addrman(peers_dat, bucket_key=0)
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error("Corrupt data. Consistency check failed with code -16: .*"),
+            match=ErrorMatch.FULL_REGEX,
+        )
+
         self.log.info("Check that missing addrman is recreated")
         self.stop_node(0)
         os.remove(peers_dat)