tests: fix -fsanitize=integer complaints #19713

pull Crypt-iQ wants to merge 4 commits into bitcoin:master from Crypt-iQ:fuzz_supp_0813 changing 5 files +24 −8
  1. Crypt-iQ commented at 5:51 pm on August 13, 2020: contributor

    First steps towards running fuzz tests with -fsanitize=integer and not having it fail. This PR fixes the complaints in the src/test/fuzz directory only. There are other complaints that are not handled because I thought the scope of the PR would sprawl.

    c0f40ba767dcbcd89223eb7f7678d8d4263a9f1a fixes

    0SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change test/fuzz/FuzzedDataProvider.h:108:27 in
1test/fuzz/FuzzedDataProvider.h:87:51
2
3SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow test/fuzz/FuzzedDataProvider.h:108:31 in
4test/fuzz/FuzzedDataProvider.h:108:31
5
6SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change /usr/local/opt/llvm/bin/../include/c++/v1/memory:1876:35

    03826e53ba292ae2434acd21424e82adf40804dc fixes

    0SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow test/fuzz/FuzzedDataProvider.h:108:31 in
1test/fuzz/util.h:166:35
2
3SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change test/fuzz/FuzzedDataProvider.h:108:27 in
4test/fuzz/script.cpp:145:37

    febe0f8a3c3c469a0d65c2d1f35eda1e04f9bcf1 fixes

    0SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change test/fuzz/FuzzedDataProvider.h:108:27 in
1test/fuzz/crypto_chacha20_poly1305_aead.cpp:63:25

    d698eadfac153d426b10c730b95eed2c06373baf fixes

    0test/fuzz/pow.cpp:46:39: runtime error: implicit conversion from type 'long' of value 4294967712 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int')      changed the value to 416 (32-bit, unsigned)
  2. DrahtBot added the label Tests on Aug 13, 2020
  3. fanquake requested review from practicalswift on Aug 13, 2020
  4. practicalswift approved
  5. practicalswift commented at 3:39 pm on August 14, 2020: contributor

    Tested ACK 8fdf71cba92d45444f67d28c9f2cf1705fc5682e

    Thanks a lot for improving our fuzzing harness @Crypt-iQ. Love it!

  6. practicalswift commented at 3:43 pm on August 14, 2020: contributor

    @Crypt-iQ While you’re at it touching crypto_chacha20_poly1305_aead.cpp, would you mind fixing this one too? :)

    0test/fuzz/crypto_chacha20_poly1305_aead.cpp:50:27: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'

    (Line 50 is after applying this patch.)

    Base64 encoded test case:

    0AOcDAAAKI0q6uhoA/wAKf////xUVFf//AAAAAABKFRUVFRUVFQoVFRUVFRUVFRUVFRUbBhUVFQAVFUkAAAAOABoA/wAKf/////8B///////////////////k/////////0AbBg==
  7. Crypt-iQ force-pushed on Aug 15, 2020
  8. Crypt-iQ requested review from practicalswift on Aug 15, 2020
  9. Crypt-iQ commented at 10:18 pm on August 15, 2020: contributor

    There is one other complaint in the fuzz directory: https://github.com/bitcoin/bitcoin/blob/3ab2582c7fe76d2839ab493512758d5601903c86/src/test/fuzz/pow.cpp#L45-L47

    0test/fuzz/pow.cpp:46:39: runtime error: implicit conversion from type 'long' of value 4294967712 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int')      changed the value to 416 (32-bit, unsigned)

    Not entirely sure what value to set current_block.nTime to in that scenario. Also it can overflow via multiplication or addition.

  10. practicalswift commented at 7:48 am on August 17, 2020: contributor

    Not entirely sure what value to set current_block.nTime to in that scenario. Also it can overflow via multiplication or addition.

    Perhaps you can use MultiplicationOverflow and AdditionOverflow (both in test/fuzz/util.h) to guard against the overflows? :)

  11. Crypt-iQ force-pushed on Aug 18, 2020
  12. Crypt-iQ force-pushed on Aug 18, 2020
  13. Crypt-iQ commented at 11:46 am on August 18, 2020: contributor

    Get this complaint on macOS Catalina v10.15.4 with clang version 10.0.1 (installed via brew install llvm):

     0/usr/local/Cellar/llvm/10.0.1/bin/../include/c++/v1/memory:1876:35: runtime error: implicit conversion from type 'char' of value -41 (8-bit, signed) to type 'unsigned char' changed the value to 215 (8-bit, unsigned)
 1    [#0](/bitcoin-bitcoin/0/) 0x1075d43ff in std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >::vector<std::__1::__wrap_iter<char const*> >(std::__1::__wrap_iter<char const*>, std::__1::enable_if<(__is_cpp17_forward_iterator<std::__1::__wrap_iter<char const*> >::value) && (is_constructible<unsigned char, std::__1::iterator_traits<std::__1::__wrap_iter<char const*> >::reference>::value), std::__1::__wrap_iter<char const*> >::type) vector:1223
 2    [#1](/bitcoin-bitcoin/1/) 0x1075d3656 in std::__1::optional<CBlockHeader> ConsumeDeserializable<CBlockHeader>(FuzzedDataProvider&, unsigned long) util.h:70
 3    [#2](/bitcoin-bitcoin/2/) 0x1075d1867 in test_one_input(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) pow.cpp:31
 4    [#3](/bitcoin-bitcoin/3/) 0x107b13429 in LLVMFuzzerTestOneInput fuzz.cpp:45
 5    [#4](/bitcoin-bitcoin/4/) 0x107be4d00 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:556
 6    [#5](/bitcoin-bitcoin/5/) 0x107be4445 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) FuzzerLoop.cpp:470
 7    [#6](/bitcoin-bitcoin/6/) 0x107be6ae7 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:765
 8    [#7](/bitcoin-bitcoin/7/) 0x107be6e49 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:792
 9    [#8](/bitcoin-bitcoin/8/) 0x107bd413d in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:829
10    [#9](/bitcoin-bitcoin/9/) 0x107c00572 in main FuzzerMain.cpp:19
11    [#10](/bitcoin-bitcoin/10/) 0x7fff7114bcc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

    Can’t reproduce on my ubuntu digitalocean box with clang-10 and am wondering if anybody else with a Mac can reproduce.

  14. adaminsky commented at 5:25 pm on August 19, 2020: contributor
    I can reproduce this on macOS 10.15.5 with clang installed from brew install llvm.
  15. Crypt-iQ commented at 0:50 am on August 20, 2020: contributor
    @adaminsky what are the options you pass to configure?
  16. adaminsky commented at 1:35 am on August 20, 2020: contributor
    @Crypt-iQ I used ./configure --enable-fuzz --with-sanitizers=fuzzer,integer CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++ --disable-asm.
  17. DrahtBot commented at 8:34 pm on August 20, 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:

    • #20828 (fuzz: Introduce CallOneOf helper to replace switch-case by MarcoFalke)
    • #17791 (Remove UBSan suppressions for CTxMemPool* by hebasto)

    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.

  18. Crypt-iQ commented at 5:59 am on August 22, 2020: contributor

    @adaminsky If you also run with:

    0UBSAN_OPTIONS="suppressions=test/sanitizer_suppressions/ubsan:print_stacktrace=1:report_error_type=1" src/test/fuzz/pow ~/qa-assets/fuzz_seed_corpus/pow

    Do you also get the following complaint?

     0/usr/local/opt/llvm/bin/../include/c++/v1/memory:1876:35: runtime error: implicit conversion from type 'unsigned char' of value 215 (8-bit, unsigned) to type 'char' changed the value to -41 (8-bit, signed)
 1    [#0](/bitcoin-bitcoin/0/) 0x1029da360 in void std::__1::allocator_traits<zero_after_free_allocator<char> >::__construct<char, unsigned char const&>(std::__1::integral_constant<bool, true>, zero_after_free_allocator<char>&, char*, unsigned char const&) memory:1768
 2    [#1](/bitcoin-bitcoin/1/) 0x1029d9e95 in void std::__1::allocator_traits<zero_after_free_allocator<char> >::__construct_range_forward<std::__1::__wrap_iter<unsigned char const*>, char*>(zero_after_free_allocator<char>&, std::__1::__wrap_iter<unsigned char const*>, std::__1::__wrap_iter<unsigned char const*>, char*&) memory:1688
 3    [#2](/bitcoin-bitcoin/2/) 0x1029d8a78 in std::__1::enable_if<__is_cpp17_forward_iterator<std::__1::__wrap_iter<unsigned char const*> >::value, void>::type std::__1::vector<char, zero_after_free_allocator<char> >::__construct_at_end<std::__1::__wrap_iter<unsigned char const*> >(std::__1::__wrap_iter<unsigned char const*>, std::__1::__wrap_iter<unsigned char const*>, unsigned long) vector:1076
 4    [#3](/bitcoin-bitcoin/3/) 0x1029d8642 in std::__1::vector<char, zero_after_free_allocator<char> >::vector<std::__1::__wrap_iter<unsigned char const*> >(std::__1::__wrap_iter<unsigned char const*>, std::__1::enable_if<(__is_cpp17_forward_iterator<std::__1::__wrap_iter<unsigned char const*> >::value) && (is_constructible<char, std::__1::iterator_traits<std::__1::__wrap_iter<unsigned char const*> >::reference>::value), std::__1::__wrap_iter<unsigned char const*> >::type) vector:1223
 5    [#4](/bitcoin-bitcoin/4/) 0x1029d830f in CDataStream::CDataStream(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&, int, int) streams.h:247
 6    [#5](/bitcoin-bitcoin/5/) 0x1029ccaf2 in std::__1::optional<CBlockHeader> ConsumeDeserializable<CBlockHeader>(FuzzedDataProvider&, unsigned long) util.h:71
 7    [#6](/bitcoin-bitcoin/6/) 0x1029cbb1a in test_one_input(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) pow.cpp:31
 8    [#7](/bitcoin-bitcoin/7/) 0x103808f30 in LLVMFuzzerTestOneInput fuzz.cpp:45
 9    [#8](/bitcoin-bitcoin/8/) 0x1039e10d0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:556
10    [#9](/bitcoin-bitcoin/9/) 0x1039e0815 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) FuzzerLoop.cpp:470
11    [#10](/bitcoin-bitcoin/10/) 0x1039e2eb7 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:765
12    [#11](/bitcoin-bitcoin/11/) 0x1039e3219 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:792
13    [#12](/bitcoin-bitcoin/12/) 0x1039d06dd in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:829
14    [#13](/bitcoin-bitcoin/13/) 0x1039fc3b2 in main FuzzerMain.cpp:19
15    [#14](/bitcoin-bitcoin/14/) 0x7fff73778cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

    It hits util.h:71 instead of util.h:70

  19. Crypt-iQ commented at 12:12 pm on August 22, 2020: contributor
    These two complaints happen because char is signed by default with our Mac setups. Ubuntu box reports no errors.
  20. Crypt-iQ commented at 2:20 pm on August 23, 2020: contributor

    The first complaint happens because of the conversion from std::string (which is std::basic_string<char>) to std::vector<uint8_t> here: https://github.com/bitcoin/bitcoin/blob/197450f80868fe752c6107955e5da80704212b34/src/test/fuzz/util.h#L35-L39

    The second complaint happens because a std::vector<uint8_t> is passed into CDataStream here: https://github.com/bitcoin/bitcoin/blob/197450f80868fe752c6107955e5da80704212b34/src/test/fuzz/util.h#L70-L71

    and CDataStream uses a byte-vector of char: https://github.com/bitcoin/bitcoin/blob/78dae8caccd82cfbfd76557f1fb7d7557c7b5edb/src/support/allocators/zeroafterfree.h#L45-L46

    Curious as to your thoughts about these @practicalswift. In the first example, perhaps ConsumeRandomLengthString could return std::basic_string<unsigned char>. Haven’t figured out what to do about the second example.

  21. adaminsky commented at 7:21 am on August 24, 2020: contributor
    @Crypt-iQ Confirming that I can reproduce both complaints and that char is signed for me.
  22. darosior changes_requested
  23. darosior commented at 10:16 am on August 24, 2020: member

    Concept ACK.

    In the first example, perhaps ConsumeRandomLengthString could return std::basic_string. Haven’t figured out what to do about the second example.

    ConsumeRandomLengthString is part of LLVM’s header i don’t think it’s a good idea to modify it.

    Regarding the second complaint, you could directly consume a string without the cast to uint8_t from ConsumeRandomLengthByteVector:

     0index ae47a2805..fb89dc713 100644
 1--- a/src/test/fuzz/util.h
 2+++ b/src/test/fuzz/util.h
 3@@ -38,9 +38,15 @@ NODISCARD inline std::vector<uint8_t> ConsumeRandomLengthByteVector(FuzzedDataPr
 4     return {s.begin(), s.end()};
 5 }
 6 
 7+NODISCARD inline std::vector<char> ConsumeRandomLengthCharVector(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096) noexcept
 8+{
 9+    const std::string s = fuzzed_data_provider.ConsumeRandomLengthString(max_length);
10+    return {s.begin(), s.end()};
11+}
12+
13 NODISCARD inline CDataStream ConsumeDataStream(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096) noexcept
14 {
15-    return {ConsumeRandomLengthByteVector(fuzzed_data_provider, max_length), SER_NETWORK, INIT_PROTO_VERSION};
16+    return {ConsumeRandomLengthCharVector(fuzzed_data_provider, max_length), SER_NETWORK, INIT_PROTO_VERSION};
17 }
18 
19 NODISCARD inline std::vector<std::string> ConsumeRandomLengthStringVector(FuzzedDataProvider& fuzzed_data_provider, const size_t max_vector_size = 16, const size_t max_string_length = 16) noexcept

    Regarding the first complaint, ConsumeBytes:

    0// Returns a std::vector containing |num_bytes| of input data. If fewer than
1  // |num_bytes| of data remain, returns a shorter std::vector containing all
2  // of the data that's left. Can be used with any byte sized type, such as
3  // char, unsigned char, uint8_t, etc.
4  template <typename T> std::vector<T> ConsumeBytes(size_t num_bytes) {
5    num_bytes = std::min(num_bytes, remaining_bytes_);
6    return ConsumeBytes<T>(num_bytes, num_bytes);
7  }

    Is almost what you want, as it’s just missing the “random length” part. Maybe our ConsumeRandomLengthByteVector could use a combination of this one combined with ConsumeRandomLengthString’s randomness logic ?

  24. Crypt-iQ force-pushed on Aug 28, 2020
  25. Crypt-iQ force-pushed on Aug 28, 2020
  26. in src/test/fuzz/crypto_chacha20_poly1305_aead.cpp:50 in e677639523 outdated 
    46@@ -47,20 +47,24 @@ void test_one_input(const std::vector<uint8_t>& buffer)
47             break;
48         }
49         case 3: {
50-            seqnr_payload += 1;
51+            if (!AdditionOverflow(seqnr_payload, static_cast<uint64_t>(1))) {
    practicalswift commented at 5:40 am on August 29, 2020:
    In the case of wrap-around I think we want to set seqnr_payload = 0 emulating how it used to work prior to this change (but without the UBSan complaint since we’re explicitly opting in to the wrap-around).
  27. in src/test/fuzz/crypto_chacha20_poly1305_aead.cpp:56 in e677639523 outdated 
    53+            }
54             aad_pos += CHACHA20_POLY1305_AEAD_AAD_LEN;
55             if (aad_pos + CHACHA20_POLY1305_AEAD_AAD_LEN > CHACHA20_ROUND_OUTPUT) {
56                 aad_pos = 0;
57-                seqnr_aad += 1;
58+                if (!AdditionOverflow(seqnr_aad, static_cast<uint64_t>(1))) {
    practicalswift commented at 5:40 am on August 29, 2020:
    Same here.
  28. in src/test/fuzz/util.h:90 in e677639523 outdated 
    86@@ -67,7 +87,7 @@ NODISCARD inline std::vector<T> ConsumeRandomLengthIntegralVector(FuzzedDataProv
87 template <typename T>
88 NODISCARD inline std::optional<T> ConsumeDeserializable(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096) noexcept
89 {
90-    const std::vector<uint8_t> buffer = ConsumeRandomLengthByteVector(fuzzed_data_provider, max_length);
91+    const std::vector<char> buffer = ConsumeRandomLengthCharVector(fuzzed_data_provider, max_length);
    practicalswift commented at 5:46 am on August 29, 2020:

    Doing this will make the coverage achieved by running the seed corpus depend on if char is signed or not.

    I don’t think that is cost worth paying to suppress wrap-around warnings. I suggest adding a test/sanitizer_suppressions/ubsan suppression instead :)

  29. in src/test/fuzz/util.h:37 in e677639523 outdated 
    32@@ -33,6 +33,26 @@
33 #include <vector>
34 
35 NODISCARD inline std::vector<uint8_t> ConsumeRandomLengthByteVector(FuzzedDataProvider& fuzzed_data_provider, const size_t max_length = 4096) noexcept
36+{
37+    std::vector<uint8_t> result;
    practicalswift commented at 5:48 am on August 29, 2020:

    I’m afraid changing ConsumeRandomLengthByteVector this way will invalidate quite a few seeds in our seed corpus.

    I don’t think that is cost worth paying to suppress wrap-around warnings. I suggest adding a test/sanitizer_suppressions/ubsan suppression instead and leave ConsumeRandomLengthByteVector unchanged :)

    Crypt-iQ commented at 5:27 am on September 1, 2020:
    But if a suppression is added for */include/c++/*/memory.h then other legitimate complaints will also be suppressed. As a temporary measure it works, but I think eventually it would be ideal to have the least amount of suppressions possible?
    Crypt-iQ commented at 7:46 pm on September 1, 2020:
    Suppression added.
  30. practicalswift commented at 6:00 am on August 29, 2020: contributor

    Some additional review :)

    Since -fsanitize=integer’s unsigned-integer-overflow and implicit-integer-sign-change are not UB (but could be indications of bugs) I think it makes sense to be somewhat liberal with tackling these cases by adding suppressions instead of harness changes. Rationales given in each comment.

  31. Crypt-iQ force-pushed on Sep 1, 2020
  32. tests: add unsigned-integer-overflow, implicit-integer-sign-change suppressions 
    This is a first step towards getting -fsanitize=integer to not report
complaints when fuzzing. Without this, the implicit-integer-sign-change
and unsigned-integer-overflow checks are triggered for pretty much
every fuzz target. A suppression is added for memory* since macOS
builds can trigger implicit-integer-sign-change when using either
ConsumeRandomLengthByteVector or ConsumeDeserializable.
    c0f40ba767
  33. tests: use unsigned int instead of int for WitnessUnknown version 03826e53ba
  34. tests: seqnr_{aad,payload} populated via ConsumeIntegral<uint64_t> 
    This commit also uses AdditionOverflow to prevent overflow.
    febe0f8a3c
  35. tests: prevent overflow by addition, multiplication in pow.cpp d698eadfac
  36. Crypt-iQ force-pushed on Sep 1, 2020
  37. Crypt-iQ renamed this:
    fuzz: fix -fsanitize=integer complaints
    tests: fix -fsanitize=integer complaints
    on Sep 1, 2020
  38. Crypt-iQ requested review from practicalswift on Sep 1, 2020
  39. practicalswift approved
  40. practicalswift commented at 7:38 pm on September 1, 2020: contributor
    ACK d698eadfac153d426b10c730b95eed2c06373baf – patch looks correct
  41. DrahtBot added the label Needs rebase on Jan 14, 2021
  42. DrahtBot commented at 10:17 am on January 14, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  43. Crypt-iQ commented at 3:46 pm on February 14, 2021: contributor
    Closed by #21000
  44. Crypt-iQ closed this on Feb 14, 2021
  45. Crypt-iQ deleted the branch on Feb 14, 2021
  46. fanquake removed the label Needs rebase on May 31, 2021
  47. DrahtBot locked this on Aug 18, 2022


Crypt-iQ practicalswift adaminsky DrahtBot darosior

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