tests: Add fuzzing harness for CNode #19067

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:fuzzers-2020-05-25 changing 2 files +163 −0
  1. practicalswift commented at 3:06 pm on May 25, 2020: contributor

    Add fuzzing harness for CNode.

    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 force-pushed on May 25, 2020
  3. in src/test/fuzz/net.cpp:24 in 7960082e50 outdated 
    19+#include <string>
20+#include <vector>
21+
22+void initialize()
23+{
24+    static TestingSetup no_log_regtest_setup{CBaseChainParams::REGTEST, {"-nodebuglogfile"}};
    practicalswift commented at 3:18 pm on May 25, 2020:
    Note to self: TestingSetup introduces non-determinism and is way too heavy to use just to disable logging. Consider switching to something more lightweight which does exactly what we want and nothing more :)
  4. DrahtBot added the label Build system on May 25, 2020
  5. DrahtBot added the label Tests on May 25, 2020
  6. jb55 commented at 4:40 pm on May 25, 2020: member
    Concept ACK
  7. practicalswift force-pushed on May 25, 2020
  8. fanquake removed the label Build system on May 26, 2020
  9. in src/test/fuzz/net.cpp:152 in 086991817e outdated 
    147+    const int ref_count = node.GetRefCount();
148+    assert(ref_count >= 0);
149+    (void)node.GetSendVersion();
150+    (void)node.IsAddrRelayPeer();
151+
152+    const NetPermissionFlags net_permission_flags = fuzzed_data_provider.ConsumeBool() ? fuzzed_data_provider.PickValueInArray<NetPermissionFlags>({
    MarcoFalke commented at 12:05 pm on May 26, 2020: 
    0    const NetPermissionFlags net_permission_flags = fuzzed_data_provider.ConsumeBool() ?
1    fuzzed_data_provider.PickValueInArray<NetPermissionFlags>({

    Couldn’t this start a new line after the ? Seems overly wasteful to fill the screen with 8 * 90 pure whitespace

    practicalswift commented at 12:57 pm on May 26, 2020:

    Sure! Now fixed: clang-format:ed version replaced with another clang-format:ed version after manual re-arrangement.

    When it comes to formatting I really don’t have any personal preferences: I’m happy as long as clang-format is happy :)

  10. practicalswift force-pushed on May 26, 2020
  11. practicalswift force-pushed on May 26, 2020
  12. practicalswift force-pushed on May 26, 2020
  13. practicalswift commented at 7:21 pm on June 28, 2020: contributor
    Anything left to do here? :) The changes are limited to src/test/fuzz/ and should hopefully be trivial to review.
  14. Crypt-iQ commented at 2:13 am on July 14, 2020: contributor
    Fails to build with the same error as #19065 , rebasing it onto master reveals that PushBlockHash & PushInventory no longer exist since 344e831de54f7b864f03a90f6cb19692eafcd463.
  15. MarcoFalke added the label Needs rebase on Jul 14, 2020
  16. DrahtBot removed the label Needs rebase on Jul 14, 2020
  17. in src/test/fuzz/net.cpp:115 in 2206f168f6 outdated 
    110+        case 10: {
111+            const std::optional<CInv> inv_opt = ConsumeDeserializable<CInv>(fuzzed_data_provider);
112+            if (!inv_opt) {
113+                break;
114+            }
115+            node.PushInventory(*inv_opt);
    MarcoFalke commented at 7:29 am on July 14, 2020: 
     0test/fuzz/net.cpp:115:18: error: no member named 'PushInventory' in 'CNode'
 1
 2            node.PushInventory(*inv_opt);
 3
 4            ~~~~ ^
 5
 6test/fuzz/net.cpp:119:18: error: no member named 'PushBlockHash' in 'CNode'
 7
 8            node.PushBlockHash(ConsumeUInt256(fuzzed_data_provider));
 9
10            ~~~~ ^
11
122 errors generated.
    practicalswift commented at 7:58 am on July 14, 2020:
    Oh, thanks for letting me know! Now fixed!
  18. practicalswift force-pushed on Jul 14, 2020
  19. practicalswift commented at 7:58 am on July 14, 2020: contributor
    @Crypt-iQ Thanks for reporting. Should be fixed now. Would you mind retrying? :)
  20. Crypt-iQ commented at 10:26 pm on July 17, 2020: contributor
    @practicalswift Fuzzing with clang-10 libfuzzer. afl-gcc/g++ can also build on a22b42e.
  21. in src/test/fuzz/net.cpp:40 in a22b42e441 outdated 
    35+    const std::optional<CAddress> address_bind = ConsumeDeserializable<CAddress>(fuzzed_data_provider);
36+    if (!address_bind) {
37+        return;
38+    }
39+
40+    CNode node{fuzzed_data_provider.ConsumeIntegral<NodeId>(), static_cast<ServiceFlags>(fuzzed_data_provider.ConsumeIntegral<uint64_t>()), fuzzed_data_provider.ConsumeIntegral<int>(), INVALID_SOCKET, *address, fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), *address_bind, fuzzed_data_provider.ConsumeRandomLengthString(32), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool()};
    Crypt-iQ commented at 12:50 pm on July 19, 2020:

    If the fuzzer didn’t always pass in INVALID_SOCKET, every line would be covered by the fuzzer in CNode::CloseSocketDisconnect().

    Maybe instead of doing this CNode node {...}, you could make a function ConsumeCNode that can also be used in the process_message.cpp & process_messages.cpp fuzzing harnesses. I think this would improve coverage of those fuzzing harnesses too :).

    practicalswift commented at 5:50 pm on August 14, 2020:
    I think that lack of coverage in CNode::CloseSocketDisconnect is okay: INVALID_SOCKET basically guarantees that all CNode operations are safe (that’s how it is done in the unit tests). We can look at expanding the coverage safely in a future PR :)
    Crypt-iQ commented at 7:10 pm on August 15, 2020:
    Ok that makes sense to me about INVALID_SOCKET. What about void AdvertiseLocal(CNode *pnode)?
    practicalswift commented at 4:01 pm on August 16, 2020:
    @Crypt-iQ Both IsPeerAddrLocalGood and AdvertiseLocal depend on global state (fDiscover and fListen), and in the case of AdvertiseLocal a FastRandomContext is deciding which execution path to take. I think they’re sufficiently different to do them in another PR taking those things into account to achieve maximum possible determinism. Makes sense? :)
    Crypt-iQ commented at 6:45 pm on August 16, 2020:
    Yes that makes sense 👍
    MarcoFalke commented at 2:37 pm on August 27, 2020: 
    0    CNode node{fuzzed_data_provider.ConsumeIntegral<NodeId>(), static_cast<ServiceFlags>(fuzzed_data_provider.ConsumeIntegral<uint64_t>()), fuzzed_data_provider.ConsumeIntegral<int>(), INVALID_SOCKET, *address, fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), *address_bind, fuzzed_data_provider.ConsumeRandomLengthString(32), fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool(),};

    Could add a trailing comma and then clang format to avoid an excessively long line?

    practicalswift commented at 5:51 pm on August 27, 2020:
    Good point. Fixed!
  22. in src/test/fuzz/net.cpp:57 in a22b42e441 outdated 
    43+        case 0: {
44+            node.CloseSocketDisconnect();
45+            break;
46+        }
47+        case 1: {
48+            node.MaybeSetAddrName(fuzzed_data_provider.ConsumeRandomLengthString(32));
    Crypt-iQ commented at 12:50 pm on July 19, 2020:
    Since this always has length of 32, not every line is covered by the fuzzer in MaybeSetAddrName.
    practicalswift commented at 5:51 pm on August 14, 2020:
    ConsumeRandomLengthString(32) will return a string up to 32 characters long :)
    Crypt-iQ commented at 7:14 pm on August 15, 2020:
    Nvm. 👍
  23. Crypt-iQ commented at 1:16 pm on July 19, 2020: contributor

    30 hour fuzzing coverage for this harness (libfuzzer --with-sanitizers=address,fuzzer,undefined): https://crypt-iq.github.io/cnode_cov/src/index.html

    Just minor comments from me. Two more functions could be fuzzed:

    • bool IsPeerAddrLocalGood(CNode *pnode)
    • void AdvertiseLocal(CNode *pnode)
  24. Crypt-iQ commented at 6:45 pm on August 16, 2020: contributor
    Tested ACK a22b42e
  25. practicalswift commented at 7:35 pm on August 26, 2020: contributor
    Ready for merge?
  26. MarcoFalke commented at 2:35 pm on August 27, 2020: member
    needs rebase
  27. tests: Add fuzzing harness for CNode cc26fab48d
  28. practicalswift force-pushed on Aug 27, 2020
  29. practicalswift commented at 5:52 pm on August 27, 2020: contributor
    @jb55 @Crypt-iQ @MarcoFalke Thanks a lot for reviewing. Now rebased. Please re-review :)
  30. MarcoFalke merged this on Aug 28, 2020
  31. MarcoFalke closed this on Aug 28, 2020
  32. sidhujag referenced this in commit 3f5b13d9ba on Aug 28, 2020
  33. deadalnix referenced this in commit 34ae942842 on Feb 9, 2021
  34. practicalswift deleted the branch on Apr 10, 2021
  35. PastaPastaPasta referenced this in commit 805decd086 on Jul 17, 2022
  36. kittywhiskers referenced this in commit c74aec6409 on Aug 11, 2022
  37. DrahtBot locked this on Aug 18, 2022


practicalswift jb55 MarcoFalke Crypt-iQ

Labels
Tests

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-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me