fuzz: Check banman roundtrip

MarcoFalke commented at 10:50 am on June 23, 2021: member

MarcoFalke requested review from vasild on Jun 23, 2021

MarcoFalke requested review from practicalswift on Jun 23, 2021

fanquake added the label Tests on Jun 23, 2021

jonatack commented at 11:00 am on June 23, 2021: member

Concept ACK, good idea.

in src/test/fuzz/banman.cpp:113 in fabadb7702 outdated

108+            auto it2{banmap_read.begin()};
109+            for (; it1 != banmap.end(); ++it1, ++it2) {
110+                assert(it1->first == it2->first);
111+                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
112+            }
113+        }

jonatack commented at 1:21 pm on June 23, 2021:

proposed docs if you like

 0-        BanMan ban_man{banlist_file, nullptr, offset};
 1+        BanMan ban_man{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ offset};
 2         while (--limit_max_ops >= 0 && fuzzed_data_provider.ConsumeBool()) {
 3             CallOneOf(
 4                 fuzzed_data_provider,
 5@@ -100,15 +100,15 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
 6             SetMockTime(ConsumeTime(fuzzed_data_provider));
 7             banmap_t banmap;
 8             ban_man.GetBanned(banmap);
 9-            BanMan ban_man_read{banlist_file, nullptr, offset};
10+            BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ offset};
11             banmap_t banmap_read;
12             ban_man_read.GetBanned(banmap_read);
13             assert(banmap.size() == banmap_read.size());
14             auto it1{banmap.begin()};
15             auto it2{banmap_read.begin()};
16             for (; it1 != banmap.end(); ++it1, ++it2) {
17-                assert(it1->first == it2->first);
18-                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
19+                assert(it1->first == it2->first); // CSubNet
20+                assert(it1->second.ToJson().write() == it2->second.ToJson().write()); // CBanEntry
21             }
22         }
23     }

MarcoFalke commented at 10:04 am on June 24, 2021:

Thanks, added comments

jonatack commented at 1:22 pm on June 23, 2021: member

tACK fabadb7702a9cbe477e9e5f91706620456154903

practicalswift approved

practicalswift commented at 2:41 pm on June 23, 2021: contributor

cr ACK fabadb7702a9cbe477e9e5f91706620456154903

Great work!

in src/test/fuzz/banman.cpp:59 in fabadb7702 outdated

56         }
57     }
58 
59     {
60-        BanMan ban_man{banlist_file, nullptr, ConsumeBanTimeOffset(fuzzed_data_provider)};
61+        const auto offset{ConsumeBanTimeOffset(fuzzed_data_provider)};

vasild commented at 3:41 pm on June 23, 2021:

Unrelated to this PR, why is this called “offset”? It is actually “ban_time” - number of seconds to keep the ban entry after the misbehavior occurred.

~~There is some “offset” involved in BanMan::Ban(), but that is completely internal to BanMan.~~

MarcoFalke commented at 10:04 am on June 24, 2021:

Removed offset

in src/test/fuzz/banman.cpp:111 in fabadb7702 outdated

100+            SetMockTime(ConsumeTime(fuzzed_data_provider));
101+            banmap_t banmap;
102+            ban_man.GetBanned(banmap);
103+            BanMan ban_man_read{banlist_file, nullptr, offset};
104+            banmap_t banmap_read;
105+            ban_man_read.GetBanned(banmap_read);

vasild commented at 3:49 pm on June 23, 2021:

The dumped data may end up containing more entries than banmap and banmap_read because SweepBanned() may leave them when called from DumpBanlist() but when called from GetBanned() after SetMockTime() it may remove “expired” entries.

It is ok since both banmap and banmap_read will contain the same, but if somebody ends up debugging this and looking at the file, then it may be confusing. Maybe swap lines 99 and 100.

MarcoFalke commented at 4:42 pm on June 23, 2021:

This was done on purpose to check SweepBanned is called on read

vasild commented at 7:53 am on June 24, 2021:

Hmm, then maybe add one entry that will certainly expire? This will also make the intention more explicit:

 0diff --git i/src/test/fuzz/banman.cpp w/src/test/fuzz/banman.cpp
 1index 6aa95b484f..b3b10da084 100644
 2--- i/src/test/fuzz/banman.cpp
 3+++ w/src/test/fuzz/banman.cpp
 4@@ -93,16 +93,23 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
 5                 },
 6                 [&] {
 7                     ban_man.Discourage(ConsumeNetAddr(fuzzed_data_provider));
 8                 });
 9         }
10         if (!force_read_and_write_to_err) {
11+            const auto mock_time = ConsumeTime(fuzzed_data_provider);
12+            SetMockTime(mock_time);
13+            // Add one ban entry that will expire to check that on read expired entries are removed.
14+            ban_man.Ban(ConsumeNetAddr(fuzzed_data_provider), mock_time + 5, true);
15             ban_man.DumpBanlist();
16-            SetMockTime(ConsumeTime(fuzzed_data_provider));
17+            banmap_t banmap_unexpired;
18+            ban_man.GetBanned(banmap_unexpired);
19+            SetMockTime(mock_time + 10);
20             banmap_t banmap;
21             ban_man.GetBanned(banmap);
22+            assert(banmap.size() < banmap_unexpired.size()); // At least one should have expired.
23             BanMan ban_man_read{banlist_file, nullptr, offset};
24             banmap_t banmap_read;
25             ban_man_read.GetBanned(banmap_read);
26             assert(banmap.size() == banmap_read.size());
27             auto it1{banmap.begin()};
28             auto it2{banmap_read.begin()};

in src/test/fuzz/banman.cpp:109 in fabadb7702 outdated

104+            banmap_t banmap_read;
105+            ban_man_read.GetBanned(banmap_read);
106+            assert(banmap.size() == banmap_read.size());
107+            auto it1{banmap.begin()};
108+            auto it2{banmap_read.begin()};
109+            for (; it1 != banmap.end(); ++it1, ++it2) {

vasild commented at 3:57 pm on June 23, 2021:

This may brick if banmap_t is changed from std::map to std::unordered_map. It should be changed because the latter is faster and we don’t need an order into this container.

Maybe add something like static_assert(std::is_base_of<std::map, banmap_t>::value) to ensure this gets adjusted if banmap_t is changed in the future?

Or a slower, but more robust variant: for each entry in banmap ensure that it can be found in banmap_read and that both maps have the same size.

vasild commented at 8:03 am on June 24, 2021:

 0             assert(banmap.size() == banmap_read.size());
 1-            auto it1{banmap.begin()};
 2-            auto it2{banmap_read.begin()};
 3-            for (; it1 != banmap.end(); ++it1, ++it2) {
 4-                assert(it1->first == it2->first);
 5-                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
 6+            for (const auto& [subnet, ban_entry] : banmap) {
 7+                const auto it = banmap_read.find(subnet);
 8+                assert(it != banmap_read.end());
 9+                assert(it->second.ToJson().write() == ban_entry.ToJson().write());
10             }

vasild commented at 8:18 am on June 24, 2021:

Or even better:

 0diff --git i/src/test/fuzz/banman.cpp w/src/test/fuzz/banman.cpp
 1index 6aa95b484f..15ccd9df3c 100644
 2--- i/src/test/fuzz/banman.cpp
 3+++ w/src/test/fuzz/banman.cpp
 4@@ -29,12 +29,18 @@ int64_t ConsumeBanTimeOffset(FuzzedDataProvider& fuzzed_data_provider) noexcept
 5 
 6 void initialize_banman()
 7 {
 8     static const auto testing_setup = MakeNoLogFileContext<>();
 9 }
10 
11+static bool operator==(const CBanEntry& lhs, const CBanEntry& rhs)
12+{
13+    return lhs.nVersion == rhs.nVersion && lhs.nCreateTime == rhs.nCreateTime &&
14+           lhs.nBanUntil == rhs.nBanUntil;
15+}
16+
17 FUZZ_TARGET_INIT(banman, initialize_banman)
18 {
19     // The complexity is O(N^2), where N is the input size, because each call
20     // might call DumpBanlist (or other methods that are at least linear
21     // complexity of the input size).
22     int limit_max_ops{300};
23@@ -100,18 +106,12 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
24             SetMockTime(ConsumeTime(fuzzed_data_provider));
25             banmap_t banmap;
26             ban_man.GetBanned(banmap);
27             BanMan ban_man_read{banlist_file, nullptr, offset};
28             banmap_t banmap_read;
29             ban_man_read.GetBanned(banmap_read);
30-            assert(banmap.size() == banmap_read.size());
31-            auto it1{banmap.begin()};
32-            auto it2{banmap_read.begin()};
33-            for (; it1 != banmap.end(); ++it1, ++it2) {
34-                assert(it1->first == it2->first);
35-                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
36-            }
37+            assert(banmap_read == banmap);
38         }
39     }
40     fs::remove(banlist_file.string() + ".dat");
41     fs::remove(banlist_file.string() + ".json");
42 }

MarcoFalke commented at 10:04 am on June 24, 2021:

Made it a compile failure to change the type without touching this file

vasild commented at 10:32 am on June 24, 2021:

This is now addressed in another way. Just out of curiosity - why not the assert(banmap_read == banmap); variant? It is shorter and works for both map and unordered_map.

in src/test/fuzz/banman.cpp:103 in fabadb7702 outdated

 94@@ -93,6 +95,22 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
 95                     ban_man.Discourage(ConsumeNetAddr(fuzzed_data_provider));
 96                 });
 97         }
 98+        if (!force_read_and_write_to_err) {
 99+            ban_man.DumpBanlist();
100+            SetMockTime(ConsumeTime(fuzzed_data_provider));
101+            banmap_t banmap;
102+            ban_man.GetBanned(banmap);
103+            BanMan ban_man_read{banlist_file, nullptr, offset};

vasild commented at 7:28 am on June 24, 2021:

The 3rd argument offset is only used by BanMan::Ban() which we never call on ban_man_read. So this can be

0            BanMan ban_man_read{banlist_file, nullptr, 0};

and then the offset variable is not necessary.

MarcoFalke commented at 10:04 am on June 24, 2021:

Thanks, done

MarcoFalke force-pushed on Jun 24, 2021

MarcoFalke commented at 10:05 am on June 24, 2021: member

Addressed feedback

vasild approved

vasild commented at 10:29 am on June 24, 2021: member

ACK fa57878340e6e9e34b01e26091d9d57f1c26f771

jonatack commented at 1:42 pm on June 24, 2021: member

Thanks for updating.

Tested ACK fa57878340e6e9e34b01e26091d9d57f1c26f771

fuzz: Check banman roundtrip fa485d06ec

MarcoFalke force-pushed on Jun 24, 2021

vasild approved

vasild commented at 5:41 am on June 25, 2021: member

ACK fa485d06ec10acd9a791f8d29689e1e82591fb70

practicalswift commented at 6:14 am on June 25, 2021: contributor

cr ACK fa485d06ec10acd9a791f8d29689e1e82591fb70

MarcoFalke merged this on Jun 25, 2021

MarcoFalke closed this on Jun 25, 2021

MarcoFalke deleted the branch on Jun 25, 2021

sidhujag referenced this in commit 68ec9eae51 on Jun 25, 2021

in src/test/fuzz/banman.cpp:112 in fa485d06ec

107+            banmap_t banmap;
108+            ban_man.GetBanned(banmap);
109+            BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ 0};
110+            banmap_t banmap_read;
111+            ban_man_read.GetBanned(banmap_read);
112+            assert(banmap == banmap_read);

hebasto commented at 1:48 pm on August 6, 2021:

Errors in CI job (https://cirrus-ci.com/task/6055643191705600?logs=ci#L2769):

0fuzz: test/fuzz/banman.cpp:112: void banman_fuzz_target(FuzzBufferType): Assertion `banmap == banmap_read' failed.

vasild commented at 2:28 pm on August 6, 2021:

Is there a way to retrieve ./crash-0671aac15e619e99522e2119487eaa9cc97e5a34 from the CI machine or make the fuzzer print it in base64?

Latest master (d67330d11245b11fbdd5e2dd5343ee451186931e) has been changed to:

https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/test/fuzz/banman.cpp#L111-L113

gwillen referenced this in commit 9a6ea2c704 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

fuzz: Check banman roundtrip #22322