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

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/):

 0addrman.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)
 1addrman.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)
 2arith_uint256.cpp:32:35: runtime error: left shift of 1712128 by 24 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
 3arith_uint256.cpp:47:39: runtime error: left shift of 4294966784 by 31 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
 4chain.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)
 5coins.cpp:114:22: runtime error: unsigned integer overflow: 0 - 96 cannot be represented in type 'unsigned long'
 6compressor.cpp:162:33: runtime error: unsigned integer overflow: 15617702637291228364 * 10 cannot be represented in type 'unsigned long'
 7compressor.cpp:188:11: runtime error: unsigned integer overflow: 2265760372865400000 * 10 cannot be represented in type 'unsigned long'
 8hash.cpp:13:15: runtime error: left shift of 1692305888 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
 9pubkey.h:152:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
10streams.h:570:31: runtime error: left shift of 350879 by 52 places cannot be represented in type 'uint64_t' (aka 'unsigned long')
11util/bip32.cpp:57:36: runtime error: left shift of 3241096244 by 1 places cannot be represented in type 'unsigned int'
12util/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)
13util/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:

laanwj added the label Tests on Jan 24, 2021

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).

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

#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.

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:

0net.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)

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:

0$ ./fuzz_survival_test.sh 2
1Running fuzzer "addition_overflow" for 2 second(s): ✅
2Running fuzzer "addrdb" for 2 second(s): ✅
3Running fuzzer "address_deserialize" for 2 second(s): ✅
4Running fuzzer "addr_info_deserialize" for 2 second(s): ✅
5Running fuzzer "addrman" for 2 second(s): ✅
6…

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 :)

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:

0XAAAAABcXP///7z/EAAAAAAAAAAAXAAKQABcAACAYYcACgD/APP6gjtdEwAhO784//X+///9QAAA
1EAAAQSoBAAr6CRhAgb/BAQAAXCcsABAAAEEqCkAAAAAAXAAKQIBBhwAKAP/z4IDgBWX/////7ztp
2x/709Qh6AAAAAAAAAED3/50AAP//

practicalswift force-pushed on Jan 25, 2021

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:

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

To reproduce:

 0# Unit tests
 1$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1:suppressions=$(pwd)/test/sanitizer_suppressions/ubsan" \
 2      src/test/test_bitcoin
 3
 4# Functional tests
 5$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1:suppressions=$(pwd)/test/sanitizer_suppressions/ubsan" \
 6      test/functional/test_runner.py --timeout-factor=0 --failfast --combinedlogslen=100
 7
 8# Fuzz tests starting with an empty corpus
 9$ ./fuzz_survival_test.sh # From https://gist.github.com/practicalswift/e3de0d96af31f42d3a2f5bcb6f631432
10
11# Fuzz tests running over the qa-assets corpus
12# Run ./fuzz_survival_test.sh after adjusting it to use the qa-seeds corpus.
13$ ./fuzz_survival_test_qa.sh

fuzz: Avoid -fsanitize=integer warnings in fuzzing harnesses 58232e3ffb

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

practicalswift force-pushed on Jan 25, 2021

MarcoFalke commented at 8:15 am on January 26, 2021: member

review ACK f0f8b1a076c362c6e26570a2129809f4d6a0abad 🤚

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK f0f8b1a076c362c6e26570a2129809f4d6a0abad 🤚
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgaOwv/Yl+4miTBnNJtB6jjBfibJcMOStYIEzi0t/LWFwEPNOViBnz4+FBYszgy
 8a2Zusppxw/Mt7hKB1cHmJgs9WAgS3kTXyH/c1vg043d7/qQrOBxSXu7EL/xO3sx8
 9NCDmXoIyMT0+wWdLFg/TGP/TeGnmmoK+UJ/i/seEl5quVaBPTauP3sdZv6tGqRaP
10zY1Qq8btdAGohbp8o6Sxwp7gjfQBjoy8ggH4TheiGSpvuyvbomDcEmNu/j0E3b4d
11t8g1Ef3Vcve88BN8SvRB5UN+DtpNjbsKjMjxTh8k1GOGEKgv3QdKABkNjQjsKABx
12I3FbSZz0wElfIBHDZL6W7jopwhMFPf24v1dWKJyCNI1IwNE0T23EOndZxFpKqFKW
13ZG5EhSY19vAkykW6NUo9KRUfHzuPvKPRBC64cvtX2AShHvltPynesIJkM2d+LTyn
142sW6Je34/oOKnWoyA2JrxlyMgJXzIqZkSFbfQAqztY8BU87sMcILEdxlV7leLdch
15CZsN3rMe
16=5LC+
17-----END PGP SIGNATURE-----

Timestamp of file with hash 6949a3e88bd4ef7a52797e84db8a5eea62639166a78d8c2eed13eccb4dcfd32d -

MarcoFalke commented at 8:16 am on January 26, 2021: member

review ACK 0d9899789e5d8f834eeb1bc67017a79bbc77530a 🎅

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 0d9899789e5d8f834eeb1bc67017a79bbc77530a 🎅
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhBggwAhZaV0ehCdKDwIh/BP4PCpM/dybM7tXzkTO+Q31MgI3deJ31J0BXs+zSi
 8fBv16UbSLLXMAO7BmSOjt7WO4Q8f+B138ERrR+1W6DlmR8HJ/6hZTaiQAlikQIG5
 9ixgzzH1PX6FdMsAbTGbIrWOv4hflIoIs2MkKjXnKRKighD62viF8KFrzZSUOwls8
10EYCxo/mcLE9uPWsccnUWM6ElpBwLwXdloEIDz9+k1X26VHVQk5Hb44dLOjFSIJld
11vSZhRVgt8o9ZDaWV2F2Qi64A7pQAkvZ/fXjommAbeBZtzrh95r+WRECzXWjVnjrA
12KtwcI9rYfbzePawFtxN4RHQab5VDxqUU1Og7ZG7c0ro6sqL8sM3eianvZaRnCJb1
13EkR9k/yicw//S9XKXyNX/xVjBe/ead9IQdzUCLYbLqfbhj95nZxSzF47iAtJsnV0
14m26U/s8d6zBtSsGFqk1q3Qa/58U4EFqrs05OTItaFsJ65lO3nFEDp0nroetojw+M
15dWk7V6vD
16=8Lkf
17-----END PGP SIGNATURE-----

Timestamp of file with hash a8d5520f601c503b2ecc40e4169f8355ed84571b1827d52455af1e4256107a2b -

MarcoFalke force-pushed on Jan 26, 2021

MarcoFalke commented at 8:42 am on January 26, 2021: member

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

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.

MarcoFalke merged this on Jan 26, 2021

MarcoFalke closed this on Jan 26, 2021

sidhujag referenced this in commit 2c90d0a335 on Jan 26, 2021

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022

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

Conflicts