Skip to content

Commit

Permalink
Merge bitcoin#27935: fuzz: call lookup functions before calling Ban
Browse files Browse the repository at this point in the history
fca0a89 ci: remove "--exclude banman" for fuzzing in mac (brunoerg)
f9b2863 fuzz: call lookup functions before calling `Ban` (brunoerg)

Pull request description:

  Fixes bitcoin#27924

  To not have any discrepancy, it's required to call lookup functions before calling `Ban`. If we don't do it, the assertion `assert(banmap == banmap_read);` may fail because `BanMapFromJson` will call `LookupSubNet` and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (bitcoin#27924).

  Also, calling lookup functions before banning is what RPC `setban` does.

ACKs for top commit:
  maflcko:
    lgtm ACK fca0a89
  dergoegge:
    ACK fca0a89

Tree-SHA512: a3d635088a556df4507e65542157f10b41d4f87dce42927b58c3b812f262f4544b6b57f3384eef1097ffdd7c32b8dd1556aae201254960cbfbf48d45551200f7
  • Loading branch information
fanquake committed Nov 13, 2023
2 parents dd5f571 + fca0a89 commit e862bce
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 6 deletions.
1 change: 0 additions & 1 deletion ci/test/00_setup_env_mac_native.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,3 @@ export NO_DEPENDS=1
export OSX_SDK=""
export CCACHE_MAXSIZE=400M
export RUN_FUZZ_TESTS=true
export FUZZ_TESTS_CONFIG="--exclude banman" # https://github.com/bitcoin/bitcoin/issues/27924
23 changes: 18 additions & 5 deletions src/test/fuzz/banman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,28 @@ FUZZ_TARGET(banman, .init = initialize_banman)
// The complexity is O(N^2), where N is the input size, because each call
// might call DumpBanlist (or other methods that are at least linear
// complexity of the input size).
bool contains_invalid{false};
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
{
CallOneOf(
fuzzed_data_provider,
[&] {
ban_man.Ban(ConsumeNetAddr(fuzzed_data_provider),
ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
CNetAddr net_addr{ConsumeNetAddr(fuzzed_data_provider)};
const std::optional<CNetAddr>& addr{LookupHost(net_addr.ToStringAddr(), /*fAllowLookup=*/false)};
if (addr.has_value() && addr->IsValid()) {
net_addr = *addr;
} else {
contains_invalid = true;
}
ban_man.Ban(net_addr, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
},
[&] {
ban_man.Ban(ConsumeSubNet(fuzzed_data_provider),
ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
CSubNet subnet{ConsumeSubNet(fuzzed_data_provider)};
subnet = LookupSubNet(subnet.ToString());
if (!subnet.IsValid()) {
contains_invalid = true;
}
ban_man.Ban(subnet, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
},
[&] {
ban_man.ClearBanned();
Expand Down Expand Up @@ -109,7 +120,9 @@ FUZZ_TARGET(banman, .init = initialize_banman)
BanMan ban_man_read{banlist_file, /*client_interface=*/nullptr, /*default_ban_time=*/0};
banmap_t banmap_read;
ban_man_read.GetBanned(banmap_read);
assert(banmap == banmap_read);
if (!contains_invalid) {
assert(banmap == banmap_read);
}
}
}
fs::remove(fs::PathToString(banlist_file + ".json"));
Expand Down

0 comments on commit e862bce

Please sign in to comment.