Improve asmap checks and add sanity check

[sipa]

This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.

In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.

I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.

[sipa]

cc @naumenkogs.

[sipa]

@practicalswift There may be opportunity to fuzz here: if SanityCheckASMap succeeds, then no input to Interpret should return 0.

EDIT: Oh no, 0 can mean no match or error; it's still possible to reach no match.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• [RFC] bitcoin-asmap utility #18573 ([RFC] bitcoin-asmap utility by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[maflcko]

Would be nice to have the new code covered by some tests. Maybe the fuzzers?

diff --git a/src/test/fuzz/asmap.cpp b/src/test/fuzz/asmap.cpp
index 7f3eef79a1..1ab5459b12 100644
--- a/src/test/fuzz/asmap.cpp
+++ b/src/test/fuzz/asmap.cpp
@@ -25,4 +25,5 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         }
     }
     (void)net_addr.GetMappedAS(asmap);
+    (void)SanityCheckASMap(asmap);
 }

[naumenkogs]

Code review ACK 4719223
Tested: did some bit-flipping and saw that sanity check does not pass anymore.

Thank you for working on this.

[sipa]

A few changes:

• I've split out the changes into more commits
• Made it an error to reach EOF without a RETURN instruction
• Made the sanity checker slightly more tolerant (it now supports multiple jumps to the same location, a potential optimization that buildmap.py doesn't currently use).
• Added a commit that makes failures in the Interpreter assertion failures, and adds a fuzz tester that verifies this does not happen with asmaps that pass SanityCheckASMap.

I've verified that the fuzzer actually finds asmaps that pass the sanity check, and the Interpreter is run on it (by adding an early assertion failure in Interpreter). I'll try to share my corpus.

[naumenkogs]

Using gcc 9.2.1, AFL 2.5.2, got this at compilation for fuzzing:

libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::~UniqueLock()':
/home/gleb/bitcoin/src/./sync.h:169: undefined reference to `LeaveCritical()'
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::TryEnter(char const*, char const*, int)':
/home/gleb/bitcoin/src/./sync.h:139: undefined reference to `EnterCritical(char const*, char const*, int, void*, bool)'
/home/gleb/bitcoin/src/./sync.h:142: undefined reference to `LeaveCritical()'
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::Enter(char const*, char const*, int)':
/home/gleb/bitcoin/src/./sync.h:126: undefined reference to `EnterCritical(char const*, char const*, int, void*, bool)'
collect2: error: ld returned 1 exit status
Makefile:7281: recipe for target 'test/fuzz/addrman_deserialize' failed
make[2]: *** [test/fuzz/addrman_deserialize] Error 1
make[2]: *** Waiting for unfinished jobs....
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::~UniqueLock()':
/home/gleb/bitcoin/src/./sync.h:169: undefined reference to `LeaveCritical()'
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::TryEnter(char const*, char const*, int)':
/home/gleb/bitcoin/src/./sync.h:139: undefined reference to `EnterCritical(char const*, char const*, int, void*, bool)'
/home/gleb/bitcoin/src/./sync.h:142: undefined reference to `LeaveCritical()'
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::Enter(char const*, char const*, int)':
/home/gleb/bitcoin/src/./sync.h:126: undefined reference to `EnterCritical(char const*, char const*, int, void*, bool)'
collect2: error: ld returned 1 exit status
Makefile:7329: recipe for target 'test/fuzz/block' failed
make[2]: *** [test/fuzz/block] Error 1
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::~UniqueLock()':
/home/gleb/bitcoin/src/./sync.h:169: undefined reference to `LeaveCritical()'
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::TryEnter(char const*, char const*, int)':
/home/gleb/bitcoin/src/./sync.h:139: undefined reference to `EnterCritical(char const*, char const*, int, void*, bool)'
/home/gleb/bitcoin/src/./sync.h:142: undefined reference to `LeaveCritical()'
libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): In function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::Enter(char const*, char const*, int)':
/home/gleb/bitcoin/src/./sync.h:126: undefined reference to `EnterCritical(char const*, char const*, int, void*, bool)'
collect2: error: ld returned 1 exit status
Makefile:7369: recipe for target 'test/fuzz/block_header_and_short_txids_deserialize' failed
make[2]: *** [test/fuzz/block_header_and_short_txids_deserialize] Error 1
make[2]: Leaving directory '/home/gleb/bitcoin/src'
Makefile:18893: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/gleb/bitcoin/src'
Makefile:783: recipe for target 'all-recursive' failed
make: *** [all-recursive] Error 1

cc: @practicalswift

[maflcko]

@naumenkogs have you run make distclean?

[practicalswift]

Concept ACK

Thanks a lot for adding a fuzzing harness!

[naumenkogs]

@MarcoFalke
This helped.

@sipa
With the latest code, the map I generated with truncate -s 1M wrong_asmap seems to be valid. I don't think that's correct behavior?

[sipa]

@naumenkogs Nice catch. DecodeBits wasn't returning failure when EOF occurred in the mantissa bits. Should be fixed now. I believe it should be impossible to take a valid asmap and truncate it, and having it still be acceptable.

[naumenkogs]

ACK 906e031

[sipa]

Now added an extra check that verifies that no truncation of a valid asmap remains valid.

[hebasto]

@sipa
Mind addressing a compiler warning issue on ARM 32-bit platforms: d0b10ed from #18686 ?

[practicalswift]

ACK 7489776 modulo feedback below.

@sipa, would it be possible to make it so that the existing coverage-increasing inputs to src/test/fuzz/asmap are not invalidated by the merge of this PR? The qa-assets inputs achieve very comprehensive coverage for asmap thanks to previous fuzzing efforts. It would be nice to keep those inputs. Perhaps your src/test/fuzz/asmap.cpp changes can be moved to a separate new fuzzing harness file? :)

[jonatack]

Thanks for adding sanity checks and fuzzing.

ACK 7489776 code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.

fuzz/asmap_direct

#900803874	REDUCE cov: 730 ft: 3838 corp: 210/19Kb exec/s: 19950 rss: 562Mb L: 78/2049 MS: 1 EraseBytes-
#906401851	REDUCE cov: 730 ft: 3838 corp: 210/19Kb exec/s: 19899 rss: 562Mb L: 249/2049 MS: 2 InsertRepeatedBytes-EraseBytes-
#906756962	REDUCE cov: 730 ft: 3838 corp: 210/19Kb exec/s: 19895 rss: 562Mb L: 247/2049 MS: 1 EraseBytes-

fuzz/asmap

#183578499	REDUCE cov: 1226 ft: 3413 corp: 222/7168b exec/s: 3930 rss: 421Mb L: 39/147 MS: 1 EraseBytes-
#184582017	REDUCE cov: 1226 ft: 3413 corp: 222/7165b exec/s: 3928 rss: 421Mb L: 116/147 MS: 2 EraseBytes-ChangeByte-
#185253963	REDUCE cov: 1226 ft: 3413 corp: 222/7164b exec/s: 3928 rss: 421Mb L: 115/147 MS: 1 EraseBytes-

A few non-blocking ideas for your consideration below.

[jonatack]

nit: Perhaps use a switch statement for the opcode conditionals, here and in SanityCheckASMap, to make explicit the nature of the operation (testing a single value against a set of scoped enumerations) and possibly generate better code with a jump table instead of repeatedly checking individual values.

[jonatack]

nit suggestion:

+    size_t bits = buffer.size() - sep_pos - 1;
-    if (buffer.size() - sep_pos - 1 > 128) return; // At most 128 bits in IP address
+    if (bits > 128) return; // At most 128 bits in IP address
 
     // Checks on asmap
     std::vector<bool> asmap(buffer.begin(), buffer.begin() + sep_pos);
-    if (SanityCheckASMap(asmap, buffer.size() - 1 - sep_pos)) {
+    if (SanityCheckASMap(asmap, bits)) {
         // Verify that for valid asmaps, no prefix (except up to 7 zero padding bits) is valid.
         std::vector<bool> asmap_prefix = asmap;
         while (!asmap_prefix.empty() && asmap_prefix.size() + 7 > asmap.size() && asmap_prefix.back() == false) asmap_prefix.pop_back();
         while (!asmap_prefix.empty()) {
             asmap_prefix.pop_back();
-            assert(!SanityCheckASMap(asmap_prefix, buffer.size() - 1 - sep_pos));
+            assert(!SanityCheckASMap(asmap_prefix, bits));

[jonatack]

nit suggestion:

-    bool have_sep = false;
+    int separators{0};
     size_t sep_pos;
     for (size_t pos = 0; pos < buffer.size(); ++pos) {
         uint8_t x = buffer[pos];
         if ((x & 0xFE) == 0) continue;
         if (x == 0xFF) {
-            if (have_sep) return;
-            have_sep = true;
+            if (separators != 0) return;
+            separators += 1;
             sep_pos = pos;
         } else {
             return;
         }
     }
-    if (!have_sep) return; // Needs exactly 1 separator
+    if (separators != 1) return; // Needs exactly 1 separator

[fjahr]

@jonatack Even in your code the only values the variable can have is 0 and 1. Why would you want to use an int instead of a bool then?

[jonatack]

My thoughts were (a) a sanity check is better than just eyeballing code, not only for now but also future changes, and (b) a check for exactly one is more strongly verified when a state of more than one is also tested.

[fjahr]

ACK 7489776

Reviewed code and tested asmap and asmap_direct fuzzers. Confirmed that bitcoind starts with a correct map and Sanity checker catches a corrupt file.

[fjahr]

nit: It's a bit inconsistent that this function has ASMap in it while DecodeAsmap for example has Asmap.