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

practicalswift · 2021-01-24T20:31:26Z

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:

bitcoin/src/addrman.cpp

Line 306 in 32b191f

pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty);

bitcoin/src/addrman.h

Line 446 in 32b191f

const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;

bitcoin/src/arith_uint256.cpp

Line 32 in 32b191f

pn[i + k] |= (a.pn[i] << shift);

bitcoin/src/arith_uint256.cpp

Line 47 in 32b191f

pn[i - k - 1] |= (a.pn[i] << (32 - shift));

bitcoin/src/chain.cpp

Line 151 in 32b191f

return sign * r.GetLow64();

bitcoin/src/coins.cpp

Line 114 in 32b191f

cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();

bitcoin/src/compressor.cpp

Line 162 in 32b191f

return 1 + (n*9 + d - 1)*10 + e;

bitcoin/src/compressor.cpp

Line 188 in 32b191f

n *= 10;

bitcoin/src/hash.cpp

Line 13 in 32b191f

return (x << r) | (x >> (32 - r));

bitcoin/src/pubkey.h

Line 152 in 32b191f

while (len--)

bitcoin/src/streams.h

Line 570 in 32b191f

m_buffer |= (data << (64 - nbits)) >> (64 - 8 + m_offset);

bitcoin/src/util/bip32.cpp

Line 57 in 32b191f

ret += strprintf("/%i", (i << 1) >> 1);

bitcoin/src/util/strencodings.cpp

Line 562 in 32b191f

for (auto ch : str) r += ToLower((unsigned char)ch);

bitcoin/src/util/strencodings.h

Line 164 in 32b191f

accumulator |= a[i] ^ b[i%b.size()];

practicalswift · 2021-01-24T22:02:11Z

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

Edit: See #21001 (comment).

DrahtBot · 2021-01-24T23:43:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

maflcko · 2021-01-25T07:37:56Z

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)

practicalswift · 2021-01-25T12:29:41Z

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

maflcko · 2021-01-25T12:34:52Z

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

practicalswift · 2021-01-25T20:50:19Z

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

# 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

…-fsanitize=integer

maflcko · 2021-01-26T08:15:33Z

review ACK f0f8b1a 🤚

Show signature and timestamp

Signature:

Timestamp of file with hash 6949a3e88bd4ef7a52797e84db8a5eea62639166a78d8c2eed13eccb4dcfd32d -

maflcko · 2021-01-26T08:16:00Z

review ACK 0d98997 🎅

Show signature and timestamp

Signature:

Timestamp of file with hash a8d5520f601c503b2ecc40e4169f8355ed84571b1827d52455af1e4256107a2b -

maflcko · 2021-01-26T08:42:29Z

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

maflcko · 2021-01-26T08:44:46Z

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

…ts to n… …ot warn under -fsanitize=integer

laanwj added the Tests label Jan 24, 2021

This was referenced Jan 24, 2021

Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification #20962

Closed

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

Remove UBSan suppressions for CTxMemPool* #17791

Closed

practicalswift force-pushed the fsanitize-integer branch 2 times, most recently from 73f9dce to 03e34b5 Compare January 25, 2021 20:40

practicalswift added 2 commits January 25, 2021 20:55

fuzz: Avoid -fsanitize=integer warnings in fuzzing harnesses

58232e3

fuzz: Add UBSan suppressions needed for fuzz tests to not warn under …

f0f8b1a

…-fsanitize=integer

practicalswift force-pushed the fsanitize-integer branch from 03e34b5 to 0d98997 Compare January 25, 2021 20:59

maflcko force-pushed the fsanitize-integer branch from 0d98997 to f0f8b1a Compare January 26, 2021 08:34

maflcko merged commit 32d44d2 into bitcoin:master Jan 26, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 26, 2021

Merge bitcoin#21000: fuzz: Add UBSan suppressions needed for fuzz tes…

2c90d0a

…ts to n… …ot warn under -fsanitize=integer

ajtowns mentioned this pull request Jan 28, 2021

Cleanup of -debug=net log messages #20724

Merged

Crypt-iQ mentioned this pull request Feb 14, 2021

tests: fix -fsanitize=integer complaints #19713

Closed

practicalswift deleted the fsanitize-integer branch April 10, 2021 19:45

vasild mentioned this pull request Jul 28, 2021

p2p: fix ubsan addrman errors, make nTime truncation conversion explicit #22094

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

practicalswift commented Jan 24, 2021

Uh oh!

practicalswift commented Jan 24, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Jan 24, 2021

Uh oh!

maflcko commented Jan 25, 2021

Uh oh!

practicalswift commented Jan 25, 2021

Uh oh!

maflcko commented Jan 25, 2021 •

edited

Loading

Uh oh!

practicalswift commented Jan 25, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

practicalswift commented Jan 24, 2021

Uh oh!

practicalswift commented Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 24, 2021

Conflicts

Uh oh!

maflcko commented Jan 25, 2021

Uh oh!

practicalswift commented Jan 25, 2021

Uh oh!

maflcko commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jan 25, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

maflcko commented Jan 26, 2021

Uh oh!

Uh oh!

practicalswift commented Jan 24, 2021 •

edited

Loading

maflcko commented Jan 25, 2021 •

edited

Loading