tests: Add fuzzing harness for Bech32 encoding/decoding #17357

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:fuzzers-bech32 changing 5 files +92 −17
  1. practicalswift commented at 0:06 am on November 3, 2019: contributor

    Add fuzzing harness for Bech32 encoding/decoding.

    Testing this PR

    Run:

    0$ make distclean
    1$ ./autogen.sh
    2$ CC=clang CXX=clang++ ./configure --enable-fuzz \
    3      --with-sanitizers=address,fuzzer,undefined
    4$ make
    5$ src/test/fuzz/bech32 -max_total_time=60
    6
  2. fanquake added the label Tests on Nov 3, 2019
  3. DrahtBot commented at 2:13 am on November 3, 2019: 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:

    • #17229 (tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions by practicalswift)
    • #17071 (tests: Add fuzzing harness for CheckBlock(…) and other CBlock related functions 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.

  4. practicalswift force-pushed on Nov 3, 2019
  5. practicalswift force-pushed on Nov 3, 2019
  6. jonatack commented at 11:19 am on November 4, 2019: member

    When I try to test this, make fails with:

    0CXXLD    test/fuzz/address_deserialize
    1/usr/bin/ld: libbitcoin_util.a(libbitcoin_util_a-threadinterrupt.o): in function `UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::~UniqueLock()':
    2/home/jon/projects/bitcoin/bitcoin/src/./sync.h:168: undefined reference to `LeaveCritical()'
    3clang: error: linker command failed with exit code 1 (use -v to see invocation)
    

    Full configure/make gist here.

    I have never been able to get the fuzzing to run when following the info in doc/fuzzing.md, so this may be unrelated to the PR. Any tips would be welcome.

  7. MarcoFalke commented at 12:53 pm on November 4, 2019: member
    Have you tried a make distclean before ./configure?
  8. practicalswift commented at 1:22 pm on November 4, 2019: contributor

    @jonatack What @MarcoFalke said :)

    I’ve now updated OP with make distclean; ./autogen.sh. Will use that in the “how to test this PR template” in future OP:s for fuzzing PR:s.

    0$ make distclean
    1$ ./autogen.sh
    2$ CC=clang CXX=clang++ ./configure --enable-fuzz \
    3      --with-sanitizers=address,fuzzer,undefined
    4$ make
    5$ src/test/fuzz/bech32
    6
  9. jonatack commented at 8:03 pm on November 4, 2019: member

    It looks like that solved it. Thanks!

    How long does the test run – until it’s interrupted?

     0$ src/test/fuzz/bech32
     1INFO: Seed: 2164764235
     2INFO: Loaded 1 modules   (506453 inline 8-bit counters): 506453 [0x559733d74988, 0x559733df03dd), 
     3INFO: Loaded 1 PC tables (506453 PCs): 506453 [0x559733df03e0,0x5597345aa930), 
     4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
     5INFO: A corpus is not provided, starting from an empty corpus
     6[#2](/bitcoin-bitcoin/2/)	INITED cov: 740 ft: 741 corp: 1/1b lim: 4 exec/s: 0 rss: 107Mb
     7[#4](/bitcoin-bitcoin/4/)	NEW    cov: 743 ft: 853 corp: 2/3b lim: 4 exec/s: 0 rss: 107Mb L: 2/2 MS: 2 ShuffleBytes-CopyPart-
     8...
     9[#4005097](/bitcoin-bitcoin/4005097/)	REDUCE cov: 829 ft: 2420 corp: 213/17Kb lim: 3381 exec/s: 2673 rss: 734Mb L: 44/641 MS: 3 CopyPart-EraseBytes-CopyPart-
    10[#4132416](/bitcoin-bitcoin/4132416/)	REDUCE cov: 829 ft: 2420 corp: 213/17Kb lim: 3502 exec/s: 2640 rss: 734Mb L: 7/641 MS: 4 ChangeBit-ChangeBit-ShuffleBytes-EraseBytes-
    

    Concept ACK, initial code review looks good.

  10. practicalswift commented at 8:12 pm on November 4, 2019: contributor

    @jonasnick Yes, libFuzzer runs until interrupted :)

    Pass -max_total_time=60 to run the fuzzer during 60 seconds, and -help=1 to list all other libFuzzer options.

    Recommended setup:

    0$ mkdir -p generated-test-corpus/
    1$ src/test/fuzz/bech32 -max_total_time=60 generated-test-corpus/
    2$ # now go into generated-test-corpus/ and investigate the generated test corpus :)
    

    You might want to look at the libFuzzer documentation too if you haven’t done that already :)

  11. jonasnick commented at 8:19 pm on November 4, 2019: contributor
    Thank you @practicalswift, I almost lost my mind trying to figure out libFuzzer today.
  12. practicalswift commented at 8:25 pm on November 4, 2019: contributor
    Sorry - wrong Jonas/Jon. Was meaning to mention @jonatack :)
  13. in src/util/strencodings.h:261 in c089b428fd outdated
    257@@ -258,4 +258,6 @@ std::string ToUpper(const std::string& str);
    258  */
    259 std::string Capitalize(std::string str);
    260 
    261+bool CaseInsensitiveEqual(const std::string& s1, const std::string& s2);
    


    MarcoFalke commented at 8:35 pm on November 4, 2019:
    This is only used in tests? Could move to src/test/util/str?
  14. in test/fuzz/test_runner.py:18 in c089b428fd outdated
    11@@ -12,6 +12,10 @@
    12 import subprocess
    13 import logging
    14 
    15+# Fuzzers known to lack a seed corpus in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus
    16+FUZZERS_MISSING_CORPORA = [
    17+    "bech32",
    18+]
    


    MarcoFalke commented at 9:03 pm on November 4, 2019:
    could remove?
  15. practicalswift force-pushed on Nov 5, 2019
  16. tests: Move CaseInsensitiveEqual to test/util/str 85a34b1683
  17. tests: Add fuzzing harness for Bech32 encoding/decoding b7541705d0
  18. practicalswift commented at 11:45 am on November 5, 2019: contributor
    @MarcoFalke Feedback addressed! Please re-review :)
  19. practicalswift force-pushed on Nov 5, 2019
  20. in src/test/fuzz/bech32.cpp:14 in b7541705d0
     9+
    10+#include <cassert>
    11+#include <cstdint>
    12+#include <string>
    13+#include <utility>
    14+#include <vector>
    


    jonatack commented at 3:10 pm on November 5, 2019:

    Can remove?

    0+#include <cstdint>
    1+#include <string>
    2+#include <utility>
    3+#include <vector>
    

    practicalswift commented at 4:33 pm on November 5, 2019:

    We are supposed to include what we use according to the developer notes :)

    Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

    Rationale: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.


    jonatack commented at 4:56 pm on November 5, 2019:
    Oh right. Thanks for the reminder :)
  21. jonatack commented at 3:26 pm on November 5, 2019: member

    ACK b7541705d0abfbddf682a0134f3fa8a8e1d06cdf

    This fuzz test afaict verifies round-trip through bech32 Decode + Encode, comparing the initial random_string with the reencoded value, then Encode + Decode and asserts the correct values for the hrp (human-readable part as a string) and data.

    Tested as per PR description, read code, ran tests.

     0((HEAD detached at origin/pr/17357))$ src/test/fuzz/bech32 -max_total_time=60
     1INFO: Seed: 3403448425
     2INFO: Loaded 1 modules   (506466 inline 8-bit counters): 506466 [0x5606fb3bc9e8, 0x5606fb43844a), 
     3INFO: Loaded 1 PC tables (506466 PCs): 506466 [0x5606fb438450,0x5606fbbf2a70), 
     4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
     5INFO: A corpus is not provided, starting from an empty corpus
     6[#2](/bitcoin-bitcoin/2/)	INITED cov: 746 ft: 747 corp: 1/1b lim: 4 exec/s: 0 rss: 106Mb
     7.../...
     8[#316157](/bitcoin-bitcoin/316157/)	DONE   cov: 835 ft: 2274 corp: 152/4114b lim: 122 exec/s: 5182 rss: 587Mb
     9###### Recommended dictionary. ######
    10"\xfd\xff\xff\xff" # Uses: 22198
    11"\x00\x00\x00\x00\x00\x00\x00\x00" # Uses: 3159
    12###### End of recommended dictionary. ######
    13Done 316157 runs in 61 second(s)
    
  22. MarcoFalke referenced this in commit 50591f6ec6 on Nov 5, 2019
  23. MarcoFalke merged this on Nov 5, 2019
  24. MarcoFalke closed this on Nov 5, 2019

  25. jonatack commented at 12:56 pm on November 6, 2019: member

    I’m seeing appveyor failures:

    0bech32_tests.obj : error LNK2001: unresolved external symbol "bool __cdecl CaseInsensitiveEqual
    
  26. ryanofsky commented at 1:42 pm on November 6, 2019: member

    I’m seeing appveyor failures:

    Appveyor error is presumably caused by this PR and fixed by #17384 (comment)

  27. sidhujag referenced this in commit c41a998f82 on Nov 7, 2019
  28. jasonbcox referenced this in commit 842263b6a9 on Jul 20, 2020
  29. jasonbcox referenced this in commit bd65ad08d2 on Jul 20, 2020
  30. sidhujag referenced this in commit f9a2c3fdc5 on Nov 10, 2020
  31. practicalswift deleted the branch on Apr 10, 2021
  32. kittywhiskers referenced this in commit 6cea477a21 on Feb 25, 2022
  33. kittywhiskers referenced this in commit 52e62b6601 on Feb 26, 2022
  34. PastaPastaPasta referenced this in commit 6af24e87c4 on Feb 26, 2022
  35. PastaPastaPasta referenced this in commit fde9e3fc05 on Mar 3, 2022
  36. DrahtBot locked this on Aug 16, 2022

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-11-17 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me