build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

practicalswift commented at 12:42 PM on March 7, 2020: contributor

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

fanquake added the label Build system on Mar 7, 2020

fanquake added the label Tests on Mar 7, 2020

practicalswift force-pushed on Mar 7, 2020

MarcoFalke commented at 9:20 PM on March 7, 2020: member

We already run against valgrind and msan is a subset of valgrind according to: "MSan implements a subset of functionality found in Valgrind (Memcheck tool). It is significantly faster than Memcheck (TODO:benchmark)." (https://github.com/google/sanitizers/wiki/MemorySanitizer)

practicalswift force-pushed on Mar 7, 2020

practicalswift commented at 11:14 PM on March 7, 2020: contributor

@MarcoFalke

I think we should use both. While both tools try to find uses of uninitialized memory (UUM) they are implemented in totally different ways: Valgrind's memcheck is using binary translation while MSan is using compiler-based instrumentation. ~~From my experience it is not guaranteed that Valgrind finds all issues reported by MSan and vice versa.~~

Furthermore, having the combination libFuzzer+MSan readily available is really crucial when fuzzing. The combination libFuzzer+Valgrind is only usable for testing against an existing fuzzing corpus like we do in Travis -- not for doing any actual fuzzing. For the libFuzzer+MSan combination to be available it is important to have the cumbersome MSan setup process documented and also kept up-to-date and working over time. We get that for free by having an MSan build in Travis.

Also MSan is order of magnitudes faster than Valgrind which means that we can run also tests that are too slow for Valgrind in Travis.

And last but not least MSan can report the name of the variable that has not been initialized (thanks to MSan using compiler-based instrumentation). Valgrind only points to the function name.

In summary: I think having MSan in Travis is a very clear win. Let's use all available tools to try to kill the uninitialized memory (UUM) bug class :)

Let me know if you can think of any problems that could arise from testing also under MSan in Travis.

practicalswift force-pushed on Mar 7, 2020

practicalswift commented at 11:23 PM on March 7, 2020: contributor

@MarcoFalke

We already run against valgrind […]

Correct if you mean the unit tests.

Incorrect if you mean the functional tests :)

Due to Valgrind's slowness we currently only run a single(!) functional test under Valgrind (TEST_RUNNER_EXTRA="p2p_segwit.py").

Note that this PR runs all available functional tests (and unit tests) under MSan.

gmaxwell commented at 1:38 AM on March 8, 2020: contributor

I'd love to see a bug that msan catches that valgrind doesn't.

practicalswift commented at 8:24 AM on March 8, 2020: contributor

From my experience it is not guaranteed that Valgrind finds all issues reported by MSan and vice versa.

I'd love to see a bug that msan catches that valgrind doesn't.

Sorry about making a vague claim that I cannot back up with an example. Until I can provide an example we can simplify the discussion by assuming that I was flat out wrong about the possibility of Valgrind not catching an issue found by MSan (given enough runtime to compensate for being orders of magnitude slower than MSan).

From a practical perspective the main reason to start using MSan is the speedup that will allow us to go from detecting use of uninitialized memory in code covered by a single(!) functional test (p2p_segwit.py) to detecting use of uninitialized memory in all code covered by our functional tests.

Please note that I'm not suggesting removing the Valgrind job: I think we should use both and ideally bump the timeouts so that Valgrind can test a couple of more functional tests at least :)

(Just to be clear: I love both Valgrind (proof of love: #18166, #18162, #18159, #17633, #17640, #17624, #17455, #15117, #11035, #11024, #10977) and MSan – I use them daily :))

Let me know if you can think of any problems that could arise from testing also under MSan in Travis.

gmaxwell commented at 9:39 AM on March 8, 2020: contributor

That's fine, if you ever find any-- I'd be interested in seeing them whenever that is. The question was primarily driven out of my own curiosity.

Faster can be pretty useful, though I've found msan in the past to be somewhat lacking compared to valgrind in terms of sensitivity -- though my experience is mostly from when msan was very new. I've found msan most useful in the context of fuzzing, where any speed difference makes for a meaningful throughput difference which ultimately results in a sensitivity difference.

The fact that even paid travis is so cpu starved that this is a big issue for just CI tests is something of is own problem-- labour is expensive, insight is rare. CPU time is the one resource needed by the project which should never be the limiting ingredient, this matters because a surplus of CPU time can be a force multiplier for human labour (though my idea of 'reasonable amounts of computing power' starts at about a thousand core ghz per person, and I've always found it really weird how software people who are at the top of their field often work from just a single underpowered laptop; so maybe I'm just a weirdo). FWIW, for a number of years I tested every release of bitcoin in valgrind, including doing IBDs (overnight).

I don't see any problem with making more use of msan, though it's not a complete replacement for valgrind. But more is more. :)

practicalswift commented at 1:27 PM on March 10, 2020: contributor

@MarcoFalke

Would you mind reviewing? :)

I believe that having this merged would have allowed us to automatically detect at least two remotely triggerable uses of uninitialized memory (UUM) in our P2P networking code:

2020: Use of uninitialized memory in Erlay networking code - found pre-merge
2020: Use of uninitialized memory in BIP324 encrypted p2p transport de-/serializer code (truth in advertising: I haven't verified this one by writing a PoC) - found pre-merge
2019: Use of uninitialized memory in networking code when receiving a transaction we already have - found post-merge

FWIW - a subset of non-networking uses of uninitialized memory:

in ci/test/00_setup_env_native_nowallet_msan.sh:19 in a4a25a0f1a outdated

  14 | +
  15 | +export CONTAINER_NAME="ci_native_nowallet_msan"
  16 | +export PACKAGES="clang-8 llvm-8 cmake"
  17 | +export DEP_OPTS="NO_WALLET=1 NO_UPNP=1 NO_QT=1 CC='clang-8' CXX='clang++-8' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' boost_toolset_linux='clang' boost_cxx='clang++-8' boost_cxxflags='-std=c++11 -fvisibility=hidden -fPIC ${MSAN_AND_LIBCXX_FLAGS}'"
  18 | +export GOAL="install"
  19 | +export BITCOIN_CONFIG="--enable-wallet --with-sanitizers=memory --disable-asm --with-asm=no --prefix=${BASE_ROOT_DIR}/depends/x86_64-pc-linux-gnu/ CC=clang-8 CXX=clang++-8 CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' BDB_LIBS='-L${BDB_PREFIX}/lib -ldb_cxx-4.8' BDB_CFLAGS='-I${BDB_PREFIX}/include'"

MarcoFalke commented at 2:43 PM on March 10, 2020:

The file name indicates nowallet, this is --enable-wallet. Which is true?

practicalswift commented at 3:16 PM on March 10, 2020:

Thanks! The filename is wrong -- wallet is used. See contrib/install_db4.sh $(pwd) --enable-umrw below :)

--enable-umrw required to avoid UUM.

in ci/test/00_setup_env_native_nowallet_msan.sh:16 in a4a25a0f1a outdated

  11 | +LIBCXX_FLAGS="-nostdinc++ -stdlib=libc++ -L${LIBCXX_DIR}lib -lc++abi -I${LIBCXX_DIR}include -I${LIBCXX_DIR}include/c++/v1 -lpthread -Wl,-rpath,${LIBCXX_DIR}lib"
  12 | +export MSAN_AND_LIBCXX_FLAGS="${MSAN_FLAGS} ${LIBCXX_FLAGS}"
  13 | +export BDB_PREFIX="${BASE_ROOT_DIR}/db4"
  14 | +
  15 | +export CONTAINER_NAME="ci_native_nowallet_msan"
  16 | +export PACKAGES="clang-8 llvm-8 cmake"

MarcoFalke commented at 2:44 PM on March 10, 2020:

Any reason this needs clang-8, as opposed to just clang?

practicalswift commented at 3:14 PM on March 10, 2020:

I could not get it working reliably with a version older than Clang 8. I guess MSan has matured over the versions :)

in .travis.yml:128 in a4a25a0f1a outdated

 123 | @@ -124,6 +124,11 @@ jobs:
 124 |        env: >-
 125 |          FILE_ENV="./ci/test/00_setup_env_native_asan.sh"
 126 |  
 127 | +    - stage: test
 128 | +      name: 'x86_64 Linux  [GOAL: install]  [bionic]  [sanitizers: memory (MSan), no wallet]'

MarcoFalke commented at 2:44 PM on March 10, 2020:

      name: 'x86_64 Linux  [GOAL: install]  [bionic]  [depends, sanitizers: memory (MSan), no wallet]'

practicalswift commented at 3:19 PM on March 10, 2020:

Fixed!

in depends/packages/boost.mk:47 in a4a25a0f1a outdated

  42 | +  ./b2 -d2 -j2 -d1 --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) toolset=$($(package)_toolset_$(host_os)) stage
  43 |  endef
  44 |  
  45 |  define $(package)_stage_cmds
  46 | -  ./b2 -d0 -j4 --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) install
  47 | +  ./b2 -d0 -j4 --prefix=$($(package)_staging_prefix_dir) $($(package)_config_opts) toolset=$($(package)_toolset_$(host_os)) install

MarcoFalke commented at 2:47 PM on March 10, 2020:

This should probably be a separate pull?

practicalswift commented at 3:31 PM on March 10, 2020:

Sure! Submitted as #18308 :)

MarcoFalke commented at 2:49 PM on March 10, 2020: member

Concept ACK. I had issues getting valgrind run on non-x86 arches, so this might help in that regard. Not sure though if we should also run this on travis, given that travis hopefully will run valgrind on everything: #18304

practicalswift force-pushed on Mar 10, 2020

practicalswift commented at 3:38 PM on March 10, 2020: contributor

Not sure though if we should also run this on travis, given that travis hopefully will run valgrind on everything: #18304

Even if we assume that we get Valgrind to run all the unit and functional tests within the Travis time limit (which I unfortunately doubt is technically possible) I still don't see why we would not like to use both Valgrind and MSan in Travis? :)

As @gmaxwell put it: "more is more" :)

MSan is literally orders of magnitude faster than Valgrind and also gives better debugging info in case of presence of UUM.

Let me know if you can think of any problems that could arise from testing also under MSan in Travis.

MarcoFalke referenced this in commit 16dfaed1b7 on Mar 11, 2020

Sjors commented at 1:17 PM on March 12, 2020: member

Concept ACK. cd120cf71cb98f06fd779a0ccfa62afd8ab86c00 looks sane, doesn't hurt my macOS build, but otherwise a bit above my pay-grade. The changes are contained to their own machine, except:

boost depends tweak that's now moved to #18308
MSAN handling in GetOSRand, which is a more surgical alternative to #17627

For the libFuzzer+MSan combination to be available it is important to have the cumbersome MSan setup process documented and also kept up-to-date and working over time. We get that for free by having an MSan build in Travis.

Keeping this up to date, by having it run on Travis, seems useful. We can always triage Travis resources if funding runs dry.

The build took 1 hours and 15 minutes the first time. You could push a trivial commit to see much of that is shaved off by the cache.

Slow Travis builds can be a burden on review, because Github won't show a build as failed until all Travis machines of the current phase are done. I often find myself reviewing PRs in EU morning with trivial bugs not spotted by US devs before they went to be.

This can be mitigated if you add a new stage grind(?) and move the machine there. (I also agree with @gmaxwell about developers not using enough tooling, guilty as charged)

It could also run as part of a cron job on master ("$TRAVIS_EVENT_TYPE" = "cron"), but I'm not sure who gets notified when those fail. And it's better to catch stuff before merge, so let's not do that unless we run short on resources.

practicalswift commented at 10:57 PM on March 12, 2020: contributor

The build took 1 hours and 15 minutes the first time.

That seems to be roughly the time it takes also with caching. That might sound long but it is still 45 minutes quicker than the current valgrind job which only covers a subset of the functional tests whereas this one covers all of them :)

(To be clear: even with the two hour valgrind job I think it easily is worth taking that cost in terms of Travis build time to protect our users from at least a subset of UUM bugs. This kind of (partial) protection is long-overdue :))

practicalswift force-pushed on Mar 12, 2020

Sjors commented at 5:02 PM on March 13, 2020: member

Moving this and the valgrind task to a separate Travis phase can be experimented with in another PR.

in ci/test/04_install.sh:113 in cd120cf71c outdated

 107 | @@ -108,6 +108,13 @@ export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/
 108 |  
 109 |  DOCKER_EXEC mkdir -p "${BASE_SCRATCH_DIR}/sanitizer-output/"
 110 |  
 111 | +if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
 112 | +  DOCKER_EXEC mkdir -p "${BASE_SCRATCH_DIR}/msan/build/"
 113 | +  DOCKER_EXEC git clone https://github.com/llvm/llvm-project "${BASE_SCRATCH_DIR}/msan/llvm-project"

MarcoFalke commented at 5:04 PM on March 13, 2020:

Just testing locally and it takes a long time to download all of this, mostly it times out. Would it be possible to set --depth=1?

practicalswift commented at 7:40 PM on March 13, 2020:

Oh, of course! Thanks! Now updated.

MarcoFalke commented at 6:12 PM on March 13, 2020: member

Running this on odroid c2, gave me:

...


ranlib libdb-4.8.a
rm -f libdb.a
ln -s libdb-4.8.a libdb.a
./libtool --mode=compile clang++-8 -c -I. -I/root/workspace/bitcoin-core/db4/db-4.8.30.NC/dist/..  -D_GNU_SOURCE -D_REENTRANT -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -stdlib=libc++ -L/root/workspace/bitcoin-core/ci/scratch/msan/build/lib -lc++abi -I/root/workspace/bitcoin-core/ci/scratch/msan/build/include -I/root/workspace/bitcoin-core/ci/scratch/msan/build/include/c++/v1 -lpthread -Wl,-rpath,/root/workspace/bitcoin-core/ci/scratch/msan/build/lib /root/workspace/bitcoin-core/db4/db-4.8.30.NC/dist/../cxx/cxx_db.cpp
libtool: compile:  clang++-8 -c -I. -I/root/workspace/bitcoin-core/db4/db-4.8.30.NC/dist/.. -D_GNU_SOURCE -D_REENTRANT -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls -nostdinc++ -stdlib=libc++ -L/root/workspace/bitcoin-core/ci/scratch/msan/build/lib -lc++abi -I/root/workspace/bitcoin-core/ci/scratch/msan/build/include -I/root/workspace/bitcoin-core/ci/scratch/msan/build/include/c++/v1 -lpthread -Wl,-rpath,/root/workspace/bitcoin-core/ci/scratch/msan/build/lib /root/workspace/bitcoin-core/db4/db-4.8.30.NC/dist/../cxx/cxx_db.cpp  -fPIC -DPIC -o cxx_db.o
clang: warning: -lc++abi: 'linker' input unused [-Wunused-command-line-argument]
clang: warning: -lpthread: 'linker' input unused [-Wunused-command-line-argument]
clang: warning: -Wl,-rpath,/root/workspace/bitcoin-core/ci/scratch/msan/build/lib: 'linker' input unused [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-stdlib=libc++' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-L/root/workspace/bitcoin-core/ci/scratch/msan/build/lib' [-Wunused-command-line-argument]
In file included from /root/workspace/bitcoin-core/db4/db-4.8.30.NC/dist/../cxx/cxx_db.cpp:13:
./db_cxx.h:59:10: fatal error: 'iostream.h' file not found
#include <iostream.h>
         ^~~~~~~~~~~~
1 error generated.
Makefile:2018: recipe for target 'cxx_db.o' failed
make: *** [cxx_db.o] Error 1

practicalswift force-pushed on Mar 13, 2020

practicalswift commented at 3:10 PM on March 15, 2020: contributor

@MarcoFalke I wonder if that is MSan-related. Could you try if contrib/install_db4.sh (without MSan) works on your odroid c2? :)

MarcoFalke commented at 9:38 PM on March 16, 2020: member

rm -r db4/ && ./contrib/install_db4.sh `pwd` CC=clang-8 CXX=clang++-8

passed

practicalswift commented at 9:51 PM on March 16, 2020: contributor

@MarcoFalke I'd love to help out but looks hard to debug remotely and I've never tried compiling anything on under MSan on an odroid c2. Can you give the context: are we doing CI under odroid c2 in a way that makes it blocking? :)

MarcoFalke commented at 2:22 AM on March 17, 2020: member

As msan does a subset of what valgrind does, this is effectively testing valgrind, and not Bitcoin Core. Note that we already have a valgrind ci run.

The only issue I saw was when running the ci on non-x86, which spit out too many warnings under valgrind, so maybe msan is better suited for that use case.

I am not against adding this ci config, but I'd say to disable it for travis. At least for pull requests. I may be enabled for branch pushes.

DrahtBot commented at 7:40 AM on April 9, 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:

#19424 (ci: Run tsan ci config on cirrus by MarcoFalke)
#19179 ([WIP RFC DONOTMERGE] ci: Run ci configs on cirrus by MarcoFalke)

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 Needs rebase on Apr 9, 2020

practicalswift force-pushed on Apr 13, 2020

DrahtBot removed the label Needs rebase on Apr 13, 2020

practicalswift commented at 6:41 AM on April 28, 2020: contributor

@MarcoFalke

As msan does a subset of what valgrind does, this is effectively testing valgrind, and not Bitcoin Core. Note that we already have a valgrind ci run.

Are you sure the claim "valgrind is a strict subset of MSAN" holds also in the presence of compiler optimisations?

See the well-documented optimisation issues in the Valgrind documentation:

"If you are planning to use Memcheck: On rare occasions, compiler optimisations (at -O2 and above, and sometimes -O1) have been observed to generate code which fools Memcheck into wrongly reporting uninitialised value errors, or missing uninitialised value errors."

"We have looked in detail into fixing this, and unfortunately the result is that doing so would give a further significant slowdown in what is already a slow tool. So the best solution is to turn off optimisation altogether. Since this often makes things unmanageably slow, a reasonable compromise is to use -O. This gets you the majority of the benefits of higher optimisation levels whilst keeping relatively small the chances of false positives or false negatives from Memcheck. Also, you should compile your code with -Wall because it can identify some or all of the problems that Valgrind can miss at the higher optimisation levels. (Using -Wall is also a good idea in general.)"

#18785 appears to be a real-world example of this issue in our project.

Also note that MSAN is so much faster than Valgrind that it allows testing at lower optimization levels that would be practically impossible with Valgrind :)

Another reason to enable MSAN is that it allows for testing forks (we are currently disabling Valgrind in forks due to timeouts).

I hope we can enable MSAN in Travis: that would improve our testing without any major downside AFAICT :)

practicalswift commented at 2:28 PM on April 29, 2020: contributor

@MarcoFalke Following up on yesterday's addition on information: is there anything I can change in this PR to make it more appropriate for Travis? :)

Perhaps we can enable it for branch pushes to start with as you suggested in an earlier comment?

I think everything that can help us guard against uninitialised reads entering master is worth considering :)

MarcoFalke commented at 3:06 PM on April 29, 2020: member

Concept ACK, but this is waiting on the depends change to boost to go in first (and probably the non-ci related things as well)

practicalswift commented at 3:34 PM on April 29, 2020: contributor

@MarcoFalke Do you mean #18308 (friendly ping @dongcarl)? :)

fanquake commented at 12:07 PM on May 7, 2020: member

@practicalswift can you drop your boost changes here, and rebase on #18820? We'll get the macOS issues sorted out.

DrahtBot added the label Needs rebase on May 7, 2020

practicalswift force-pushed on Jun 1, 2020

DrahtBot removed the label Needs rebase on Jun 1, 2020

practicalswift commented at 9:01 AM on June 1, 2020: contributor

Updated now that the depends change to boost has been merged in to master.

This PR should be ready for final review.

It has previously received concept ACK:ish comments from MarcoFalke¹, gmaxwell² and Sjors³:

Concept ACK, but this is waiting on the depends change to boost to go in first (and probably the non-ci related things as well)¹

I don't see any problem with making more use of msan, though it's not a complete replacement for valgrind. But more is more. :)²

Concept ACK³

It should be noted that Valgrind testing has been removed from Travis (due to Valgrind being super slow) since this PR was first submitted.

fanquake added this to the "Blockers" column in a project

in ci/test/04_install.sh:99 in 5c6abdc563 outdated

  91 | @@ -92,6 +92,15 @@ export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/
  92 |  
  93 |  DOCKER_EXEC mkdir -p "${BASE_SCRATCH_DIR}/sanitizer-output/"
  94 |  
  95 | +if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
  96 | +  DOCKER_EXEC "update-alternatives --install /usr/bin/clang++ clang++ \$(which clang++-8) 100"
  97 | +  DOCKER_EXEC "update-alternatives --install /usr/bin/clang clang \$(which clang-8) 100"
  98 | +  DOCKER_EXEC "mkdir -p ${BASE_SCRATCH_DIR}/msan/build/"
  99 | +  DOCKER_EXEC "git clone --depth=1 https://github.com/llvm/llvm-project ${BASE_SCRATCH_DIR}/msan/llvm-project"

MarcoFalke commented at 10:45 AM on June 1, 2020:

Not sure if we want to compile a random commit from the default branch. Is msan required to be built from source? Would it help if you used the Ubuntu focal libcxx like in #19041?

practicalswift commented at 11:11 AM on June 1, 2020:

Pinning against a specific commit in the LLVM repo sounds like a good idea :)

Unfortunately I am not aware of any way to to achieve full MemorySanitizer instrumentation without linking against an MSan-instrumented C++ standard library, and I'm now aware of any way of doing so without compiling from source.

From various parts of the MemorySanitizer documentation: "Full MemorySanitizer instrumentation is very difficult to achieve." […] "If you want MemorySanitizer to work properly and not produce any false positives, you must ensure that all the code in your program and in libraries it uses is instrumented (i.e. built with -fsanitize=memory). In particular, you would need to link against MSan-instrumented C++ standard library. We recommend to use libc++ for that purpose."

practicalswift commented at 11:19 AM on June 1, 2020:

Now pinning against e5b8772756737e41cb1e8ee1a5a33cb3d8a25be6 (https://github.com/llvm/llvm-project/commit/e5b8772756737e41cb1e8ee1a5a33cb3d8a25be6) which happens to be the current master at the time of writing.

MarcoFalke commented at 11:21 AM on June 1, 2020:

Oh, so the Ubuntu focal libcxx does not ship with msan?

practicalswift commented at 11:34 AM on June 1, 2020:

Nope, I don't think so (in other words: it is not built with MSan-instrumentation AFAIK).

practicalswift commented at 2:14 PM on June 1, 2020:

Now pinning to v10.0.0 instead which feels less random :)

in ci/test/05_before_script.sh:29 in 95ca402bd9 outdated

  14 | @@ -15,6 +15,10 @@ fi
  15 |  
  16 |  DOCKER_EXEC mkdir -p ${DEPENDS_DIR}/SDKs ${DEPENDS_DIR}/sdk-sources
  17 |  
  18 | +if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
  19 | +  DOCKER_EXEC "contrib/install_db4.sh \$(pwd) --enable-umrw CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"

MarcoFalke commented at 11:49 AM on June 1, 2020:

Why is this needed and why can't the bdb from depends be used instead?

practicalswift commented at 12:21 PM on June 1, 2020:

I tried to build bdb from depends first but couldn't get it to work with MSan instrumentation, so I simply switched to using contrib/install_db4.sh which worked out of the box. Perhaps @dongcarl or some other expert on the depends system would succeed where I failed :)

Note that regardless of method of installation --enable-umrw is needed for MSan to work:

--enable-umrw

    Rational Software's Purify product and other run-time tools complain about
    uninitialized reads/writes of structure fields whose only purpose is padding,
    as well as when heap memory that was never initialized is written to disk.
    Specify the --enable-umrw argument during configuration to mask these
    errors. This argument should not be specified when configuring to build
    production binaries.

MarcoFalke commented at 12:48 PM on June 1, 2020:

It would be good to document the error you ran into when building bdb in depends. Otherwise the workaround will be left in the code lingering likely forever.

practicalswift commented at 12:55 PM on June 1, 2020:

Sure! I'll try to reproduce the issue: it was a while ago so I don't remember the details TBH :)

practicalswift commented at 3:26 PM on June 1, 2020:

Patch to reproduce:

diff --git a/ci/test/00_setup_env_native_msan.sh b/ci/test/00_setup_env_native_msan.sh
index f868142a2..41c788738 100644
--- a/ci/test/00_setup_env_native_msan.sh
+++ b/ci/test/00_setup_env_native_msan.sh
@@ -10,12 +10,11 @@ export LIBCXX_DIR="${BASE_ROOT_DIR}/ci/scratch/msan/build/"
 export MSAN_FLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls"
 LIBCXX_FLAGS="-nostdinc++ -stdlib=libc++ -L${LIBCXX_DIR}lib -lc++abi -I${LIBCXX_DIR}include -I${LIBCXX_DIR}include/c++/v1 -lpthread -Wl,-rpath,${LIBCXX_DIR}lib -Wno-unused-command-line-argument"
 export MSAN_AND_LIBCXX_FLAGS="${MSAN_FLAGS} ${LIBCXX_FLAGS}"
-export BDB_PREFIX="${BASE_ROOT_DIR}/db4"

 export CONTAINER_NAME="ci_native_msan"
 export PACKAGES="clang-8 llvm-8 cmake"
-export DEP_OPTS="NO_WALLET=1 NO_UPNP=1 NO_QT=1 NO_ZMQ=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' boost_cxxflags='-std=c++11 -fvisibility=hidden -fPIC ${MSAN_AND_LIBCXX_FLAGS}'"
+export DEP_OPTS="NO_UPNP=1 NO_QT=1 NO_ZMQ=1 CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' boost_cxxflags='-std=c++11 -fvisibility=hidden -fPIC ${MSAN_AND_LIBCXX_FLAGS}' bdb_config_opts='--disable-shared --enable-cxx --disable-replication --enable-option-checking --enable-umrw --with-pic'"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-wallet --with-sanitizers=memory --disable-asm --disable-zmq --with-asm=no --prefix=${BASE_ROOT_DIR}/depends/x86_64-pc-linux-gnu/ CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' BDB_LIBS='-L${BDB_PREFIX}/lib -ldb_cxx-4.8' BDB_CFLAGS='-I${BDB_PREFIX}/include'"
+export BITCOIN_CONFIG="--enable-wallet --with-sanitizers=memory --disable-asm --disable-zmq --with-asm=no --prefix=${BASE_ROOT_DIR}/depends/x86_64-pc-linux-gnu/ CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
 export USE_MEMORY_SANITIZER="true"
 export RUN_FUNCTIONAL_TESTS="false"
diff --git a/ci/test/05_before_script.sh b/ci/test/05_before_script.sh
index 17a97e22f..368550452 100755
--- a/ci/test/05_before_script.sh
+++ b/ci/test/05_before_script.sh
@@ -15,10 +15,6 @@ fi

 DOCKER_EXEC mkdir -p ${DEPENDS_DIR}/SDKs ${DEPENDS_DIR}/sdk-sources

-if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
-  DOCKER_EXEC "contrib/install_db4.sh \$(pwd) --enable-umrw CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
-fi
-
 if [ -n "$OSX_SDK" ] && [ ! -f ${DEPENDS_DIR}/sdk-sources/MacOSX${OSX_SDK}.sdk.tar.gz ]; then
   curl --location --fail $SDK_URL/MacOSX${OSX_SDK}.sdk.tar.gz -o ${DEPENDS_DIR}/sdk-sources/MacOSX${OSX_SDK}.sdk.tar.gz
 fi

Issue encountered:

  CXXLD    bitcoin-wallet
  AR       libtest_util.a
  CXXLD    bitcoind
  CXXLD    test/test_bitcoin
  CXXLD    bench/bench_bitcoin
libbitcoin_wallet.a(libbitcoin_wallet_a-db.o): In function `BerkeleyEnvironment::Verify(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)':
/home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/db.cpp:277: undefined reference to `Db::verify(char const*, char const*, std::__1::basic_ostream<char, std::__1::char_traits<char> >*, unsigned int)'
libbitcoin_wallet.a(libbitcoin_wallet_a-salvage.o): In function `RecoverDatabaseFile(boost::filesystem::path const&)':
/home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/salvage.cpp:54: undefined reference to `Db::verify(char const*, char const*, std::__1::basic_ostream<char, std::__1::char_traits<char> >*, unsigned int)'
/home/travis/build/practicalswift/bitcoin/depends/x86_64-pc-linux-gnu/share/../lib/libdb_cxx-4.8.a(cxx_db.o): In function `_verify_callback_c':
cxx_db.cpp:(.text+0x25e9): undefined reference to `std::basic_ostream<char, std::char_traits<char> >& std::operator<< <std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*)'
cxx_db.cpp:(.text+0x2603): undefined reference to `std::basic_ios<char, std::char_traits<char> >::fail() const'
/home/travis/build/practicalswift/bitcoin/depends/x86_64-pc-linux-gnu/share/../lib/libdb_cxx-4.8.a(cxx_db.o): In function `__cxx_global_var_init':
cxx_db.cpp:(.text.startup+0x10): undefined reference to `std::ios_base::Init::Init()'
cxx_db.cpp:(.text.startup+0x17): undefined reference to `std::ios_base::Init::~Init()'
…

practicalswift commented at 5:39 PM on June 1, 2020:

Comment added :)

practicalswift force-pushed on Jun 1, 2020

jonatack commented at 9:32 AM on June 15, 2020: member

Concept ACK

in contrib/install_db4.sh:67 in 98cd7a938f outdated

  63 | @@ -64,7 +64,7 @@ http_get() {
  64 |  
  65 |  mkdir -p "${BDB_PREFIX}"
  66 |  http_get "${BDB_URL}" "${BDB_VERSION}.tar.gz" "${BDB_HASH}"
  67 | -tar -xzvf ${BDB_VERSION}.tar.gz -C "$BDB_PREFIX"
  68 | +tar -xzf ${BDB_VERSION}.tar.gz -C "$BDB_PREFIX"

fjahr commented at 4:14 PM on June 16, 2020:

This seems unrelated to the actual change? I guess it's fine to remove the verbose option, I am just curious if this was done on purpose.

practicalswift commented at 9:01 PM on June 16, 2020:

@fjahr This change is intentional: this PR uses contrib/install_db4.sh and the -v flag fills the CI output with an unnecessary file listing. That said: if anyone finds a need for the verbose flag (-v) here I'm happy to revert :)

practicalswift commented at 1:35 PM on June 17, 2020:

Removed this change to minimize the diff and avoid review distractions :)

fjahr commented at 4:16 PM on June 16, 2020: member

Concept ACK

Code looks good but I don't have extensive experience with the CI scripts so far.

practicalswift commented at 5:49 AM on June 17, 2020: contributor

@fanquake

@practicalswift can you drop your boost changes here, and rebase on #18820? We'll get the macOS issues sorted out.

That has been done :) Ready for final review? :)

in ci/test/00_setup_env_native_msan.sh:19 in ae5e9476d5 outdated

  14 | +
  15 | +export CONTAINER_NAME="ci_native_msan"
  16 | +export PACKAGES="clang-8 llvm-8 cmake"
  17 | +export DEP_OPTS="NO_WALLET=1 NO_UPNP=1 NO_QT=1 NO_ZMQ=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' boost_cxxflags='-std=c++11 -fvisibility=hidden -fPIC ${MSAN_AND_LIBCXX_FLAGS}'"
  18 | +export GOAL="install"
  19 | +export BITCOIN_CONFIG="--enable-wallet --with-sanitizers=memory --disable-asm --disable-zmq --with-asm=no --prefix=${BASE_ROOT_DIR}/depends/x86_64-pc-linux-gnu/ CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}' BDB_LIBS='-L${BDB_PREFIX}/lib -ldb_cxx-4.8' BDB_CFLAGS='-I${BDB_PREFIX}/include'"

fanquake commented at 7:54 AM on June 17, 2020:

--with-asm=no should be redundant if you're already passing --disable-asm. Although I'd be interested to know what issues you were seeing?

fanquake commented at 7:55 AM on June 17, 2020:

If depends is being built with NO_ZMQ=1, --disable-zmq shouldn't be needed. Was zmq skipped for a particular reason?

practicalswift commented at 2:43 PM on June 17, 2020:

Judging from my testing --disable-asm does not seem to imply --with-asm=no:

When building with --disable-asm but without --with-asm=no I'm getting the following:

==5175==WARNING: MemorySanitizer: use-of-uninitialized-value
    [#0](/bitcoin-bitcoin/0/) 0x55da72d1630e in secp256k1_fe_normalize_var /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/secp256k1/./src/field_5x52_impl.h:141:9
    [#1](/bitcoin-bitcoin/1/) 0x55da72d1630e in secp256k1_ecmult_odd_multiples_table_storage_var /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/secp256k1/./src/ecmult_impl.h:171
    [#2](/bitcoin-bitcoin/2/) 0x55da72d1630e in secp256k1_ecmult_context_build /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/secp256k1/./src/ecmult_impl.h:317
    [#3](/bitcoin-bitcoin/3/) 0x55da72d1630e in secp256k1_context_create /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/secp256k1/src/secp256k1.c:86
    [#4](/bitcoin-bitcoin/4/) 0x55da729d77e6 in ECCVerifyHandle::ECCVerifyHandle() /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/pubkey.cpp:294:36
    [#5](/bitcoin-bitcoin/5/) 0x55da729d7aff in (anonymous namespace)::ECCryptoClosure::ECCryptoClosure() /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/bitcoinconsensus.cpp:65:8
    [#6](/bitcoin-bitcoin/6/) 0x55da70a70319 in __cxx_global_var_init /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/bitcoinconsensus.cpp:70:17
    [#7](/bitcoin-bitcoin/7/) 0x55da70a70399 in _GLOBAL__sub_I_bitcoinconsensus.cpp /home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/script/bitcoinconsensus.cpp
    [#8](/bitcoin-bitcoin/8/) 0x55da7332448c in __libc_csu_init (/home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x297b48c)
    [#9](/bitcoin-bitcoin/9/) 0x7f525aa04b27 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b27)
    [#10](/bitcoin-bitcoin/10/) 0x55da70a74009 in _start (/home/travis/build/practicalswift/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0xcb009)

fanquake commented at 2:51 PM on June 17, 2020:

Right, --with-asm is an option for libsecp's configure. I assume there are issues with our assembly as well then?

practicalswift commented at 11:21 PM on June 17, 2020:

Without NO_ZMQ=1:

/usr/bin/ld: /home/travis/build/practicalswift/bitcoin/depends/x86_64-pc-linux-gnu/share/../lib/libzmq.a(src_libzmq_la-ctx.o): in function `zmq::thread_ctx_t::thread_ctx_t()':
ctx.cpp:(.text+0x2da): undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string()'
/usr/bin/ld: /home/travis/build/practicalswift/bitcoin/depends/x86_64-pc-linux-gnu/share/../lib/libzmq.a(src_libzmq_la-ctx.o): in function `zmq::ctx_t::terminate()':
ctx.cpp:(.text+0x866): undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::c_str() const'
/usr/bin/ld: /home/travis/build/practicalswift/bitcoin/depends/x86_64-pc-linux-gnu/share/../lib/libzmq.a(src_libzmq_la-ctx.o): in function `zmq::thread_ctx_t::set(int, int)':
ctx.cpp:(.text+0x1515): undefined reference to `std::__cxx11::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> >::basic_ostringstream()'
/usr/bin/ld: ctx.cpp:(.text+0x1524): undefined reference to `std::ostream::operator<<(int)'
/usr/bin/ld: ctx.cpp:(.text+0x1554): undefined reference to `std::__cxx11::basic_ostringstream<char, std::char_traits<char>, std::allocator<char> >::str() const'
/usr/bin/ld: ctx.cpp:(.text+0x1575): undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator=(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&)'
/usr/bin/ld: ctx.cpp:(.text+0x1588): undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string()'

practicalswift commented at 11:22 PM on June 17, 2020:

It seems like --with-asm=no is enough. Will drop --disable-asm.

fanquake commented at 4:49 AM on June 18, 2020:

Can you try passing the flags through to zeromq, same as you're doing with Boost? i.e: zeromq_cxxflags="-std=c++11 ${MSAN_AND_LIBCXX_FLAGS}". Doing that I can compile with zmq.

practicalswift commented at 10:27 AM on June 18, 2020:

Done!

practicalswift commented at 11:40 AM on June 18, 2020:

I'm afraid getting the same issue when passing zeromq_cxxflags :\

practicalswift commented at 6:10 PM on June 22, 2020:

Managed to get it working with ZeroMQ support. Thanks for helping out!

in ci/test/00_setup_env_native_msan.sh:16 in ae5e9476d5 outdated

  11 | +LIBCXX_FLAGS="-nostdinc++ -stdlib=libc++ -L${LIBCXX_DIR}lib -lc++abi -I${LIBCXX_DIR}include -I${LIBCXX_DIR}include/c++/v1 -lpthread -Wl,-rpath,${LIBCXX_DIR}lib -Wno-unused-command-line-argument"
  12 | +export MSAN_AND_LIBCXX_FLAGS="${MSAN_FLAGS} ${LIBCXX_FLAGS}"
  13 | +export BDB_PREFIX="${BASE_ROOT_DIR}/db4"
  14 | +
  15 | +export CONTAINER_NAME="ci_native_msan"
  16 | +export PACKAGES="clang-8 llvm-8 cmake"

fanquake commented at 7:59 AM on June 17, 2020:

Can we use clang-9 and llvm-9 here now?

MarcoFalke commented at 12:51 PM on June 17, 2020:

Might as well use focal and clang-10 to avoid another bump

practicalswift commented at 10:29 AM on June 18, 2020:

Now bumped to focal and clang-9/llvm-9. Ran in to some issues with clang-10 which I'll investigate later, but perhaps we can go with focal and clang-9 for now?

in src/random.cpp:316 in ae5e9476d5 outdated

 310 | @@ -304,6 +311,22 @@ void GetOSRand(unsigned char *ent32)
 311 |          } else {
 312 |              RandFailure();
 313 |          }
 314 | +    } else {
 315 | +#if defined(__has_feature)
 316 | +#if __has_feature(memory_sanitizer)

fanquake commented at 8:19 AM on June 17, 2020:

I don't like that we're patching our RNG to "work around" a tool. I have not tested it yet, but why couldn't we a sanitizer special case list, rather than this patch? If the issue is just with syscall, then the list won't be very long, as this is the only place we are using it in /src.

MarcoFalke commented at 12:08 PM on June 18, 2020:

Agree with this ^

fanquake commented at 11:30 AM on June 21, 2020:

I tried using a sanitizer special case list, https://github.com/fanquake/core-review/commit/9158e2d0d56b6aab86a8ee47eb8d8a6a70800c5a, and it seems to work ok.

practicalswift commented at 7:33 AM on June 23, 2020:

Good points. I also dislike patching non-testing code for testing purposes: I've now removed the patching.

Since MemorySanitizer (MSAN) does not support tracking memory initialization done by using the Linux getrandom I'm now opting out of getrandom usage by undefining HAVE_SYS_GETRANDOM.

That is done without touching anything in src/.

fanquake commented at 8:29 AM on June 17, 2020: member

Left a few comments. I also wrapped up these changes into a msan dockerfile for my own use. Will check that we can use a special case list rather than patching random.cpp

in ci/test/00_setup_env_native_msan.sh:9 in ae5e9476d5 outdated

   0 | @@ -0,0 +1,21 @@
   1 | +#!/usr/bin/env bash
   2 | +#
   3 | +# Copyright (c) 2020 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 LIBCXX_DIR="${BASE_ROOT_DIR}/ci/scratch/msan/build/"

MarcoFalke commented at 12:53 PM on June 17, 2020:

Could add some documentation which parts of the build system pick up this global?

MarcoFalke commented at 12:54 PM on June 17, 2020:

I guess none? So this wouldn't need to be a global?

practicalswift commented at 1:35 PM on June 17, 2020:

You're right: no longer a global. Good catch! :)

practicalswift force-pushed on Jun 17, 2020

practicalswift force-pushed on Jun 18, 2020

DrahtBot added the label Needs rebase on Jun 19, 2020

practicalswift force-pushed on Jun 21, 2020

DrahtBot removed the label Needs rebase on Jun 21, 2020

practicalswift force-pushed on Jun 23, 2020

practicalswift commented at 7:37 AM on June 23, 2020: contributor

@MarcoFalke @fanquake Thanks a lot for great and detailed feedback!

All feedback should now be addressed:

Dropped --disable-asm.
Now building with ZeroMQ (dropped NO_ZMQ=1 and --disable-zmq).
Bumped to clang-9 and llvm-9.
No longer patching src/random.cpp with MemorySanitizer annotations.
Removed unneccessary bash export of LIBCXX_DIR.

The current version of this PR only touches only .travis.yml and ci/test/.

Should be ready for final review :)

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

practicalswift force-pushed on Jun 23, 2020

MarcoFalke commented at 10:07 PM on June 24, 2020: member

ACK 870f0cd2a0534d54bba18190e9f024f88e832933

in ci/test/00_setup_env_native_msan.sh:17 in 870f0cd2a0

  12 | +LIBCXX_FLAGS="-nostdinc++ -stdlib=libc++ -L${LIBCXX_DIR}lib -lc++abi -I${LIBCXX_DIR}include -I${LIBCXX_DIR}include/c++/v1 -lpthread -Wl,-rpath,${LIBCXX_DIR}lib -Wno-unused-command-line-argument"
  13 | +export MSAN_AND_LIBCXX_FLAGS="${MSAN_FLAGS} ${LIBCXX_FLAGS}"
  14 | +export BDB_PREFIX="${BASE_ROOT_DIR}/db4"
  15 | +
  16 | +export CONTAINER_NAME="ci_native_msan"
  17 | +export PACKAGES="clang-9 llvm-9 cmake"

MarcoFalke commented at 2:09 PM on June 27, 2020:

export PACKAGES="clang-9 llvm-9 cmake"  # When changing the clang version, also adjust ci/test/04_install.sh

MarcoFalke commented at 2:27 PM on July 2, 2020: member

nit (for a follow-up)

MarcoFalke merged this on Jul 2, 2020

MarcoFalke closed this on Jul 2, 2020

laanwj removed this from the "Blockers" column in a project

practicalswift deleted the branch on Apr 10, 2021

vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021

MarcoFalke referenced this in commit 597ee30b5e on Mar 10, 2022

DrahtBot locked this on Aug 16, 2022