Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors #18166

Merged
merged 5 commits into from Feb 19, 2020
Merged

ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors #18166

merged 5 commits into from Feb 19, 2020

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Feb 17, 2020 •
edited

Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind.

This would have caught util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value (#18162) and similar cases.

@practicalswift practicalswift changed the title ci: Run fuzz testing test cases under valgrind ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind Feb 17, 2020
@practicalswift practicalswift changed the title ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors Feb 17, 2020
@practicalswift
Copy link
Contributor Author

Failing as intended in Travis:

…
Run integer with args ['valgrind', '--quiet', '--error-exitcode=1', '/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/integer', '-runs=1', '-detect_leaks=0', '/home/travis/build/bitcoin/bitcoin/ci/scratch//qa-assets/fuzz_seed_corpus/integer']
Output: INFO: Seed: 3664174428
INFO: Loaded 1 modules   (154709 inline 8-bit counters): 154709 [0xea0bc8, 0xec681d), 
INFO: Loaded 1 PC tables (154709 PCs): 154709 [0xec6820,0x1122d70), 
INFO:       79 files found in /home/travis/build/bitcoin/bitcoin/ci/scratch//qa-assets/fuzz_seed_corpus/integer
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 79 min: 1b max: 83b total: 5116b rss: 129Mb
==26395== Conditional jump or move depends on uninitialised value(s)
==26395==    at 0x4F43C0A: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==26395==    by 0x4F501A4: std::ostream& std::ostream::_M_insert<long>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==26395==    by 0x543C1C: formatValue<int> (tinyformat.h:358)
==26395==    by 0x543C1C: void tinyformat::detail::FormatArg::formatImpl<int>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543)
==26395==    by 0x532D1F: format (tinyformat.h:528)
==26395==    by 0x532D1F: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907)
==26395==    by 0x5EE190: vformat (tinyformat.h:1054)
==26395==    by 0x5EE190: format<int, int, int> (tinyformat.h:1064)
==26395==    by 0x5EE190: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int>(char const*, int const&, int const&, int const&) (tinyformat.h:1073)
==26395==    by 0x5EE0C0: FormatISO8601Date[abi:cxx11](long) (time.cpp:112)
…

Will fail until #18162 is merged.

@practicalswift practicalswift mentioned this pull request Feb 17, 2020
Merged
@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, 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.

@DrahtBot DrahtBot added the Tests label Feb 17, 2020
@DrahtBot DrahtBot mentioned this pull request Feb 17, 2020
Merged
maflcko
maflcko approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

ci/test/00_setup_env_native_fuzz_with_valgrind.sh Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor Author

@MarcoFalke Thanks for reviewing! CONTAINER_NAME now changed. Please re-review :)

@maflcko
Copy link
Member

maflcko commented Feb 18, 2020

It could make sense to add an --exclude argument to the test_runner to skip a test target (in this case --exclude integer).

@maflcko
Copy link
Member

maflcko commented Feb 19, 2020

ACK f2472f6 👼

Show signature and timestamp

Signature:

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

ACK f2472f64604a0c583f950c56e8753d0bee246388 👼
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiMtgv/eLFsfqh9fC/1+Xam7Wy9imO1pDZigPBciBNC65SfSHx8iOWLFaN/AoNs
CcCvOSnjd6yPsja6uajgjiz2q83DjCGBLNKph8D03yJ+WMQt2DcLJUF52EZffSVq
XU7w+81kmxtQMmpFQOYFb/re1ScaGJ4U1bUeISsVeHlWhljw6XinSuLFFyJW4OA0
ahD9Wrs1VF8zypuIPEKwNTzkCjyj6eeUsZLyE7JCvJ3LXhPn/HX7dt18ErekvTFV
7IHr3shTot5aMhmDnugjmLJ/NeMO1ynFBFjFioNNzU4Ztf/PaUtTt3jxe/umfPyK
4kp4ntEOGTzSBW2WDN+qXGNwrBmToIolOnRcVyYTgwLn3kMC3x/952Om2+D8m1WI
9up5/fEd5UEHLv5GVi3BXXFwVI/qZcCvpy3IqxnDMSwi3juhIJ5QonmsIEpqv2yo
KWriT4nvULRhFBpfmlHaDoWLIQkl5nJTeJZLlEt2/t5/ioT/hkz8Ig7wNjAowRaQ
K3ltYj3u
=adP/
-----END PGP SIGNATURE-----

Timestamp of file with hash 70c5ad8b8dd4b65a0d243dba35a2ed1c733a95d5036423b9984096ba45d53f36 -

maflcko pushed a commit that referenced this pull request Feb 19, 2020
Merge #18166: ci: Run fuzz testing test cases (bitcoin-core/qa-assets…
eddcbfb 
…) under valgrind to catch memory errors

f2472f6 tests: Improve test runner output in case of target errors (practicalswift)
733bbec tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea8144 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a ci: Run fuzz testing test cases under valgrind (practicalswift)

Pull request description:

  Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.

  This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases.

ACKs for top commit:
  MarcoFalke:
    ACK f2472f6 👼

Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
@maflcko maflcko merged commit f2472f6 into bitcoin:master Feb 19, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 22, 2020
Merge bitcoin#18166: ci: Run fuzz testing test cases (bitcoin-core/qa…
0d366a0 
…-assets) under valgrind to catch memory errors

f2472f6 tests: Improve test runner output in case of target errors (practicalswift)
733bbec tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea8144 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a ci: Run fuzz testing test cases under valgrind (practicalswift)

Pull request description:

  Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.

  This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (bitcoin#18162) and similar cases.

ACKs for top commit:
  MarcoFalke:
    ACK f2472f6 👼

Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
@practicalswift practicalswift mentioned this pull request Mar 7, 2020
Merged
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
@practicalswift practicalswift mentioned this pull request May 8, 2020
Merged
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 25, 2020
Merge bitcoin#18912: ci: Run fuzz testing test cases (bitcoin-core/qa…
ead6d68 
…-assets) under valgrind to catch memory errors

3f686d1 ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors (practicalswift)

Pull request description:

  Re-introduce the Travis valgrind fuzzing job which was removed by PR bitcoin#18899. The removal seems to have been made by accident since the removed job does not appear to be the source of the problem the PR set out to fix.

  ---

  Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.

  This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (bitcoin#18162) and similar cases.

  This fuzzing job was introduced in bitcoin#18166.

Top commit has no ACKs.

Tree-SHA512: 6e2681eb0ade6af465c5ea91ac163a337465d2130ec9880ba57a36d9af7c25682734586a32977dc25972d4f78483f339d680ea48c0ae13cf1dfa52b617aae401
maflcko pushed a commit that referenced this pull request Jul 2, 2020
Merge #18288: build: Add MemorySanitizer (MSan) in Travis to detect u…
7027c67 
…se of uninitialized memory

870f0cd build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory (practicalswift)

Pull request description:

  Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory.

  First UBSan, then ASan followed by TSan... and now: yes, the wait is over -- **MSan is finally here!** :)

  Some historical context:
  * 2017: Continuous compilation with Clang Thread Safety analysis enabled (#10866, #10923)
  * 2018: Continuous testing with trapping on signed integer overflows (`-ftrapv`) (#12686)
  * 2018: Continuous testing of use of locale dependent functions (#13041)
  * 2018: Continuous testing of format strings (#13705)
  * 2018: Continuous compilation with MSVC `TreatWarningAsError` (#14151)
  * 2018: Continuous testing under UndefinedBehaviorSanitizer – UBSan (#14252, #14673, #17006)
  * 2018: Continuous testing under AddressSanitizer – ASan (#14794, #17205, #17674)
  * 2018: Continuous testing under ThreadSanitizer – TSan (#14829)
  * 2019: Continuous testing in an unsigned char environment (`-funsigned-char`) (#15134)
  * 2019: Continuous compile-time testing of assumptions we're making (#15391)
  * 2019: Continuous testing of fuzz test cases under Valgrind (#17633, #18159, #18166)
  * 2020: Finally... MemorySanitizer – MSAN! :)

  What is the next step? What tools should we add to CI to keep bugs from entering `master`? :)

ACKs for top commit:
  MarcoFalke:
    ACK 870f0cd

Tree-SHA512: 38327c8b75679d97d469fe42e704cacd1217447a5a603701dd8a58ee50b3be2c10248f8d68a479ed081c0c4b254589d3081c9183f991640b06ef689061f75578
@practicalswift practicalswift mentioned this pull request Oct 1, 2020
Merged
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 5, 2020
@practicalswift @Fabcien
Various fuzz test runner improvements
5ab8191 
Summary:
Backport of core [[bitcoin/bitcoin#18166 | PR18166]].

The title has been changed because the CI changes are not ported.

Test Plan:
  ninja bitcoin-fuzzers
  ./test/fuzz/test_runner.py <path_to_corpus>

Reviewers: #bitcoin_abc, PiRK

Reviewed By: PiRK

Differential Revision: https://reviews.bitcoinabc.org/D8276
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
Merge bitcoin#18166: ci: Run fuzz testing test cases (bitcoin-core/qa…
f98402e 
…-assets) under valgrind to catch memory errors

f2472f6 tests: Improve test runner output in case of target errors (practicalswift)
733bbec tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea8144 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a ci: Run fuzz testing test cases under valgrind (practicalswift)

Pull request description:

  Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.

  This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (bitcoin#18162) and similar cases.

ACKs for top commit:
  MarcoFalke:
    ACK f2472f6 👼

Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
@furszy furszy mentioned this pull request Mar 23, 2021
Merged
@practicalswift practicalswift deleted the fuzz-test-cases-under-valgrind branch April 10, 2021 19:39
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request May 28, 2021
@random-zebra
Merge #2252: Fuzzing framework support
44b5327 
d059544 [Build] fuzz target, change LIBBITCOIN_ZEROCOIN link order. (furszy)
2396e6b [fuzz] Add ContextualCheckTransaction call to transaction target. (furszy)
f0887a0 Fuzzing documentation "PIVX-fication" (furszy)
9631f46 [doc] add sanitizers documentation in developer-notes.md (furszy)
70a0ace tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. Avoid code repetition. (practicalswift)
e1b92b6 ignore new fuzz targets gitignore (furszy)
d058d8c tests: Add deserialization fuzzing harnesses (furszy)
e1f666c tests: Remove TRANSACTION_DESERIALIZE (replaced by transaction fuzzer) (practicalswift)
b5f291c tests: Add fuzzing harness for CheckTransaction(...), IsStandardTx(...) and other CTransaction related functions (furszy)
3205871 fuzz: Remove option --export_coverage from test_runner (MarcoFalke)
52693ee fuzz: Add option to merge input dir to test runner (MarcoFalke)
2b4f8aa doc: Remove --disable-ccache from docs (MarcoFalke)
b54b1d6 tests: Improve test runner output in case of target errors (practicalswift)
cd6134f test: Log output even if fuzzer failed (MarcoFalke)
48cd0c8 doc: Improve fuzzing docs for macOS users (Fabian Jahr)
d642b67 [Build] Do not disable wallet when fuzz is enabled. (furszy)
c3447b5 Update doc and CI config (qmma)
1266d3e Disable other targets when enable-fuzz is set (qmma)
f28ac9a build: Allow to configure --with-sanitizers=fuzzer (MarcoFalke)
425742c fuzz: test_runner: Better error message when built with afl (MarcoFalke)
541f442 qa: Add test/fuzz/test_runner.py (MarcoFalke)
89fe5b2 Add missing LIBBITCOIN_ZMQ to test target (furszy)
58dbe79 add fuzzing binaries to gitignore. (furszy)
393a126 fuzz: Move deserialize tests to test/fuzz/deserialize.cpp (MarcoFalke)
a568df5 test: Build fuzz targets into separate executables (furszy)
d5dddde [test] fuzz: make test_one_input return void (MarcoFalke)
2e4ec58 [fuzzing] initialize chain params by default. (furszy)
08d8ebe [tests] Add libFuzzer support. (practicalswift)
84f72da [test] Speed up fuzzing by ~200x when using afl-fuzz (practicalswift)
faf2be6 Init ECC context for test_bitcoin_fuzzy. (Gregory Maxwell)
11150df Make fuzzer actually test CTxOutCompressor (Pieter Wuille)
d6f6a85 doc: Add bare-bones documentation for fuzzing (Wladimir J. van der Laan)
5c3b550 Simple fuzzing framework (pstratem)

Pull request description:

  As the title says, adding fuzzing framework support so we can start getting serious on this area as well.

  Adapted the following PRs:

  * bitcoin#9172.
  * bitcoin#9354.
  * bitcoin#9691.
  * bitcoin#10415.
  * bitcoin#10440.
  * bitcoin#15043.
  * bitcoin#15047.
  * bitcoin#15295.
  * bitcoin#15399 (fabcfa5 only).
  * bitcoin#16338.
  * bitcoin#17051.
  * bitcoin#17076.
  * bitcoin#17225.
  * bitcoin#17942.
  * bitcoin#16236 (only fa35c42).
  * bitcoin#18166 (only f2472f6).
  * bitcoin#18300.
  * And.. probably will go further and continue adapting more PRs..

ACKs for top commit:
  random-zebra:
    utACK d059544 and merging...

Tree-SHA512: c0b05bca47bf99bafd8abf1453c5636fe05df75f16d0e9c750368ea2aed8142f0b28d28af1d23468b8829188412a80fd3b7bdbbda294b940d78aec80c1c7d03a
kwvg added a commit to kwvg/dash that referenced this pull request Aug 15, 2021
@kwvg
bitcoin#18166: Run fuzz testing test cases (bitcoin-core/qa-assets) u…
a8493e0 
…nder valgrind to catch memory errors
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Oct 4, 2021
@vijaydasmp
Merge bitcoin#18288: build: Add MemorySanitizer (MSan) in Travis to d…
dfa262c 
…etect use of uninitialized memory

870f0cd build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory (practicalswift)

Pull request description:

  Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory.

  First UBSan, then ASan followed by TSan... and now: yes, the wait is over -- **MSan is finally here!** :)

  Some historical context:
  * 2017: Continuous compilation with Clang Thread Safety analysis enabled (bitcoin#10866, bitcoin#10923)
  * 2018: Continuous testing with trapping on signed integer overflows (`-ftrapv`) (bitcoin#12686)
  * 2018: Continuous testing of use of locale dependent functions (bitcoin#13041)
  * 2018: Continuous testing of format strings (bitcoin#13705)
  * 2018: Continuous compilation with MSVC `TreatWarningAsError` (bitcoin#14151)
  * 2018: Continuous testing under UndefinedBehaviorSanitizer – UBSan (bitcoin#14252, bitcoin#14673, bitcoin#17006)
  * 2018: Continuous testing under AddressSanitizer – ASan (bitcoin#14794, bitcoin#17205, bitcoin#17674)
  * 2018: Continuous testing under ThreadSanitizer – TSan (bitcoin#14829)
  * 2019: Continuous testing in an unsigned char environment (`-funsigned-char`) (bitcoin#15134)
  * 2019: Continuous compile-time testing of assumptions we're making (bitcoin#15391)
  * 2019: Continuous testing of fuzz test cases under Valgrind (bitcoin#17633, bitcoin#18159, bitcoin#18166)
  * 2020: Finally... MemorySanitizer – MSAN! :)

  What is the next step? What tools should we add to CI to keep bugs from entering `master`? :)

ACKs for top commit:
  MarcoFalke:
    ACK 870f0cd

Tree-SHA512: 38327c8b75679d97d469fe42e704cacd1217447a5a603701dd8a58ee50b3be2c10248f8d68a479ed081c0c4b254589d3081c9183f991640b06ef689061f75578
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@practicalswift @DrahtBot @maflcko