Raise InitError when peers.dat is invalid or corrupted

Author: MarcoFalke

src/addrdb.cpp

@@ -21,6 +21,12 @@
 #include <util/translation.h>
 
 namespace {
+
+class DbNotFoundError : public std::exception
+{
+    using std::exception::exception;
+};
+
 template <typename Stream, typename Data>
 bool SerializeDB(Stream& stream, const Data& data)
 {
@@ -78,47 +84,40 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
 }
 
 template <typename Stream, typename Data>
-bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
+void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
 {
-    try {
-        CHashVerifier<Stream> verifier(&stream);
-        // de-serialize file header (network specific magic number) and ..
-        unsigned char pchMsgTmp[4];
-        verifier >> pchMsgTmp;
-        // ... verify the network matches ours
-        if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp)))
-            return error("%s: Invalid network magic number", __func__);
-
-        // de-serialize data
-        verifier >> data;
-
-        // verify checksum
-        if (fCheckSum) {
-            uint256 hashTmp;
-            stream >> hashTmp;
-            if (hashTmp != verifier.GetHash()) {
-                return error("%s: Checksum mismatch, data corrupted", __func__);
-            }
+    CHashVerifier<Stream> verifier(&stream);
+    // de-serialize file header (network specific magic number) and ..
+    unsigned char pchMsgTmp[4];
+    verifier >> pchMsgTmp;
+    // ... verify the network matches ours
+    if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) {
+        throw std::runtime_error{"Invalid network magic number"};
+    }
+
+    // de-serialize data
+    verifier >> data;
+
+    // verify checksum
+    if (fCheckSum) {
+        uint256 hashTmp;
+        stream >> hashTmp;
+        if (hashTmp != verifier.GetHash()) {
+            throw std::runtime_error{"Checksum mismatch, data corrupted"};
         }
     }
-    catch (const std::exception& e) {
-        return error("%s: Deserialize or I/O error - %s", __func__, e.what());
-    }
-
-    return true;
 }
 
 template <typename Data>
-bool DeserializeFileDB(const fs::path& path, Data& data, int version)
+void DeserializeFileDB(const fs::path& path, Data& data, int version)
 {
     // open input file, and associate with CAutoFile
     FILE* file = fsbridge::fopen(path, "rb");
     CAutoFile filein(file, SER_DISK, version);
     if (filein.IsNull()) {
-        LogPrintf("Missing or invalid file %s\n", path.string());
-        return false;
+        throw DbNotFoundError{};
     }
-    return DeserializeDB(filein, data);
+    DeserializeDB(filein, data);
 }
 } // namespace
 
@@ -177,9 +176,9 @@ bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr)
     return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
 }
 
-bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers)
+void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers)
 {
-    return DeserializeDB(ssPeers, addr, false);
+    DeserializeDB(ssPeers, addr, false);
 }
 
 std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<CAddrMan>& addrman)
@@ -189,13 +188,18 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
 
     int64_t nStart = GetTimeMillis();
     const auto path_addr{args.GetDataDirNet() / "peers.dat"};
-    if (DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION)) {
+    try {
+        DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
         LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman->size(), GetTimeMillis() - nStart);
-    } else {
+    } catch (const DbNotFoundError&) {
         // Addrman can be in an inconsistent state after failure, reset it
         addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
-        LogPrintf("Recreating peers.dat\n");
+        LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr);
         DumpPeerAddresses(args, *addrman);
+    } catch (const std::exception& e) {
+        addrman = nullptr;
+        return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),
+                         e.what(), PACKAGE_BUGREPORT, path_addr);
     }
     return std::nullopt;
 }
@@ -209,9 +213,10 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& a
 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 {
     std::vector<CAddress> anchors;
-    if (DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT)) {
+    try {
+        DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
         LogPrintf("Loaded %i addresses from %s\n", anchors.size(), anchors_db_path.filename());
-    } else {
+    } catch (const std::exception&) {
         anchors.clear();
     }
 

src/addrdb.h

@@ -21,7 +21,7 @@ struct bilingual_str;
 
 bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr);
 /** Only used by tests. */
-bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers);
+void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers);
 
 /** Access to the banlist database (banlist.json) */
 class CBanDB

src/test/addrman_tests.cpp

@@ -1043,7 +1043,7 @@ BOOST_AUTO_TEST_CASE(load_addrman)
 
     CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
-    BOOST_CHECK(ReadFromStream(addrman2, ssPeers2));
+    ReadFromStream(addrman2, ssPeers2);
     BOOST_CHECK(addrman2.size() == 3);
 }
 
@@ -1073,7 +1073,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
 
     CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
     BOOST_CHECK(addrman2.size() == 0);
-    BOOST_CHECK(!ReadFromStream(addrman2, ssPeers2));
+    BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure);
 }
 
 

src/test/fuzz/data_stream.cpp

@@ -23,5 +23,8 @@ 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);
-    ReadFromStream(addr_man, data_stream);
+    try {
+        ReadFromStream(addr_man, data_stream);
+    } catch (const std::exception&) {
+    }
 }

test/functional/feature_addrman.py

@@ -10,6 +10,7 @@
 from test_framework.messages import ser_uint256, hash256
 from test_framework.p2p import MAGIC_BYTES
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.test_node import ErrorMatch
 from test_framework.util import assert_equal
 
 
@@ -43,6 +44,12 @@ def set_test_params(self):
 
     def run_test(self):
         peers_dat = os.path.join(self.nodes[0].datadir, self.chain, "peers.dat")
+        init_error = lambda reason: (
+            f"Error: Invalid or corrupt peers.dat \\({reason}\\). If you believe this "
+            f"is a bug, please report it to {self.config['environment']['PACKAGE_BUGREPORT']}. "
+            f'As a workaround, you can move the file \\("{peers_dat}"\\) out of the way \\(rename, '
+            "move, or delete\\) to have a new one created on the next start."
+        )
 
         self.log.info("Check that mocked addrman is valid")
         self.stop_node(0)
@@ -54,30 +61,29 @@ def run_test(self):
         self.log.info("Check that addrman from future cannot be read")
         self.stop_node(0)
         write_addrman(peers_dat, lowest_compatible=111)
-        with self.nodes[0].assert_debug_log([
-                f'ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of {self.config["environment"]["PACKAGE_NAME"]} is 3.',
-                "Recreating peers.dat",
-        ]):
-            self.start_node(0)
-        assert_equal(self.nodes[0].getnodeaddresses(), [])
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error(
+                "Unsupported format of addrman database: 1. It is compatible with "
+                "formats >=111, but the maximum supported by this version of "
+                f"{self.config['environment']['PACKAGE_NAME']} is 3.: (.+)"
+            ),
+            match=ErrorMatch.FULL_REGEX,
+        )
 
         self.log.info("Check that corrupt addrman cannot be read")
         self.stop_node(0)
         with open(peers_dat, "wb") as f:
             f.write(serialize_addrman()[:-1])
-        with self.nodes[0].assert_debug_log([
-                "ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file",
-                "Recreating peers.dat",
-        ]):
-            self.start_node(0)
-        assert_equal(self.nodes[0].getnodeaddresses(), [])
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg=init_error("CAutoFile::read: end of file.*"),
+            match=ErrorMatch.FULL_REGEX,
+        )
 
         self.log.info("Check that missing addrman is recreated")
         self.stop_node(0)
         os.remove(peers_dat)
         with self.nodes[0].assert_debug_log([
-                f"Missing or invalid file {peers_dat}",
-                "Recreating peers.dat",
+                f'Creating peers.dat because the file was not found ("{peers_dat}")',
         ]):
             self.start_node(0)
         assert_equal(self.nodes[0].getnodeaddresses(), [])