tests: Add fuzzing harnesses for classes CBlockHeader, CFeeRate and various functions #18353
Conversation
practicalswift
force-pushed
the
fuzzers-coverage
branch
from
941f433 to
44abf41
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
left a comment
maflcko
left a comment
There was a problem hiding this comment.
ACK 44abf41 🏉
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 44abf417eb1cd8598084eee1a429ca57c7d0579a 🏉
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhRiwv/bqyRTqalOU8zmulOw+CcCpymXv4nkrcWqw+p1GlsNoQd1ZlA+tSJve6f
rsHCSx/TGpqMztqJ/5K1BW+FAjFtWKi0+s5EeQ1ah99281JcCUli2FgOeMUiH8oZ
TZxZvsfkFojWgDn4PiACr9YjWsBCM0K/JKf44poHRaybF1IM+RI7W2lUR8CJMram
sd5bXaw+3NG13KZCQvuRL7sShFlgizBcIFvch2VTH7Y+6y5VEqWt1ZVfWnXGE54U
x0qPfh6KCbro4gPwAlovXBF/PmmT7FKwplDsI8rQt2a2p6QdN8+i65NWg/Wib9nB
KXD4Mco1MYkl/QUlb92T1IramBqTdroI8lktcD6bsBns4u+fXGwVPCZJQ8LQcGYt
xKit1zMn1N4L34ZG1qdTzMGeo+vCu+paXPvoZ8fNcBehelnNec+RtQTiGOjG7+Y/
NMYnhSfMgFzMawX7HqE/lhs3F0YWUevoyUjrTkzQKymsHeY2m2ywaa6WRwFCXn8l
iRkFevd/
=b0BC
-----END PGP SIGNATURE-----
Timestamp of file with hash 0aac1971bcd3d59c21efc537215377f811eface51e1bcab3cac5dcd6d853c5b4 -
| } | ||
| } else { | ||
| return j != 0 && i > std::numeric_limits<T>::max() / j; | ||
| } |
There was a problem hiding this comment.
Why implement this from scratch? Could just use __builtin_mul_overflow?
There was a problem hiding this comment.
I think it would be handy to have MultiplicationOverflow(…) in other non-test parts of the code base.
When we are confident of the equivalence between MultiplicationOverflow and __builtin_mul_overflow we can implement MultiplicationOverflow by using the __builtin_mul_overflow builtin where available (gcc + clang) and the current implementation where it is not (MSVC).
That's why I added a fuzz test for MultiplicationOverflow too.
And when we have a version of MultiplicationOverflow(…) that is known to be working on all platforms we can start using it in non-test code.
Makes sense? :)
There was a problem hiding this comment.
When talking about integer overflows:
Don't miss this one: a signed integer overflow in Microsoft's SafeInt ("SafeInt is a class library for C++ that manages integer overflows")...
It's like rain on your wedding day
It's a free ride when you've already paid
It's the good advice that you just didn't take
:)
…eader, CFeeRate and various functions 44abf41 tests: Add fuzzing harness for various functions taking std::string as input (practicalswift) d69145a tests: Add fuzzing harness for MultiplicationOverflow(...) (practicalswift) 7726f3b tests: Add fuzzing harness for CFeeRate (practicalswift) 0579a27 tests: Add fuzzing harness for CBlockHeader (practicalswift) cb4eec1 tests: Add fuzzing harness for count_seconds(...) (practicalswift) Pull request description: Add fuzzing harnesses for classes `CBlockHeader`, `CFeeRate` and various functions. To test this PR: ``` $ make distclean $ ./autogen.sh $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/block_header ^c (ctrl-c) $ src/test/fuzz/fee_rate ^c (ctrl-c) $ src/test/fuzz/integer ^c (ctrl-c) $ src/test/fuzz/multiplication_overflow ^c (ctrl-c) $ src/test/fuzz/string ^c (ctrl-c) ``` ACKs for top commit: MarcoFalke: ACK 44abf41 🏉 Tree-SHA512: 2b382a7bc8efdcc6dd8b79f1637f194ecdca3e522c6618ae6c4b0bf6f86d2e79b1bb1c7160522083600616d1ed509b2f577f3a512ea3a7825a0a3794578d9d90
…eader, CFeeRate and various functions 44abf41 tests: Add fuzzing harness for various functions taking std::string as input (practicalswift) d69145a tests: Add fuzzing harness for MultiplicationOverflow(...) (practicalswift) 7726f3b tests: Add fuzzing harness for CFeeRate (practicalswift) 0579a27 tests: Add fuzzing harness for CBlockHeader (practicalswift) cb4eec1 tests: Add fuzzing harness for count_seconds(...) (practicalswift) Pull request description: Add fuzzing harnesses for classes `CBlockHeader`, `CFeeRate` and various functions. To test this PR: ``` $ make distclean $ ./autogen.sh $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/block_header ^c (ctrl-c) $ src/test/fuzz/fee_rate ^c (ctrl-c) $ src/test/fuzz/integer ^c (ctrl-c) $ src/test/fuzz/multiplication_overflow ^c (ctrl-c) $ src/test/fuzz/string ^c (ctrl-c) ``` ACKs for top commit: MarcoFalke: ACK 44abf41 🏉 Tree-SHA512: 2b382a7bc8efdcc6dd8b79f1637f194ecdca3e522c6618ae6c4b0bf6f86d2e79b1bb1c7160522083600616d1ed509b2f577f3a512ea3a7825a0a3794578d9d90
…arious functions Summary: ``` Add fuzzing harnesses for classes CBlockHeader, CFeeRate and various functions. ``` Backport of core [[bitcoin/bitcoin#18353 | PR18353]]. The amount random generator and the feerate fuzzer have been slightly adapted to match our codebase. Test Plan: ninja bitcoin-fuzzers ./test/fuzz/test_runner.py <path_to_corpus> Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8967
…CFeeRate and various functions
backport: bitcoin#18417, bitcoin#18521, bitcoin#18529, bitcoin#18176, bitcoin#18423, bitcoin#17926, bitcoin#18353, bitcoin#18407, bitcoin#18455, bitcoin#18565, bitcoin#18867 (fuzzing harness backports: part 2)
4 participants
Add fuzzing harnesses for classes
CBlockHeader,CFeeRateand various functions.To test this PR: