From fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 21 Aug 2021 16:17:52 +0200 Subject: [PATCH] Raise InitError when peers.dat is invalid or corrupted --- src/addrdb.cpp | 77 ++++++++++++++++-------------- src/addrdb.h | 2 +- src/test/addrman_tests.cpp | 4 +- src/test/fuzz/data_stream.cpp | 5 +- test/functional/feature_addrman.py | 34 +++++++------ 5 files changed, 68 insertions(+), 54 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 77f0897a7f1..1e73750ce30 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -21,6 +21,12 @@ #include namespace { + +class DbNotFoundError : public std::exception +{ + using std::exception::exception; +}; + template bool SerializeDB(Stream& stream, const Data& data) { @@ -78,47 +84,40 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data } template -bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true) +void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true) { - try { - CHashVerifier 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 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 -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 LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman) @@ -189,13 +188,18 @@ std::optional LoadAddrman(const std::vector& 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(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& a std::vector ReadAnchors(const fs::path& anchors_db_path) { std::vector 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(); } diff --git a/src/addrdb.h b/src/addrdb.h index 55fcd7508fa..33cc1f92040 100644 --- a/src/addrdb.h +++ b/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 diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 835b18d42ec..01a492a20b8 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -1043,7 +1043,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) CAddrMan addrman2(/* asmap */ std::vector(), /* 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(), /* 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); } diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index 08e9c91ba31..323090e0410 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/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(), /* deterministic */ false, /* consistency_check_ratio */ 0); - ReadFromStream(addr_man, data_stream); + try { + ReadFromStream(addr_man, data_stream); + } catch (const std::exception&) { + } } diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 8ccff340f0e..ee421c89b5a 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -10,6 +10,7 @@ import struct 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 @@ class AddrmanTest(BitcoinTestFramework): 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 @@ class AddrmanTest(BitcoinTestFramework): 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(), [])