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

pull practicalswift wants to merge 5 commits into bitcoin:master from practicalswift:fuzz-test-cases-under-valgrind changing 4 files +51 −7

practicalswift commented at 7:18 PM on February 17, 2020: contributor

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 renamed this:
~~ci: Run fuzz testing test cases under valgrind~~
ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind
on Feb 17, 2020
practicalswift renamed this:
~~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
on Feb 17, 2020

practicalswift commented at 7:56 PM on February 17, 2020: contributor

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.

DrahtBot commented at 8:24 PM on February 17, 2020: 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:
- #16367 (Multiprocess build support by ryanofsky)
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 added the label Tests on Feb 17, 2020

in ci/test/00_setup_env_native_fuzz_with_valgrind.sh:9 in 71cd3bea89 outdated

   0 | @@ -0,0 +1,18 @@
   1 | +#!/usr/bin/env bash
   2 | +#
   3 | +# Copyright (c) 2019 The Bitcoin Core developers
   4 | +# Distributed under the MIT software license, see the accompanying
   5 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   6 | +
   7 | +export LC_ALL=C.UTF-8
   8 | +
   9 | +export CONTAINER_NAME=ci_native_fuzz

MarcoFalke commented at 12:54 AM on February 18, 2020:

export CONTAINER_NAME=ci_native_fuzz_valgrind

practicalswift commented at 6:56 AM on February 18, 2020:

Fixed!

MarcoFalke approved
MarcoFalke commented at 12:55 AM on February 18, 2020: member

ACK
ci: Run fuzz testing test cases under valgrind a3b539a924
practicalswift force-pushed on Feb 18, 2020
practicalswift commented at 6:58 AM on February 18, 2020: contributor

@MarcoFalke Thanks for reviewing! CONTAINER_NAME now changed. Please re-review :)
MarcoFalke commented at 11:24 PM on February 18, 2020: member

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

in test/fuzz/test_runner.py:150 in a3b539a924 outdated

 149 | @@ -150,7 +150,7 @@ def run_once(*, corpus, test_list, build_dir, export_coverage, use_valgrind):
 150 |              corpus_path,

MarcoFalke commented at 11:26 PM on February 18, 2020:

Unrelated to this pull, but it why is detect_leaks disabled?

I'd suggest to include this commit:

commit ffff1245c11428968b71454e4ef85417d33c9893 (HEAD)
Author: MarcoFalke <falke.marco@gmail.com>
Date:   Fri Jan 3 10:27:04 2020 -0800

    test: Enable leak detection for fuzzers
    
    This has been disabled in 16f0a186dcee563bb1000e1ffc51da87e7623bc6 for
    no given reason.

diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py
index b638e6bac6..5174e21e2a 100755
--- a/test/fuzz/test_runner.py
+++ b/test/fuzz/test_runner.py
@@ -146,7 +146,6 @@ def run_once(*, corpus, test_list, build_dir, export_coverage, use_valgrind):
         args = [
             os.path.join(build_dir, 'src', 'test', 'fuzz', t),
             '-runs=1',
-            '-detect_leaks=0',
             corpus_path,
         ]
         if use_valgrind:

practicalswift commented at 1:17 PM on February 19, 2020:

-detect_leaks=0 was required at some points to make test/fuzz/test_runner.py pass. If that is not required any more that is great news! I'll try if Travis is happy without it.

tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed 555236f769
tests: Add support for excluding fuzz targets using -x/--exclude 5ea81449f3
tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed 733bbec34f
tests: Improve test runner output in case of target errors f2472f6460

MarcoFalke commented at 4:14 PM on February 19, 2020: member

ACK f2472f64604a0c583f950c56e8753d0bee246388 👼

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

Signature:

Timestamp of file with hash 70c5ad8b8dd4b65a0d243dba35a2ed1c733a95d5036423b9984096ba45d53f36 -

</details>

MarcoFalke referenced this in commit eddcbfb109 on Feb 19, 2020
MarcoFalke merged this on Feb 19, 2020
MarcoFalke closed this on Feb 19, 2020
sidhujag referenced this in commit 0d366a0195 on Feb 22, 2020
MarcoFalke referenced this in commit ead6d686eb on Jun 25, 2020
MarcoFalke referenced this in commit 7027c67cac on Jul 2, 2020
jasonbcox referenced this in commit 5ab819115d on Nov 5, 2020
sidhujag referenced this in commit f98402e2f7 on Nov 10, 2020
practicalswift deleted the branch on Apr 10, 2021
random-zebra referenced this in commit 44b5327e61 on May 28, 2021
kittywhiskers referenced this in commit a8493e05a1 on Aug 15, 2021
vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#18162 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value #18288 build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18912 ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors

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

Conflicts