Fixes #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 (#27924).
Also, calling lookup functions before banning is what RPC setban does.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Could rebase on master to remove export FUZZ_TESTS_CONFIG="--exclude banman" # [#27924](/bitcoin-bitcoin/27924/)?
Force-pushed to remove "--exclude banman" in Mac
Rebased.
This still crashes for me on the following input /r8BAAAC/9LcAF8gMiWADQUA//////////////8AN1wNXMnJyQ3///8Afv///wABAAAAgAAAAADPvg==
I'm investigating it.
Thanks for reviewing, @dergoegge. I noted that we also need to call LookupHost when banning with CNetAddr. Force-pushed doing it and updated the PR description and title.
Also, this way we're closer to the reality (following what RPC does to set it).
67 | @@ -68,12 +68,19 @@ FUZZ_TARGET(banman, .init = initialize_banman) 68 | CallOneOf( 69 | fuzzed_data_provider, 70 | [&] { 71 | - ban_man.Ban(ConsumeNetAddr(fuzzed_data_provider), 72 | + const std::string name{fuzzed_data_provider.ConsumeRandomLengthString(512)}; 73 | + const std::optional<CNetAddr> addr{LookupHost(name, /*fAllowLookup=*/false)}; 74 | + if (!addr.has_value() || !addr->IsValid()) return; 75 | + ban_man.Ban(addr.value(),
It looks like you can drop the IsValid() checks in your updated fuzz cases?
nit, I think this would be clearer and use the same structure for both of your updated fuzz cases (e.g. use return for neither, or for both):
- if (!addr.has_value() || !addr->IsValid()) return;
- ban_man.Ban(addr.value(),
- ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+ if (addr.has_value() && addr->IsValid()) {
+ ban_man.Ban(addr.value(), ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+ }
Yes, it makes sense. Updated it.
ACK f4bd8f204edfc0ca6f0477d78c95c548c3cb50b6
Happy to re-ack if you take the suggestion.
67 | @@ -68,12 +68,18 @@ FUZZ_TARGET(banman, .init = initialize_banman) 68 | CallOneOf( 69 | fuzzed_data_provider, 70 | [&] { 71 | - ban_man.Ban(ConsumeNetAddr(fuzzed_data_provider), 72 | - ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool()); 73 | + const std::string name{fuzzed_data_provider.ConsumeRandomLengthString(512)}; 74 | + const std::optional<CNetAddr> addr{LookupHost(name, /*fAllowLookup=*/false)}; 75 | + if (addr.has_value()) {
Thanks! However, the updates are in the wrong commit.
Could optionally define addr in the conditional (as reference to const, I think); feel free to ignore.
fuzzed_data_provider,
[&] {
const std::string name{fuzzed_data_provider.ConsumeRandomLengthString(512)};
- const std::optional<CNetAddr> addr{LookupHost(name, /*fAllowLookup=*/false)};
- if (addr.has_value()) {
+ if (const std::optional<CNetAddr>& addr = LookupHost(name, /*fAllowLookup=*/false)) {
ban_man.Ban(addr.value(), ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
}
},
Oh my bad for putting the updates in the wrong commit, just fixed it and addressed your suggestion.
ACK f107b60865412fb3d3e4ce5dd3c6d89dd29ec644
I'm not sure if this is the right approach to fixing this. If we always expect assert(banmap == banmap_read); to hold then BanMan::Ban should probably internalize the lookup call. Otherwise we just end up calling it before every Ban call.
Code review ACK f107b60865412fb3d3e4ce5dd3c6d89dd29ec644
I think this would also fix #27071 (comment).
What I suggested above would require more changes, so maybe we just go with what is here now...
76 | + } 77 | }, 78 | [&] { 79 | - ban_man.Ban(ConsumeSubNet(fuzzed_data_provider), 80 | - ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool()); 81 | + const std::string name{fuzzed_data_provider.ConsumeRandomLengthString(512)};
nit: Does the string representation make it harder for the fuzz engine to reach coverage?
Previously the engine could just copy some bytes and then trivially trigger the Ban and IsBanned code path. Now it would need to figure out two different representations of the same thing?
Does the string representation make it harder for the fuzz engine to reach coverage?
Hm, yes, perhaps an alternative to facilitate it (didnt test yet, just wondering):
+ CSubNet subnet;
+ CNetAddr net_addr;
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
{
CallOneOf(
fuzzed_data_provider,
[&] {
- const std::string name{fuzzed_data_provider.ConsumeRandomLengthString(512)};
- if (const std::optional<CNetAddr>& addr{LookupHost(name, /*fAllowLookup=*/false)}) {
- ban_man.Ban(addr.value(), ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+ CSubNet new_subnet{ConsumeSubNet(fuzzed_data_provider)};
+ if (LookupSubNet(new_subnet.ToString(), new_subnet)) {
+ subnet = new_subnet;
}
},
[&] {
- const std::string name{fuzzed_data_provider.ConsumeRandomLengthString(512)};
- CSubNet subnet;
- if (LookupSubNet(name, subnet)) {
- ban_man.Ban(subnet, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+ CNetAddr new_netaddr{ConsumeNetAddr(fuzzed_data_provider)};
+ if (const std::optional<CNetAddr>& addr{LookupHost(new_netaddr.ToStringAddr(), /*fAllowLookup=*/false)}) {
+ net_addr = addr.value();
}
},
+ [&] {
+ ban_man.Ban(net_addr, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+ },
+ [&] {
+ ban_man.Ban(subnet, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+ },
[&] {
ban_man.ClearBanned();
},
[&] {
- ban_man.IsBanned(ConsumeNetAddr(fuzzed_data_provider));
+ ban_man.IsBanned(net_addr);
},
[&] {
- ban_man.IsBanned(ConsumeSubNet(fuzzed_data_provider));
+ ban_man.IsBanned(subnet);
},
[&] {
- ban_man.Unban(ConsumeNetAddr(fuzzed_data_provider));
+ ban_man.Unban(net_addr);
},
[&] {
- ban_man.Unban(ConsumeSubNet(fuzzed_data_provider));
+ ban_man.Unban(subnet);
},
[&] {
banmap_t banmap;
@@ -103,7 +110,7 @@ FUZZ_TARGET(banman, .init = initialize_banman)
ban_man.DumpBanlist();
},
[&] {
- ban_man.Discourage(ConsumeNetAddr(fuzzed_data_provider));
+ ban_man.Discourage(net_addr);
});
}
If you're gonna change this then my suggestion from #27935 (comment) is probably simpler and doesn't touch the fuzz test at all.
Would it be possible to still call ConsumeSubNet() but check if the returned CSubNet object IsValid() before feeding it to Ban()?
ConsumeSubNet() could generate nonsensical stuff like 2bqghnldu6mcug4pikzprwhtjjnsyederctvci6klcwzepnjd46ikjyd.onion/16 which is later an invalid CSubNet whose constructor only assumes IPv4 or IPv6 subnets valid. This is the reason also for #27071 (comment) (it generates fc00::%538976288/6 IPv6 which later gets converted to CJDNS and rightfully turns out to be an invalid subnet because it is not IPv4 or IPv6).
Or change ConsumeSubNet() to only produce valid stuff?
If you're gonna change this then my suggestion from #27935 (comment) is probably simpler and doesn't touch the fuzz test at all.
like this, @dergoegge?
diff --git a/src/banman.cpp b/src/banman.cpp
index a96b7e3c53..e42dc6f9c6 100644
--- a/src/banman.cpp
+++ b/src/banman.cpp
@@ -8,6 +8,7 @@
#include <common/system.h>
#include <logging.h>
#include <netaddress.h>
+#include <netbase.h>
#include <node/interface_ui.h>
#include <sync.h>
#include <util/time.h>
@@ -116,7 +117,13 @@ bool BanMan::IsBanned(const CSubNet& sub_net)
void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_unix_epoch)
{
- CSubNet sub_net(net_addr);
+ CNetAddr netaddr;
+ if (const std::optional<CNetAddr>& addr{LookupHost(net_addr.ToStringAddr(), /*fAllowLookup=*/false)}) {
+ netaddr = addr.value();
+ } else {
+ return;
+ }
+ CSubNet sub_net(netaddr);
Ban(sub_net, ban_time_offset, since_unix_epoch);
}
@@ -128,6 +135,9 @@ void BanMan::Discourage(const CNetAddr& net_addr)
void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_unix_epoch)
{
+ CSubNet subnet;
+ LookupSubNet(sub_net.ToString(), subnet);
+
CBanEntry ban_entry(GetTime());
int64_t normalized_ban_time_offset = ban_time_offset;
@@ -140,8 +150,8 @@ void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_uni
{
LOCK(m_cs_banned);
- if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) {
- m_banned[sub_net] = ban_entry;
+ if (m_banned[subnet].nBanUntil < ban_entry.nBanUntil) {
+ m_banned[subnet] = ban_entry;
m_is_dirty = true;
} else
return;
I think it is fine to only fix the fuzz test, if the issue is limited to the fuzz test? #27935 (review) lgtm, but I haven't reviewed it.
In any case, I don't think this is a blocker for rc1. It is not a regression and this diff can be added to rc2 trivially.
#27935 (review) still could call Ban() with an invalid address or subnet (default constructed is !IsValid()). I am not sure if we should be doing that from the tests:
BanMan does not crashBanMan removes invalid entries when re-reading the bans from the json file, so it is unrealistic to expect that the ban entries will be the same before and after read from the json fileBanMan that it accepts and invalid entry into its map via Ban(), but rejects invalid entries from the json file. Maybe BanMan should reject invalid entries also from the Ban() method.
I think the problem is the logic is all in the RPC (lookup calls and the vailidity check). We could move it to a function.
This is still an issue in the latest version (8adbd44c9b). It seems unlikely that the fuzzer will generate a string that makes LookupHost() happy. I think it is best if this test keeps using ConsumeNetAddr() and ConsumeSubNet() but those functions should return valid/sensible values. Something like:
inline CSubNet ConsumeSubNet(FuzzedDataProvider& fuzzed_data_provider) noexcept
{
const CNetAddr net_base = ConsumeNetAddr(fuzzed_data_provider, {NET_IPV4, NET_IPV6}, /*always_valid=*/true);
const uint8_t cidr_max = net_base.IsIPv4() ? ADDR_IPV4_SIZE : ADDR_IPV6_SIZE;
return {net_base, fuzzed_data_provider.ConsumeIntegralInRange<uint8_t>(0, cidr_max * 8)};
}
but those functions should return valid/sensible values.
I think it is good to offer a path for the fuzz engine to produce an invalid value. Otherwise, it can miss bugs that only happen when an invalid value is passed.
Yes, this was my doubt too. But then it is unrealistic to expect that banman ser/deser will produce identical result as the input. Another try - what about feeding it anything (including invalid stuff), but keeping track if we ever fed it an invalid address or subnet and if yes, then don't do the assert at the end assert(banmap == banmap_read);?
Another try - what about feeding it anything (including invalid stuff), but keeping track if we ever fed it an invalid address or subnet and if yes, then don't do the assert at the end
assert(banmap == banmap_read);?
sgtm
Yes, this was my doubt too. But then it is unrealistic to expect that banman ser/deser will produce identical result as the input. Another try - what about feeding it anything (including invalid stuff), but keeping track if we ever fed it an invalid address or subnet and if yes, then don't do the assert at the end assert(banmap == banmap_read);?
sgtm as well. Can I change the approach so?
No opinion on the changes, as I am not familiar with low level net stuff.
I left a question about the approach.
With this PR rebased on master the fuzz target fails on ZmNmYTo6MVwzMyo6XG8x//+SfgQAsGAAAFtbAFs6AAAAgG6yQeqTIA== (took 1000+ CPU hours). Might be unrelated or the same issue, haven't checked.
Does it fail on the same assertion?
Yes
I guess there are two possible approaches:
Shorter and less invasive/risky - change the fuzz test to not ban invalid subnets. Calling LookupSubNet() should not be necessary and I guess IsValid() should suffice? Is there a case where a subnet has IsValid() == true but LookupSubNet() fails on it? Also, IPv6 subnets that start with fc while NET_CJDNS is reachable are "invalid".
Change BanMan::Ban() to not add such invalid networks to its internal map and change it to return bool, false meaning "I did not ban that" and true meaning "A ban was added".
Maybe do 1. for 26.0 and 2. for 27.0?
Looks like this can't be merged/backported as-is either way, due to the silent conflict caused by the rafactor (7be62df80ff4d1339c039c6589232e50274b4b1a)?
Looks like this can't be merged/backported as-is either way, due to the silent conflict caused by the rafactor (https://github.com/bitcoin/bitcoin/commit/7be62df80ff4d1339c039c6589232e50274b4b1a)?
Gonna check it
One more consideration wrt to changing Ban() to refuse invalid stuff (2. in above #27935 (comment)) - it will not match later anyway due to:
Rebased and changed the approach in fuzz to call Ban only with valid net address/subnet.
Are you still working on this?
Are you still working on this?
Yes, will push an update soon
Force-pushed:
Ban with an invalid subnet/netaddr. If so, we don't compare the banmaps.Ban with an invalid subnet/netaddr.
I did some experiments and I decided to keep the lookup functions. Without it, I still get discrepances even avoiding the comparison when we call Ban with an invalid subnet/netaddr.
Would be good to explain this a bit more, to make sure this is not a bug.
Would be good to explain this a bit more, to make sure this is not a bug.
I think fuzz can generate subnets with a zone identifier, like: "676:c962:7962:b787:b392:fed8:7058:c500%2038004089/121", which is a valid one. However, the lookup call might change the zone identifier. In my machine (macOS), "676:c962:7962:b787:b392:fed8:7058:c500%2038004089/121" becomes "676:c962:7962:b787:b392:fed8:7058:c500%31097/121" after lookup and it makes the assertion fails.
Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.
Also, compare banmaps only if there are no invalid
entries.
Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.
Yes, force-pushed addressing it. Also, I think this way our seeds remain great.
lgtm ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
I did not check the performance difference against 7e644308805229ab64455c01a22531756644fe69
ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
Milestone
26.0