diff options
author | merge-script <falke.marco@gmail.com> | 2021-09-21 18:21:00 +0200 |
---|---|---|
committer | merge-script <falke.marco@gmail.com> | 2021-09-21 18:21:00 +0200 |
commit | a8a272ac329da679bd489e272ed1773147f59eb9 (patch) | |
tree | 21ea162b00629318af31f930351e9a40907d91d4 | |
parent | ae674a0198b06169eb3b27f86cf6755fecde4107 (diff) | |
parent | fa3669f72f69662049b55ad1a482b4a0f9f7ae40 (diff) | |
download | bitcoin-a8a272ac329da679bd489e272ed1773147f59eb9.tar.xz |
Merge bitcoin/bitcoin#22734: addrman: Avoid crash on corrupt data, Force Check after deserialize
fa3669f72f69662049b55ad1a482b4a0f9f7ae40 fuzz: Move all addrman fuzz targets to one file (MarcoFalke)
fa7a883f5a219d5f3c2f992b090db4e6c279db12 addrman: Replace assert with throw on corrupt data (MarcoFalke)
fa298971e6890715e2b0b93f2a7f445d32d6622f Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke)
fae5c633dc05a045aaac370b383e4799cb0e0590 move-only: Move CAddrMan::Check to cpp file (MarcoFalke)
Pull request description:
Assert should only be used for program internal logic errors, not to sanitize external user input.
The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70fd9f65ef800567ff9456fac525bc3e3c2, thus won't need a backport.
Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.
For example, if `nLastSuccess` is negative, it would later result in integer overflows. Thus, this patch fixes #22931.
Also,
Fixes #22503
Fixes #22504
Fixes #22519
Closes #22498
Steps to test:
```
mkdir -p /tmp/test_235/regtest/
echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'
```
Output before:
```
2021-09-10T11:28:37Z init message: Loading P2P addresses…
2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.
(program crashes)
```
Output after:
```
2021-09-10T11:26:00Z init message: Loading P2P addresses…
2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
(program exits)
```
ACKs for top commit:
naumenkogs:
ACK fa3669f72f69662049b55ad1a482b4a0f9f7ae40
jnewbery:
Code review ACK fa3669f72f69662049b55ad1a482b4a0f9f7ae40
vasild:
ACK fa3669f72f69662049b55ad1a482b4a0f9f7ae40
Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6
-rw-r--r-- | src/Makefile.test.include | 1 | ||||
-rw-r--r-- | src/addrman.cpp | 24 | ||||
-rw-r--r-- | src/addrman.h | 18 | ||||
-rw-r--r-- | src/test/fuzz/addrman.cpp | 11 | ||||
-rw-r--r-- | src/test/fuzz/data_stream.cpp | 30 | ||||
-rwxr-xr-x | test/functional/feature_addrman.py | 11 |
6 files changed, 46 insertions, 49 deletions
diff --git a/src/Makefile.test.include b/src/Makefile.test.include index a85a359960..be63214c23 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -232,7 +232,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 \ diff --git a/src/addrman.cpp b/src/addrman.cpp index a1e8cb1bf1..7c6b8fe64d 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -386,7 +386,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 @@ -743,13 +748,24 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const } } -int CAddrMan::Check_() const +void CAddrMan::Check() const { AssertLockHeld(cs); // 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()); diff --git a/src/addrman.h b/src/addrman.h index 0885231ebc..7dd8528bef 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -391,20 +391,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); - - const int err = Check_(); - if (err) { - LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); - assert(false); - } - } + //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected. + void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs); - //! 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. diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 95c5a99c1b..45ee778b87 100644 --- a/src/test/fuzz/addrman.cpp +++ b/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: diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp deleted file mode 100644 index 323090e041..0000000000 --- a/src/test/fuzz/data_stream.cpp +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) 2020-2021 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&) { - } -} diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 42afd74ac9..55d3e48c64 100755 --- a/test/functional/feature_addrman.py +++ b/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) |