fuzz: Add fuzzing harness for TorController #19288

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:fuzzers-torcontroller changing 4 files +216 −120
  1. practicalswift commented at 9:24 PM on June 15, 2020: contributor

    Add fuzzing harness for TorController.

    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 Jun 15, 2020
  3. DrahtBot added the label Build system on Jun 15, 2020
  4. DrahtBot added the label P2P on Jun 15, 2020
  5. DrahtBot added the label Tests on Jun 15, 2020
  6. DrahtBot commented at 9:52 AM on June 16, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)

    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.

  7. DrahtBot added the label Needs rebase on Jul 11, 2020
  8. practicalswift force-pushed on Jul 11, 2020
  9. DrahtBot removed the label Needs rebase on Jul 11, 2020
  10. laanwj commented at 1:43 PM on July 15, 2020: member

    Concept ACK I don't really like moving all kinds of internal details to the public header, but testability and encapsuation are always a bit at odds.

  11. in src/test/fuzz/torcontrol.cpp:63 in 38c5e21d38 outdated 
      54 | +        }
  55 | +        tor_control_reply.lines = ConsumeRandomLengthStringVector(fuzzed_data_provider);
  56 | +        if (tor_control_reply.lines.empty()) {
  57 | +            break;
  58 | +        }
  59 | +        DummyTorControlConnection dummy_tor_control_connection;
    Crypt-iQ commented at 6:24 AM on July 23, 2020:

    Can't this be replaced by TorControlConnection tor_control_connection{nullptr}; and then you can get rid of the DummyTorControlConnection class?

    practicalswift commented at 4:46 PM on August 14, 2020:

    We don't want TorControlConnection::{Connect,Disconnect,Command} to be called, right? :) My intention with adding DummyTorControlConnection was to make sure {Connect,Disconnect,Command} return immediately (and thus be guaranteed to be safe regardless of the state of things).

    Crypt-iQ commented at 5:16 PM on August 14, 2020:

    Yes I think that's the right approach, I think my coverage confused me again as it showed DummyTorControlConnection as unused.

  12. Crypt-iQ commented at 6:44 AM on July 23, 2020: contributor

    48 hour coverage with libfuzzer here: https://crypt-iq.github.io/torcontrol_cov/src/torcontrol.cpp.gcov.html

    Currently two functions are coverage lacking from the above output:

    • authchallenge_cb
    • protocolinfo_cb

    authchallenge_cb coverage is a bit tricky to get coverage for because it has to pass the HMAC check. protocolinfo_cb just needs better seeds and it should be able to hit every part but this one: https://github.com/bitcoin/bitcoin/blob/9d4b3d86b694ac6e56495e1955f6bf5ff584cbb9/src/torcontrol.cpp#L643-L652

  13. DrahtBot added the label Needs rebase on Aug 9, 2020
  14. practicalswift force-pushed on Aug 14, 2020
  15. jonatack commented at 4:56 PM on August 14, 2020: member

    Concept ACK

  16. DrahtBot removed the label Needs rebase on Aug 14, 2020
  17. practicalswift force-pushed on Aug 17, 2020
  18. practicalswift force-pushed on Aug 18, 2020
  19. Crypt-iQ commented at 9:41 AM on August 26, 2020: contributor

    Runs without complaint on Ubuntu 18: ./configure --enable-fuzz --with-sanitizers=address,undefined,fuzzer CC=clang-10 CXX=clang++-10

    Will build on macOS and report back.

  20. Crypt-iQ commented at 9:29 PM on August 28, 2020: contributor

    Builds on macOS: ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined no actionable complaints ./configure --enable-fuzz --with-sanitizers=fuzzer,integer,undefined no actionable complaints

    Ran with Ubuntu 18 again: ./configure --enable-fuzz --with-sanitizers=fuzzer,integer:

    /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.tcc:1271:25: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::size_type' (aka 'unsigned long')
[#0](/bitcoin-bitcoin/0/) 0x55db7101bde3 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::rfind(char, unsigned long) const /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.tcc:1271:25
[#1](/bitcoin-bitcoin/1/) 0x55db7101bde3 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::find_last_of(char, unsigned long) const /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.h:2636:22
[#2](/bitcoin-bitcoin/2/) 0x55db7101bde3 in SplitHostPort(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /root/bitcoin/src/util/strencodings.cpp:111:23
[#3](/bitcoin-bitcoin/3/) 0x55db70fc30f6 in Lookup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<CService, std::allocator<CService> >&, int, bool, unsigned int) /root/bitcoin/src/netbase.cpp:213:5
[#4](/bitcoin-bitcoin/4/) 0x55db70fc36ca in Lookup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CService&, int, bool) /root/bitcoin/src/netbase.cpp:237:17
[#5](/bitcoin-bitcoin/5/) 0x55db70fc389b in LookupNumeric(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int) /root/bitcoin/src/netbase.cpp:262:9
[#6](/bitcoin-bitcoin/6/) 0x55db70ec5bfa in TorController::auth_cb(TorControlConnection&, TorControlReply const&) /root/bitcoin/src/torcontrol.cpp:411:31
[#7](/bitcoin-bitcoin/7/) 0x55db70ebb54b in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /root/bitcoin/src/test/fuzz/torcontrol.cpp:66:28
[#8](/bitcoin-bitcoin/8/) 0x55db71583916 in LLVMFuzzerTestOneInput /root/bitcoin/src/test/fuzz/fuzz.cpp:45:5
[#9](/bitcoin-bitcoin/9/) 0x55db70e4b641 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/bitcoin/src/test/fuzz/torcontrol+0x649641)
[#10](/bitcoin-bitcoin/10/) 0x55db70e4ad85 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/root/bitcoin/src/test/fuzz/torcontrol+0x648d85)
[#11](/bitcoin-bitcoin/11/) 0x55db70e4d6a7 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/bitcoin/src/test/fuzz/torcontrol+0x64b6a7)
[#12](/bitcoin-bitcoin/12/) 0x55db70e4da09 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/bitcoin/src/test/fuzz/torcontrol+0x64ba09)
[#13](/bitcoin-bitcoin/13/) 0x55db70e3c6de in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/bitcoin/src/test/fuzz/torcontrol+0x63a6de)
[#14](/bitcoin-bitcoin/14/) 0x55db70e65522 in main (/root/bitcoin/src/test/fuzz/torcontrol+0x663522)
[#15](/bitcoin-bitcoin/15/) 0x7f13c3c4fb96 in __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:310
[#16](/bitcoin-bitcoin/16/) 0x55db70e11459 in _start (/root/bitcoin/src/test/fuzz/torcontrol+0x60f459)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.tcc:1271:25
  21. Crypt-iQ commented at 5:34 AM on August 29, 2020: contributor

    ACK 9b825840daf72f20444c2ec13c3aa681df8b594f

  22. practicalswift commented at 9:49 AM on August 29, 2020: contributor

    @Crypt-iQ Thanks for reviewing! I've now added relevant -fsanitize=integer suppressions (all non-UB). Please re-review :)

  23. in test/sanitizer_suppressions/ubsan:77 in e87294918c outdated 
      77 | +implicit-integer-sign-change:util/strencodings.cpp
  78 | +implicit-integer-sign-change:util/strencodings.h
  79 |  implicit-integer-sign-change:validation.cpp
  80 |  implicit-integer-sign-change:zmq/zmqpublishnotifier.cpp
  81 | -implicit-signed-integer-truncation,implicit-integer-sign-change:chain.h
  82 | -implicit-signed-integer-truncation,implicit-integer-sign-change:test/skiplist_tests.cpp
    Crypt-iQ commented at 2:57 PM on August 29, 2020:

    chain.h & test/skiplist_tests.cpp missing the implicit-signed-integer-truncation suppression in the new version. Then CI should pass.

    practicalswift commented at 9:36 AM on September 1, 2020:

    Thanks! Should be fixed now.

  24. in test/sanitizer_suppressions/ubsan:9 in e87294918c outdated 
      11 | @@ -12,6 +12,7 @@ float-divide-by-zero:wallet/wallet.cpp
  12 |  # contains files in which we expect unsigned integer overflows to occur. The
  13 |  # list is used to suppress -fsanitize=integer warnings when running our CI UBSan
  14 |  # job.
  15 | +unsigned-integer-overflow:*/include/c++/*/bits/basic_string.tcc
    Crypt-iQ commented at 6:03 PM on August 29, 2020:

    This will hide other unsigned int overflows that occur from different call sites. I think it should eventually be deleted, no?

    practicalswift commented at 9:41 AM on September 1, 2020:

    AFAICT we can't do anything meaningful about these unsigned integer wraparounds in basic_string.tcc. I think that suppressing them is fine: unsigned integer wraparound is not UB after all :)

  25. practicalswift force-pushed on Sep 1, 2020
  26. practicalswift force-pushed on Sep 1, 2020
  27. Crypt-iQ commented at 2:31 PM on September 5, 2020: contributor

    ACK f87801d2fc0e27271aa71861979a8ef3e1fe935f

  28. MarcoFalke removed the label Build system on Sep 6, 2020
  29. practicalswift commented at 5:15 PM on September 12, 2020: contributor

    Ready for merge? :)

  30. jonatack commented at 5:28 PM on September 12, 2020: member

    Oh, nice -- will try to review this soon.

  31. practicalswift commented at 1:22 PM on September 17, 2020: contributor

    Had to rebase after the merge of 15c27c44417ab77a660b53b8574f7eb5261b19f8. @Crypt-iQ Thanks a lot for reviewing. May I ask you a re-review and hopefully give your third(!) ACK for this change? :)

  32. practicalswift force-pushed on Sep 17, 2020
  33. Crypt-iQ commented at 10:50 AM on September 20, 2020: contributor

    ACK 2a5c8188209cc6d6654ef3fefb80acbb87c134a1

  34. DrahtBot added the label Needs rebase on Oct 2, 2020
  35. practicalswift force-pushed on Oct 6, 2020
  36. practicalswift commented at 7:41 PM on October 6, 2020: contributor

    Had to rebase again due to a merge conflict. @Crypt-iQ Again: thanks a lot for your efforts in reviewing the fuzzing harnesses. May I ask you a re-review and hopefully give your fourth(!) ACK for this change? :)

    Fuzzing review beg: If we only had two or three more reviewers (like @Crypt-iQ!) who were willing to review fuzzing harnesses that would make such a huge difference for the state of fuzzing in the project. Please consider reviewing! ❤️

    Some fuzzing harnesses awaiting review:

    • #19065: tests: Add fuzzing harness for CAddrMan. Fill some fuzzing coverage gaps.
    • #19203: net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. Add thin SOCKET wrapper.
    • #19259: tests: Add fuzzing harness for LoadMempool(...) and DumpMempool(...)
    • #19288: tests: Add fuzzing harness for TorController
    • #19415: net: Make DNS lookup mockable, add fuzzing harness
    • #19972: tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.
  37. DrahtBot removed the label Needs rebase on Oct 6, 2020
  38. practicalswift renamed this:
    tests: Add fuzzing harness for TorController
    test: Add fuzzing harness for TorController
    on Nov 24, 2020
  39. practicalswift renamed this:
    test: Add fuzzing harness for TorController
    fuzz: Add fuzzing harness for TorController
    on Nov 24, 2020
  40. Crypt-iQ commented at 3:44 PM on December 5, 2020: contributor

    Will review again @practicalswift

  41. Crypt-iQ commented at 10:32 AM on December 14, 2020: contributor

    ACK 00e48011c3e1235077353ca7d209661de66e9aab

    Reviewed code and ran locally on macOS 10.15.4 with clang-10.

  42. DrahtBot added the label Needs rebase on Dec 15, 2020
  43. practicalswift force-pushed on Dec 16, 2020
  44. DrahtBot removed the label Needs rebase on Dec 16, 2020
  45. practicalswift force-pushed on Dec 16, 2020
  46. DrahtBot added the label Needs rebase on Jan 4, 2021
  47. in test/sanitizer_suppressions/ubsan:61 in 3f3982c4e1 outdated 
      57 | @@ -56,6 +58,7 @@ implicit-integer-sign-change:script/interpreter.cpp
  58 |  implicit-integer-sign-change:serialize.h
  59 |  implicit-integer-sign-change:test/arith_uint256_tests.cpp
  60 |  implicit-integer-sign-change:test/coins_tests.cpp
  61 | +implicit-integer-sign-change:test/fuzz/FuzzedDataProvider.h
    MarcoFalke commented at 11:01 AM on January 4, 2021:

    what are the steps to reproduce this?

    practicalswift commented at 1:23 PM on January 15, 2021:

    I don't think is needed now that we've updated FuzzedDataProvider.h to the most recent version. Suppression removed! :)

    Crypt-iQ commented at 12:25 AM on January 16, 2021:

    test_runner.py on macOS, clang-11, --with-sanitizers=fuzzer,undefined,integer, commit 6d81d7aa87a15edd87175c33d1fd4a2accb3549d:

    test/fuzz/FuzzedDataProvider.h:211:49: runtime error: implicit conversion from type 'int' of value -2147483648 (32-bit, signed) to type 'unsigned long long' changed the value to 18446744071562067968 (64-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test/fuzz/FuzzedDataProvider.h:211:49 in 
test/fuzz/FuzzedDataProvider.h:211:47: runtime error: unsigned integer overflow: 2147483647 - 18446744071562067968 cannot be represented in type 'unsigned long long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test/fuzz/FuzzedDataProvider.h:211:47 in
  48. practicalswift force-pushed on Jan 15, 2021
  49. DrahtBot removed the label Needs rebase on Jan 15, 2021
  50. in src/test/fuzz/torcontrol.cpp:60 in bfdc60bbed outdated 
      55 | +        tor_control_reply.lines = ConsumeRandomLengthStringVector(fuzzed_data_provider);
  56 | +        if (tor_control_reply.lines.empty()) {
  57 | +            break;
  58 | +        }
  59 | +        DummyTorControlConnection dummy_tor_control_connection;
  60 | +        switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 3)) {
    MarcoFalke commented at 1:38 PM on January 15, 2021:

    nit: CallOneOf?

    practicalswift commented at 10:15 AM on January 16, 2021:

    Good point! Fixed!

  51. in src/test/fuzz/torcontrol.cpp:54 in bfdc60bbed outdated 
      49 | +            tor_control_reply.code = 250;
  50 | +        } else if (fuzzed_data_provider.ConsumeBool()) {
  51 | +            tor_control_reply.code = 510;
  52 | +        } else {
  53 | +            tor_control_reply.code = fuzzed_data_provider.ConsumeIntegral<int>();
  54 | +        }
    MarcoFalke commented at 1:39 PM on January 15, 2021:

    nit: Are the constants from our source code or from somewhere else? Like https://src-ref.docs.torproject.org/tor/control__cmd_8c_source.html ?

    Not sure, but if you want, you can also use CallOneOf here:

            CallOneOf (fuzzed_data_provider,
        [&] {
            tor_control_reply.code = 250;
        } ,
        [&]{
            tor_control_reply.code = 510;
        } ,
        [&]{
            tor_control_reply.code = fuzzed_data_provider.ConsumeIntegral<int>();
        });
    practicalswift commented at 10:17 AM on January 16, 2021:

    Good point. Now using CallOneOf.

    250 and 510 are from our source:

    $ git grep reply.code
src/test/fuzz/torcontrol.cpp:                tor_control_reply.code = 250;
src/test/fuzz/torcontrol.cpp:                tor_control_reply.code = 510;
src/test/fuzz/torcontrol.cpp:                tor_control_reply.code = fuzzed_data_provider.ConsumeIntegral<int>();
src/torcontrol.cpp:    if (reply.code == 250) {
src/torcontrol.cpp:    } else if (reply.code == 510) { // 510 Unrecognized command
src/torcontrol.cpp:        LogPrintf("tor: Add onion failed; error code %d\n", reply.code);
src/torcontrol.cpp:    if (reply.code == 250) {
src/torcontrol.cpp:    if (reply.code == 250) {
src/torcontrol.cpp:    if (reply.code == 250) {
  52. MarcoFalke commented at 1:39 PM on January 15, 2021: member

    haven't reviewed, but it would probably be good to use the new style here

  53. practicalswift force-pushed on Jan 16, 2021
  54. practicalswift commented at 10:18 AM on January 16, 2021: contributor

    Updated to use the excellent CallOneOf!

    Should be ready for final review :)

  55. practicalswift force-pushed on Jan 24, 2021
  56. practicalswift force-pushed on Jan 24, 2021
  57. practicalswift commented at 10:19 AM on January 26, 2021: contributor

    Compared to other fuzzing PRs this PR is relatively cumbersome to keep up to date over time due to the code move from torcontrol.cpp to torcontrol.h which must be kept in sync. Final review welcome :)

    Current status:

    Concept ACK

    ACKs

    • @Crypt-iQ: counting four now stale ACK:s - thanks for being patient! :)
  58. practicalswift commented at 3:13 PM on January 26, 2021: contributor

    The first commit which is move-only is best viewed dimmed 🦓 style:

    $ git show --color-moved=dimmed_zebra bb643af8a1bc207935e9ae2c0fe61378ee935bde
  59. practicalswift commented at 5:59 PM on February 28, 2021: contributor

    This PR has one stale ACK and three Concept ACKs, and I believe all feedback has been addressed.

    Ready for merge after re-review?

  60. laanwj commented at 10:59 AM on March 1, 2021: member

    Sorry, forgot about this a bit. I'll have a look.

    Looks like this has a silent merge conflict on current master:

    …/bitcoin/src/test/fuzz/torcontrol.cpp:38:39: error: use of undeclared identifier 'MakeFuzzingContext'
    static const auto testing_setup = MakeFuzzingContext<>();
                                      ^
1 error generated.
  61. practicalswift force-pushed on Mar 1, 2021
  62. practicalswift commented at 11:18 AM on March 1, 2021: contributor

    @laanwj

    Looks like this has a silent merge conflict on current master:

    …/bitcoin/src/test/fuzz/torcontrol.cpp:38:39: error: use of undeclared identifier 'MakeFuzzingContext'
    static const auto testing_setup = MakeFuzzingContext<>();
                                      ^
1 error generated.

    Oh, thanks for noticing!

    Now addressed :)

  63. DrahtBot added the label Needs rebase on Mar 2, 2021
  64. torcontrol: Move TorControlReply, TorControlConnection and TorController to improve testability 64219c01dc
  65. tests: Add fuzzing harness for TorController 10d4477dae
  66. practicalswift force-pushed on Mar 2, 2021
  67. DrahtBot removed the label Needs rebase on Mar 2, 2021
  68. laanwj commented at 10:39 AM on March 3, 2021: member

    I want to say this one more time before merging: I dislike moving internal implementation details to the header for testing purposes. It kind of violates encapsulation and makes the headers larger and less usefully self-documenting.

    That said, I do not see any other solution in the immediate context here. A longer-term direction would be to have test-specific headers that expose internal functionality for unit testing[, benching, fuzzing] but are never included in production code.

    ACK 10d4477dae663631411a1bd5a92e4fa941d3a96c

  69. laanwj merged this on Mar 3, 2021
  70. laanwj closed this on Mar 3, 2021
  71. sidhujag referenced this in commit f763fc3eb5 on Mar 3, 2021
  72. practicalswift deleted the branch on Apr 10, 2021
  73. DrahtBot locked this on Aug 16, 2022
Contributors
practicalswiftDrahtBotlaanwjCrypt-iQjonatackMarcoFalke
Labels
TestsP2P
Linked (view graph)
#19744 Intermittent CI failure: rpc_psbt.py --descriptors
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: 2026-05-02 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me