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.

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

backport: bitcoin#17725, #18304, #18392, #18430, #18438, #18441, #18477, #18562, #18569, #18683, #18899, #18912, #19159, #19669, partial #14794 - asan/valgrind CI changes #5379

Merged
merged 19 commits into from
Jun 7, 2023

Conversation

knst
Copy link
Collaborator

@knst knst commented May 18, 2023

Issue being fixed or feature implemented

This PR is the next batch of bitcoin backports, mostly related to CI, address sanitizer and valgrind.

What was done?

There are discovered 2 issues that won't let to enable Asan CI and valgrind CI at the moment.

1. Asan (address sanitizer) generates a lot of failures like that both CI and locally:

********* Finished testing of TrafficGraphDataTests *********
~BitcoinApplication : Stopping thread
~BitcoinApplication : Stopped thread
==28475==WARNING: invalid path to external symbolizer!
==28475==WARNING: Failed to use and restart external symbolizer!
=================================================================
==28475==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 520 byte(s) in 13 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x7f7a7724bd85  (<unknown module>)
Direct leak of 432 byte(s) in 2 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x55fc0dc63b63  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0xc5d1b63)
    #2 0x8604597ff0e8feff  (<unknown module>)
Direct leak of 176 byte(s) in 2 object(s) allocated from:
    #0 0x55fc08e52cad  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x77c0cad)
    #1 0x55fc0d822372  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0xc190372)
Direct leak of 66 byte(s) in 3 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x7f7a77243633  (<unknown module>)
Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x7f7a77245449  (<unknown module>)

Here's run on CI: https://gitlab.com/dashpay/dash/-/pipelines/883649210
I tried different ways to add more debug infos or check what is these addresses but I haven't localized issue at the moment.
So, ASAN build is disabled at the moment.

2. valgrind fails for each usage of std::optional when it passes as an argument in function params.

==6024== Conditional jump or move depends on uninitialised value(s) 
==6024==    at 0xF6C314: CWallet::ScanForWalletTransactions(uint256 const&, int, std::optional<int>, WalletRescanReserver const&, bool) (wallet.cpp:1976)
==6024==    by 0x8245D8: wallet_tests::scan_for_wallet_transactions::test_method() (wallet_tests.cpp:106)
==6024==    by 0x8267B4: wallet_tests::scan_for_wallet_transactions_invoker() (wallet_tests.cpp:87)
==6024==    by 0x115D4F1: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (in DASH/src/test/test_dash)
==6024==    by 0x115BB9C: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (in DASH/src/test/test_dash)
==6024==    by 0x115BC24: boost::execution_monitor::execute(boost::function<int ()> const&) (in DASH/src/test/test_dash)
==6024==    by 0x115BD04: boost::execution_monitor::vexecute(boost::function<void ()> const&) (in DASH/src/test/test_dash)
==6024==    by 0x1122834: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) (in DASH/src/test/test_dash)
==6024==    by 0x10FC18F: boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) [clone .isra.0] (in DASH/src/test/test_dash)
==6024==    by 0x10FC489: boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) [clone .isra.0] (in DASH/src/test/test_dash)
==6024==    by 0x10FC489: boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) [clone .isra.0] (in DASH/src/test/test_dash)
==6024==    by 0x11004D6: boost::unit_test::framework::run(unsigned long, bool) (in DASH/src/test/test_dash)

There's only one example, but it happens for all other places with std::optional too.
valgrind CI is actually removed from bitcoin and super-seeded by address sanitizer, but it doesn't work anyway at the moment for dash's code.

How Has This Been Tested?

Run functional and unit tests + build locally with valgrind/address sanitizer to see a result.
Based on local runs this PR includes several fixes of UB.

Breaking Changes

No breaking changes.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst force-pushed the bc-bp-valgrind-1 branch 2 times, most recently from 56e8596 to bd7b5ed Compare May 19, 2023 15:07
@github-actions
Copy link

This pull request has conflicts, please rebase.

@knst knst force-pushed the bc-bp-valgrind-1 branch from f0000da to f51288d Compare May 25, 2023 12:26
@knst knst force-pushed the bc-bp-valgrind-1 branch from f51288d to 75b0f91 Compare May 30, 2023 18:04
@knst knst changed the title backport: - add asan sanitizer (CI), valgrind and various CI changes backport: bitcoin#14794, #17725, #18304, #18392, #18430, #18438, #18441, #18477, #18562, #18569, #18683, #18899, #18912, #19159, #19669 - asan/valgrind CI changes May 30, 2023
@knst knst added this to the 20 milestone May 30, 2023
@knst knst force-pushed the bc-bp-valgrind-1 branch from 8fe5328 to 844e017 Compare May 30, 2023 19:34
@knst knst marked this pull request as ready for review May 31, 2023 07:49
@knst knst requested review from UdjinM6 and PastaPastaPasta May 31, 2023 07:49
@github-actions
Copy link

This pull request has conflicts, please rebase.

@@ -9,6 +9,10 @@ export LC_ALL=C.UTF-8
export PACKAGES="valgrind clang llvm python3-zmq libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev"
export USE_VALGRIND=1
export NO_DEPENDS=1
export TEST_RUNNER_EXTRA="--exclude rpc_bind" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
if [[ "${TRAVIS}" == "true" && "${TRAVIS_REPO_SLUG}" != "bitcoin/bitcoin" ]]; then
Copy link

Choose a reason for hiding this comment

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

18683: NOTE: this will have no effect for us (no travis and wrong repo)

@knst knst force-pushed the bc-bp-valgrind-1 branch from ff02a26 to e92819e Compare June 1, 2023 09:26
@knst knst changed the title backport: bitcoin#14794, #17725, #18304, #18392, #18430, #18438, #18441, #18477, #18562, #18569, #18683, #18899, #18912, #19159, #19669 - asan/valgrind CI changes backport: bitcoin#17725, #18304, #18392, #18430, #18438, #18441, #18477, #18562, #18569, #18683, #18899, #18912, #19159, #19669, partial #14794 - asan/valgrind CI changes Jun 1, 2023
@knst knst requested a review from UdjinM6 June 1, 2023 09:27
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

MarcoFalke and others added 8 commits June 7, 2023 01:50
facb416 ci: Add valgrind run (MarcoFalke)

Pull request description:

  Fixes bitcoin#17460

ACKs for top commit:
  practicalswift:
    ACK facb416

Tree-SHA512: 55396e548a76f976d7b7170b68bc5f93cfd44656162267172f66db7eb549699a2a22d3b1bb0d5f180fe0697931939e652c8cdb86b435e81e7ce572485798009d
4444edc ci: Enable all functional tests in valgrind (MarcoFalke)

Pull request description:

  The travis timeout for our repo has been bumped to 2h, so we can run all tests in valgrind now

ACKs for top commit:
  practicalswift:
    ACK 4444edc -- regarding the three disabled cases (`feature_abortnode`, `feature_block` and `rpc_bind`): not a big deal since MSan will take care of those once bitcoin#18288 is merged. More is more :)

Tree-SHA512: ea2f798112911b6d1f3d88cfcdf0a7cdb698687248343703d6fe55da144542c961c15d888bffb41672c10aa76765615cb7c7ff93d468bfad3c51f962f24e7abb
fa92af5 ci: Run feature_block and feature_abortnode in valgrind (MarcoFalke)
fa01feb test: Remove ci timeout restriction in test_runner (MarcoFalke)

Pull request description:

  Also revert commit 0a4912e, because some tests take too long for this to be useful anymore.

Top commit has no ACKs.

Tree-SHA512: 363f14766e1f4a5860ab668a516b41acebc6fbdf11d8defb3a95a772dbf82304ca1f5f14b1dbad97f2029503e03d92e8c69df0466a8872409c20665838f617ed
fab7d95 test: Make valgrind.supp work on aarch64 (MarcoFalke)

Pull request description:

  Was easy to fix by simply removing a line

ACKs for top commit:
  practicalswift:
    ACK fab7d95 -- diff looks correct

Tree-SHA512: d2d7c6cac453a3177c20e256ec50a03066f8dbf5ae45299077ccf4a2b45a3a40252b1b5fcaf9224a59bb5c3df5bd90ac58af27eb0f47dc87c2640df5b2b460ca
fa082d0 travis: Remove valgrind (MarcoFalke)

Pull request description:

  When the valgrind run was added, it took 2 hours. Travis kindly raised the timeout limit to the maximum possible of 3 hours.

  Today, a full build of Bitcoin Core with all tests takes more than three hours. Thus, it is impossible to run all tests on travis.

  Moreover, the feedback loop for developers that create a pull request takes at least 2 hours, but in some cases (when the travis queue is full) until the next day. This is unacceptable.

  Fix both issues by removing the build from travis.

  Please note that the `ci/test/` configurations are *not* removed. They will stay in the repo and can be executed anywhere (just not on travis).

ACKs for top commit:
  jamesob:
    ACK bitcoin@fa082d0
  jnewbery:
    utACK fa082d0

Tree-SHA512: 9acaa0e2d3926014fadb7dd2e86c4e01df382e9399f6ae99f989fa609da66a77bdd1b75d6ff42d2686f38f730b8564e6dc722aa597a473290c9d30c2abe7ef0f
fae1e99 ci: Only clone bitcoin-core/qa-assets when fuzzing (MarcoFalke)

Pull request description:

  Currently the only content of that repo are some seeds, so we can speed up some ci builds

ACKs for top commit:
  laanwj:
    ACK fae1e99 (provided this passes travis)

Tree-SHA512: ed813738e7f24bb56a2f12aa3b398e414eb4f0ba98379836a33ff3e5602cbf42a28e89aad10e346468191ecddc03e60d5b236097112e27c07cb1c2293533ea58
25c8b73 ci: Use Homebrew addon on native macOS (Hennadii Stepanov)
596c627 ci: Fix brew in Travis (Hennadii Stepanov)

Pull request description:

  Recently almost every macOS image update on Travis breaks our builds:
  - bitcoin#17848
  - bitcoin#18436

  This PR:
  - fixes the error caused by the recent [update](https://changelog.travis-ci.com/xcode-11-3-1-xcode-11-2-1-xcode-11-1-and-xcode11-images-updated-142286) from 10.14.4 (18E226) to 10.14.6 (18G3020) on March 25
  - leverages [Homebrew addon](https://config.travis-ci.com/ref/job/addons/homebrew) to install packages

  Homebrew is not told to install `automake` and `pkg-config` packages, as the [docs](https://docs.travis-ci.com/user/reference/osx/#compilers-and-build-toolchain) states that they are pre-installed:
  >    - automake 1.16.1
  >    - pkg-config 0.29.2

Top commit has no ACKs.

Tree-SHA512: 1a70c06468fbe162503081b03dcf54614d67abf8ff0ce07d118b5bb50bbb92c182a76f769bea586c691aa82b9281a29cdef88091acc16895817a2e7cddafec6e
…scalar

e41e46c ci: Remove misplaced comments from folded block scalar (Hennadii Stepanov)

Pull request description:

  On master (f3a91ab) the [build log for ARM](https://travis-ci.org/github/bitcoin/bitcoin/jobs/667247758) contains:
  ```
  $ export FILE_ENV="./ci/test/00_setup_env_arm.sh"
  $ export QEMU_USER_CMD=""
  $ export Can=
  $ export run=
  $ export the=
  $ export tests=
  $ export natively=
  $ export without=
  $ export qemu=

  Setting up build cache
  ...
  ```
  and
  ![Screenshot from 2020-03-26 18-58-50](https://user-images.githubusercontent.com/32963518/77674223-e28d2d00-6f93-11ea-82a1-7f805ccb678d.png)

  With this PR:
  ```
  $ export FILE_ENV="./ci/test/00_setup_env_arm.sh"
  $ export QEMU_USER_CMD=""

  Setting up build cache
  ...
  ```

  and
  ![Screenshot from 2020-03-26 19-01-56](https://user-images.githubusercontent.com/32963518/77674456-4d3e6880-6f94-11ea-9c83-546c81f1cdf7.png)

  ---

  Also Travis [build config validation](https://docs.travis-ci.com/user/build-config-validation) added:
  ![Screenshot from 2020-03-26 19-04-19](https://user-images.githubusercontent.com/32963518/77674686-9ee6f300-6f94-11ea-900f-fdef8a673113.png)

Top commit has no ACKs.

Tree-SHA512: a3ea05d543a4d46c65fffd7c67df9bbd4ca8d426fa215c5ce26653de34145edd419a54884da0416608a2c90bf5b421cab3ec32290c8916f10399354b44ddeb99
fanquake and others added 11 commits June 7, 2023 01:50
faa9491 ci: Use Focal for fuzzers (MarcoFalke)

Pull request description:

  This gives us access to clang-10, as well as a newer version of valgrind

ACKs for top commit:
  fanquake:
    ACK faa9491 - [Clang 10](https://packages.ubuntu.com/focal/clang) and [valgrind 3.15](https://packages.ubuntu.com/focal/valgrind).
  practicalswift:
    ACK faa9491 -- diff looks correct & contemporary clang is better than vintage clang

Tree-SHA512: 0e67232673434c0309db79c1054e3e981115083585945967e346f4d58792635832100f89911428aab928155e44e5f401207a023681ae008fdb5280cf02c4d427
fa7af33 ci: Run unit tests sequential once (MarcoFalke)
fa68a3e appveyor: Enable minimal unit test logging to aid debugging (MarcoFalke)

Pull request description:

  Fixes bitcoin#16976

Top commit has no ACKs.

Tree-SHA512: 1f1ee8776a67afa8c1c5a16ef170c9975b6a486c087c8eba12e97d23382befd1c2801622ec70ca8e4cd1fbedce1dec46be67677ceaf07f35f1d3f3bead0200f0
6136a96 ci: Rename RUN_CI_ON_HOST to DANGER_RUN_CI_ON_HOST (Hennadii Stepanov)
97ba77a ci: Add native s390x (Hennadii Stepanov)

Pull request description:

  Unlike the Docker wrapped solution (bitcoin#17591) this PR suggests running on host system directly.

  This approach makes builds quick and stable (see: bitcoin#18106).

  The excerpt from the Travis log:
  ```
  ...
  Running on host system without docker wrapper
  ...
  Byte Order: Big Endian
  ...
  ```

ACKs for top commit:
  MarcoFalke:
    ACK 6136a96

Tree-SHA512: 1b591de13e38d10a35217e1de11cbd648a359d18d16eed166fac18ea5788b58cc9fc6d407086ed342b99e57e479efd951a0ea693710177e500eb116316b9a788
…epos to avoid timeouts

faceeae ci: Disable valgrind functionl tests on forked repos to avoid timeouts (MarcoFalke)

Pull request description:

  Allows people to fork our repo and run the tests again

  Also print more cache stats

ACKs for top commit:
  hebasto:
    ACK faceeae, tested on my own repo: https://travis-ci.org/github/hebasto/bitcoin/jobs/676257500

Tree-SHA512: 50e44edf94fcb997438eeaf7308b2b58a0854141ecb1fbb0ba7bf5ed662f19b60899f966f579cca90ad5e789234d0e90122d8c2c854da70339058adc3a475fa6
…-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
fa5288c contrib: Fixup valgrind suppressions file (MarcoFalke)

Pull request description:

  I am observing this one on bionic with system boost::fs:

  ```
  {
     <insert_a_suppression_name_here>
     Memcheck:Cond
     fun:__wcsnlen_avx2
     fun:wcsnrtombs
     fun:_ZNKSt7codecvtIwc11__mbstate_tE6do_outERS0_PKwS4_RS4_PcS6_RS6_
     fun:_ZN5boost10filesystem11path_traits7convertEPKwS3_RNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt7codecvtIwc11__mbstate_tE
     fun:_ZN5boost10filesystem6detail11unique_pathERKNS0_4pathEPNS_6system10error_codeE
  ...

ACKs for top commit:
  practicalswift:
    ACK fa5288c -- patch looks correct

Tree-SHA512: 067e10e932a7f5b13e516134e0cfd3030265c1b582cdfde1cea97042e31399aa40c4590710a39429854c68ad703a0ae9f0b06e9af4cdd81e3cacb042939a84b6
```
********* Finished testing of TrafficGraphDataTests *********
~BitcoinApplication : Stopping thread
~BitcoinApplication : Stopped thread
==28475==WARNING: invalid path to external symbolizer!
==28475==WARNING: Failed to use and restart external symbolizer!
=================================================================
==28475==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 520 byte(s) in 13 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x7f7a7724bd85  (<unknown module>)
Direct leak of 432 byte(s) in 2 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x55fc0dc63b63  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0xc5d1b63)
    dashpay#2 0x8604597ff0e8feff  (<unknown module>)
Direct leak of 176 byte(s) in 2 object(s) allocated from:
    #0 0x55fc08e52cad  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x77c0cad)
    #1 0x55fc0d822372  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0xc190372)
Direct leak of 66 byte(s) in 3 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x7f7a77243633  (<unknown module>)
Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x55fc08e2353d  (/builds/dashpay/dash/build-ci/dashcore-linux64_asan/src/qt/test/test_dash-qt+0x779153d)
    #1 0x7f7a77245449  (<unknown module>)
```
@PastaPastaPasta PastaPastaPasta merged commit ae9abb9 into dashpay:develop Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants