fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer #21000

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:fsanitize-integer changing 5 files +57 −23
  1. practicalswift commented at 8:31 PM on January 24, 2021: contributor

    Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer.

    Avoid -fsanitize=integer warnings in fuzzing harnesses.

    Suppressed warnings (excluding warnings from src/crypto/ and src/test/):

    addrman.cpp:306:24: runtime error: implicit conversion from type 'long' of value 5190149478 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 895182182 (32-bit, unsigned)
addrman.h:446:43: runtime error: implicit conversion from type 'int' of value -22 (32-bit, signed) to type 'const uint8_t' (aka 'const unsigned char') changed the value to 234 (8-bit, unsigned)
arith_uint256.cpp:32:35: runtime error: left shift of 1712128 by 24 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
arith_uint256.cpp:47:39: runtime error: left shift of 4294966784 by 31 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
chain.cpp:151:12: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551615 (64-bit, unsigned)
coins.cpp:114:22: runtime error: unsigned integer overflow: 0 - 96 cannot be represented in type 'unsigned long'
compressor.cpp:162:33: runtime error: unsigned integer overflow: 15617702637291228364 * 10 cannot be represented in type 'unsigned long'
compressor.cpp:188:11: runtime error: unsigned integer overflow: 2265760372865400000 * 10 cannot be represented in type 'unsigned long'
hash.cpp:13:15: runtime error: left shift of 1692305888 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
pubkey.h:152:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
streams.h:570:31: runtime error: left shift of 350879 by 52 places cannot be represented in type 'uint64_t' (aka 'unsigned long')
util/bip32.cpp:57:36: runtime error: left shift of 3241096244 by 1 places cannot be represented in type 'unsigned int'
util/strencodings.cpp:562:38: runtime error: implicit conversion from type 'unsigned char' of value 255 (8-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed)
util/strencodings.h:164:24: runtime error: implicit conversion from type 'int' of value -74 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551542 (64-bit, unsigned)

    The warnings above happen here:

    https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/addrman.cpp#L306 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/addrman.h#L446 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/arith_uint256.cpp#L32 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/arith_uint256.cpp#L47 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/chain.cpp#L151 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/coins.cpp#L114 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/compressor.cpp#L162 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/compressor.cpp#L188 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/hash.cpp#L13 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/pubkey.h#L152 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/streams.h#L570 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/util/bip32.cpp#L57 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/util/strencodings.cpp#L562 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/util/strencodings.h#L164

  2. laanwj added the label Tests on Jan 24, 2021
  3. practicalswift commented at 10:02 PM on January 24, 2021: contributor

    See #21001 for the src/coins.cpp case. It should probably be addressed rather than suppressed.

    Edit: See #21001 (comment).

  4. DrahtBot commented at 11:43 PM on January 24, 2021: 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:

    • #20962 (Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification by jonasschnelli)
    • #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
    • #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.

  5. MarcoFalke commented at 7:37 AM on January 25, 2021: member

    Is this supposed to be complete? If yes, could enable the sanitizer in ci. If not, it would be good to motivate the changes and explain how to test them.

    Locally it crashes soon after trying to test:

    net.cpp:183:17: runtime error: implicit conversion from type 'int64_t' (aka 'long') of value 55751376386 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4211768834 (32-bit, unsigned)
  6. practicalswift commented at 12:29 PM on January 25, 2021: contributor

    @MarcoFalke It is currently complete only with regards to the fuzz tests.

    More specifically it survives the following under -fsanitize=integer:

    $ ./fuzz_survival_test.sh 2
Running fuzzer "addition_overflow" for 2 second(s): ✅
Running fuzzer "addrdb" for 2 second(s): ✅
Running fuzzer "address_deserialize" for 2 second(s): ✅
Running fuzzer "addr_info_deserialize" for 2 second(s): ✅
Running fuzzer "addrman" for 2 second(s): ✅
…

    fuzz_survival_test.sh can be found here: https://gist.github.com/practicalswift/e3de0d96af31f42d3a2f5bcb6f631432

    I haven't gotten around to covering also the unit tests and functional tests, but it makes sense to cover them as well. I'll look in to expanding the scope of this PR to cover them too :)

  7. MarcoFalke commented at 12:34 PM on January 25, 2021: member

    I was running the process_messages harness, compiled with clang version 11.0.0 sanitizers: fuzzer,integer. Which version are you using for testing?

    Edit: The crasher for me:

    XAAAAABcXP///7z/EAAAAAAAAAAAXAAKQABcAACAYYcACgD/APP6gjtdEwAhO784//X+///9QAAA
EAAAQSoBAAr6CRhAgb/BAQAAXCcsABAAAEEqCkAAAAAAXAAKQIBBhwAKAP/z4IDgBWX/////7ztp
x/709Qh6AAAAAAAAAED3/50AAP//
  8. practicalswift force-pushed on Jan 25, 2021
  9. practicalswift force-pushed on Jan 25, 2021
  10. practicalswift commented at 8:50 PM on January 25, 2021: contributor

    @MarcoFalke Oh, I was starting from an empty seed corpus.

    I've now added all -fsanitize=integer suppressions needed for the following commands to pass:

    1. Unit tests
    2. Functional tests
    3. Fuzz tests starting with an empty corpus and running for a minute or so
    4. Fuzz tests running over the qa-assets corpus

    To reproduce:

    # Unit tests
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1:suppressions=$(pwd)/test/sanitizer_suppressions/ubsan" \
      src/test/test_bitcoin

# Functional tests
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1:suppressions=$(pwd)/test/sanitizer_suppressions/ubsan" \
      test/functional/test_runner.py --timeout-factor=0 --failfast --combinedlogslen=100

# Fuzz tests starting with an empty corpus
$ ./fuzz_survival_test.sh # From https://gist.github.com/practicalswift/e3de0d96af31f42d3a2f5bcb6f631432

# Fuzz tests running over the qa-assets corpus
# Run ./fuzz_survival_test.sh after adjusting it to use the qa-seeds corpus.
$ ./fuzz_survival_test_qa.sh
  11. fuzz: Avoid -fsanitize=integer warnings in fuzzing harnesses 58232e3ffb
  12. fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer f0f8b1a076
  13. practicalswift force-pushed on Jan 25, 2021
  14. MarcoFalke commented at 8:15 AM on January 26, 2021: member

    review ACK f0f8b1a076c362c6e26570a2129809f4d6a0abad 🤚

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK f0f8b1a076c362c6e26570a2129809f4d6a0abad 🤚
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgaOwv/Yl+4miTBnNJtB6jjBfibJcMOStYIEzi0t/LWFwEPNOViBnz4+FBYszgy
a2Zusppxw/Mt7hKB1cHmJgs9WAgS3kTXyH/c1vg043d7/qQrOBxSXu7EL/xO3sx8
NCDmXoIyMT0+wWdLFg/TGP/TeGnmmoK+UJ/i/seEl5quVaBPTauP3sdZv6tGqRaP
zY1Qq8btdAGohbp8o6Sxwp7gjfQBjoy8ggH4TheiGSpvuyvbomDcEmNu/j0E3b4d
t8g1Ef3Vcve88BN8SvRB5UN+DtpNjbsKjMjxTh8k1GOGEKgv3QdKABkNjQjsKABx
I3FbSZz0wElfIBHDZL6W7jopwhMFPf24v1dWKJyCNI1IwNE0T23EOndZxFpKqFKW
ZG5EhSY19vAkykW6NUo9KRUfHzuPvKPRBC64cvtX2AShHvltPynesIJkM2d+LTyn
2sW6Je34/oOKnWoyA2JrxlyMgJXzIqZkSFbfQAqztY8BU87sMcILEdxlV7leLdch
CZsN3rMe
=5LC+
-----END PGP SIGNATURE-----

    Timestamp of file with hash 6949a3e88bd4ef7a52797e84db8a5eea62639166a78d8c2eed13eccb4dcfd32d -

    </details>

  15. MarcoFalke commented at 8:16 AM on January 26, 2021: member

    review ACK 0d9899789e5d8f834eeb1bc67017a79bbc77530a 🎅

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 0d9899789e5d8f834eeb1bc67017a79bbc77530a 🎅
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhBggwAhZaV0ehCdKDwIh/BP4PCpM/dybM7tXzkTO+Q31MgI3deJ31J0BXs+zSi
fBv16UbSLLXMAO7BmSOjt7WO4Q8f+B138ERrR+1W6DlmR8HJ/6hZTaiQAlikQIG5
ixgzzH1PX6FdMsAbTGbIrWOv4hflIoIs2MkKjXnKRKighD62viF8KFrzZSUOwls8
EYCxo/mcLE9uPWsccnUWM6ElpBwLwXdloEIDz9+k1X26VHVQk5Hb44dLOjFSIJld
vSZhRVgt8o9ZDaWV2F2Qi64A7pQAkvZ/fXjommAbeBZtzrh95r+WRECzXWjVnjrA
KtwcI9rYfbzePawFtxN4RHQab5VDxqUU1Og7ZG7c0ro6sqL8sM3eianvZaRnCJb1
EkR9k/yicw//S9XKXyNX/xVjBe/ead9IQdzUCLYbLqfbhj95nZxSzF47iAtJsnV0
m26U/s8d6zBtSsGFqk1q3Qa/58U4EFqrs05OTItaFsJ65lO3nFEDp0nroetojw+M
dWk7V6vD
=8Lkf
-----END PGP SIGNATURE-----

    Timestamp of file with hash a8d5520f601c503b2ecc40e4169f8355ed84571b1827d52455af1e4256107a2b -

    </details>

  16. MarcoFalke force-pushed on Jan 26, 2021
  17. MarcoFalke commented at 8:42 AM on January 26, 2021: member

    Force pushed to remove unrelated commit. Hope you don't mind @practicalswift

  18. MarcoFalke commented at 8:44 AM on January 26, 2021: member

    Only change was in the linter, which passed. So we don't need to wait for the other ci checks before merge.

  19. MarcoFalke merged this on Jan 26, 2021
  20. MarcoFalke closed this on Jan 26, 2021
  21. sidhujag referenced this in commit 2c90d0a335 on Jan 26, 2021
  22. practicalswift deleted the branch on Apr 10, 2021
  23. DrahtBot locked this on Aug 16, 2022
Contributors
practicalswiftDrahtBotMarcoFalke
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: 2026-05-02 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me