fuzz: Add fuzzing harness for node eviction logic #19972

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:fuzzers-eviction-logic changing 2 files +45 −0
  1. practicalswift commented at 3:04 pm on September 18, 2020: contributor

    Add fuzzing harness for node eviction logic.

    See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don’t forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

    Happy fuzzing :)

  2. practicalswift renamed this:
    tests: Add fuzzing harness for node eviction logic
    tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    on Sep 18, 2020
  3. DrahtBot added the label Build system on Sep 18, 2020
  4. DrahtBot added the label P2P on Sep 18, 2020
  5. jonatack commented at 5:59 pm on September 18, 2020: member
    Concept ACK, thanks for working on this. Will review.
  6. practicalswift force-pushed on Sep 19, 2020
  7. DrahtBot commented at 1:23 pm on September 19, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19415 (net: Make DNS lookup mockable, add fuzzing harness by practicalswift)

    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.

  8. jonatack commented at 4:53 pm on September 20, 2020: member

    ACK c57cabdea44cab2a9dd2f0443f7a6b28cb41e7b4 code review, ran the fuzzer up to 10M execs, ran bitcoind grepping the net logging and observing the peer conns. Consider maybe running clang-format on the changes.

    0[#10750687](/bitcoin-bitcoin/10750687/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 631Mb L: 2188/4091 MS: 1 EraseBytes-
1[#10761148](/bitcoin-bitcoin/10761148/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 1430/4091 MS: 1 EraseBytes-
2[#10761655](/bitcoin-bitcoin/10761655/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 3142/4091 MS: 2 PersAutoDict-EraseBytes- DE: "_\xb0\x00\x8d\x8d\x8d\x8d\x8d\x8d\x8d\x8d\x8d"-
3[#10770237](/bitcoin-bitcoin/10770237/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 642Mb L: 2961/4091 MS: 2 ChangeBinInt-EraseBytes-
4[#10778939](/bitcoin-bitcoin/10778939/)	REDUCE cov: 1502 ft: 7161 corp: 469/821Kb lim: 4096 exec/s: 527 rss: 643Mb L: 3266/4091 MS: 2 ChangeByte-EraseBytes-
  9. jonatack commented at 5:44 pm on September 20, 2020: member

    (am running the node with this logging)

     0+++ b/src/net.cpp
 1@@ -972,6 +972,7 @@ bool CConnman::AttemptToEvictConnection()
 2 
 3     const Optional<NodeEvictionCandidate> node_to_evict = SelectNodeToEvict(vEvictionCandidates);
 4     if (!node_to_evict) {
 5+        LogPrintf("ATEC: no node to evict returned by SelectNodeToEvict()\n");
 6         return false;
 7     }
 8     const NodeId evicted = node_to_evict->id;
 9@@ -979,9 +980,12 @@ bool CConnman::AttemptToEvictConnection()
10     for (CNode* pnode : vNodes) {
11         if (pnode->GetId() == evicted) {
12             pnode->fDisconnect = true;
13+            LogPrintf("ATEC: node to evict FOUND in vEvictioncandidates and disconnected: %s\n", evicted);
14             return true;
15         }
16     }
17+    LogPrintf("ATEC: node to evict selected but not found in vEvictioncandidates: %s\n", evicted);
18+
19     return false;
20 }
  10. jonatack commented at 7:49 am on October 20, 2020: member
    A month has passed since reviewing, so a gentle bump for reviewers.
  11. MarcoFalke removed the label Build system on Oct 20, 2020
  12. in src/net.cpp:879 in 818dd2a04d outdated 
    911-                                               node->m_prefer_evict, node->addr.IsLocal()};
912-            vEvictionCandidates.push_back(candidate);
913-        }
914-    }
915-
916+NODISCARD Optional<NodeEvictionCandidate> SelectNodeToEvict(std::vector<NodeEvictionCandidate> vEvictionCandidates) {
    sipa commented at 1:44 am on November 5, 2020:
    Nit: brace on the next line.
    sipa commented at 1:47 am on November 5, 2020:

    This will cause an unnecessary copy of the vEvictionCandidates vector when invoking.

    As the caller doesn’t need its version anymore after the call, it’s possible to change this parameter to std::vector<NodeEvictionCandidate>&&, and call it with SelectNodeToEvict(std::move(vEvictionCandidates)).

    practicalswift commented at 3:26 pm on November 5, 2020:

    Feedback addressed.

    Also reduced the scope of std::vector<NodeEvictionCandidate> vEvictionCandidates to make reasoning about the life-time easier.

  13. in src/test/fuzz/node_eviction.cpp:18 in c57cabdea4 outdated 
    13+#include <cassert>
14+#include <cstdint>
15+#include <optional>
16+#include <vector>
17+
18+NODISCARD Optional<NodeEvictionCandidate> SelectNodeToEvict(std::vector<NodeEvictionCandidate> vEvictionCandidates);
    sipa commented at 1:51 am on November 5, 2020:
    Since this file is already including net.h, I’d prefer seeing this declaration moved there, rather than forward declaring it (as it risks going out of sync with the implementation here).
    practicalswift commented at 3:19 pm on November 5, 2020:
    Makes sense. Feedback addressed. Thanks!
  14. in src/test/fuzz/node_eviction.cpp:37 in c57cabdea4 outdated 
    32+    if (node_to_evict) {
33+        assert(std::any_of(eviction_candidates.begin(), eviction_candidates.end(), [&node_to_evict](const NodeEvictionCandidate& eviction_candidate) { return node_to_evict->id == eviction_candidate.id; }));
34+    }
35+    // eviction_candidates.empty() implies !node_to_evict, but !node_to_evict
36+    // does not imply eviction_candidates.empty()).
37+    if (eviction_candidates.empty()) {
    sipa commented at 1:53 am on November 5, 2020:
    I believe this test is redundant; if eviction_candidates is empty but node_to_evict isn’t, the std::any_of call above must fail.
    practicalswift commented at 3:20 pm on November 5, 2020:
    Good catch. Feedback addressed. Thanks!
  15. sipa commented at 1:53 am on November 5, 2020: member
    Concept ACK
  16. DrahtBot added the label Needs rebase on Nov 5, 2020
  17. practicalswift force-pushed on Nov 5, 2020
  18. practicalswift force-pushed on Nov 5, 2020
  19. practicalswift force-pushed on Nov 5, 2020
  20. DrahtBot removed the label Needs rebase on Nov 5, 2020
  21. practicalswift renamed this:
    tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    test/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    on Nov 24, 2020
  22. practicalswift renamed this:
    test/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    on Nov 24, 2020
  23. practicalswift renamed this:
    fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    [draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    on Dec 6, 2020
  24. practicalswift commented at 9:33 pm on December 6, 2020: contributor
    Marking this as draft since it depends on the changes in #20477 (“test/net: Add unit testing of node eviction logic”) :)
  25. DrahtBot added the label Needs rebase on Dec 7, 2020
  26. practicalswift force-pushed on Dec 7, 2020
  27. DrahtBot removed the label Needs rebase on Dec 7, 2020
  28. DrahtBot added the label Needs rebase on Dec 15, 2020
  29. practicalswift force-pushed on Dec 16, 2020
  30. practicalswift force-pushed on Dec 16, 2020
  31. DrahtBot removed the label Needs rebase on Dec 16, 2020
  32. practicalswift force-pushed on Dec 16, 2020
  33. practicalswift renamed this:
    [draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
    fuzz: Add fuzzing harness for node eviction logic
    on Dec 16, 2020
  34. practicalswift commented at 12:49 pm on December 16, 2020: contributor

    Rebased on top of master now that the prerequisite #20477 has been merged.

    This PR is now touches only the fuzz tests and should hopefully be easy to review :)

  35. in src/test/fuzz/node_eviction.cpp:23 in 42321f469f outdated 
    18+FUZZ_TARGET(node_eviction)
19+{
20+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
21+    std::vector<NodeEvictionCandidate> eviction_candidates;
22+    while (fuzzed_data_provider.ConsumeBool()) {
23+        eviction_candidates.push_back({fuzzed_data_provider.ConsumeIntegral<NodeId>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeIntegral<int64_t>(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool()});
    MarcoFalke commented at 12:55 pm on December 16, 2020:
    For the sanity of future devs, please add a trailing comma
    practicalswift commented at 1:01 pm on December 16, 2020:
    Thanks! Fixed!
  36. MarcoFalke commented at 12:57 pm on December 16, 2020: member
    Aren’t the code paths already exercised through the unit test?
  37. tests: Add fuzzing harness for node eviction logic 5a9ee0869b
  38. practicalswift force-pushed on Dec 16, 2020
  39. practicalswift commented at 1:17 pm on December 16, 2020: contributor

    Aren’t the code paths already exercised through the unit test?

    There are differences: the unit test clamp the eviction candidate parameters (like static_cast<int64_t>(random_context.randrange(100)) ) in order to trigger duplicate values, whereas this fuzzing harness can span the entire input space (like fuzzed_data_provider.ConsumeIntegral<int64_t>()). The latter is good for catching integer overflows and similar stuff resulting from “extreme” inputs.

    Also the unit testing performs a couple of thousand test eviction rounds whereas the fuzzing test runs indefinitely :)

    I think it makes sense to both unit test and fuzz test SelectNodeToEvict since that function is of extreme importance :)

  40. MarcoFalke commented at 1:20 pm on December 16, 2020: member
    cr ACK 5a9ee0869b0b722ebfcdaabaefba6376522b2eeb
  41. MarcoFalke merged this on Dec 25, 2020
  42. MarcoFalke closed this on Dec 25, 2020
  43. sidhujag referenced this in commit 92e5b1d1c6 on Dec 25, 2020
  44. in src/test/fuzz/node_eviction.cpp:34 in 5a9ee0869b 
    29+            fuzzed_data_provider.ConsumeBool(),
30+            fuzzed_data_provider.ConsumeBool(),
31+            fuzzed_data_provider.ConsumeBool(),
32+            fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
33+            fuzzed_data_provider.ConsumeBool(),
34+            fuzzed_data_provider.ConsumeBool(),
    jonatack commented at 11:48 pm on December 25, 2020:
    Following this merge I needed to update this fuzzer for #20197 and found the documentation for each data member previously added in net_tests.cpp both useful and missing here. Added it in #20197.
  45. jonatack commented at 11:50 pm on December 25, 2020: member
    ACK 5a9ee0869b0b722ebfcdaabaefba6376522b2eeb
  46. practicalswift deleted the branch on Apr 10, 2021
  47. DrahtBot locked this on Aug 16, 2022


practicalswift jonatack DrahtBot sipa MarcoFalke

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me