feat: enable msan [memory sanitizer] on CI - #7643
Closed
knst wants to merge 26 commits into
Closed
Conversation
…etect use of uninitialized memory BACKPORT NOTE: File ci/test/00_setup_env_native_msan.sh has NO_BDB instead NO_WALLET because bitcoin#19077 is already backported to dash core ----- 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: Co-authored-by: MarcoFalke <falke.marco@gmail.com>
BACKPORT NOTE: missing changes seems irrelevant ---- fa0422c ci: Add msan fuzz config (MarcoFalke) fa399a7 ci: Use clang-12 in msan task (MarcoFalke) fab3017 ci: Set BASE_SCRATCH_DIR early, so that it can be used in test configs (MarcoFalke) Pull request description: Similar to the valgrind config, this config is not run by any ci task in this repo, but it can be used by other repos or self-hosted infrastructure. ACKs for top commit: practicalswift: cr ACK fa0422c: patch looks correct Tree-SHA512: 2122ac0948978a7b952efc80d4aa3674b27d48c6166e0ce917c61ac4ee6b68d701a83e5f71ee6868c208885ee45aae409ca022ebcb23ccbe37819a8c36e34872 Co-authored-by: MarcoFalke <falke.marco@gmail.com>
…_msan.sh d48565d fix permissions on 00_setup_env_native_fuzz_with_msan (glozow) Pull request description: I think there was a silent merge conflict between bitcoin#21852 and bitcoin#21740? I have a [linter failure](https://cirrus-ci.com/task/5436849834426368): ``` File "ci/test/00_setup_env_native_fuzz_with_msan.sh" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 ci/test/00_setup_env_native_fuzz_with_msan.sh" (or remove the shebang line). ERROR: There were 1 failed tests in the lint-files.py lint test. Please resolve the above errors. ^---- failure generated from test/lint/lint-files.sh ``` ACKs for top commit: MarcoFalke: ACK d48565d Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fa3bbcf ci: Properly pass msan cflags (MarcoFalke) Pull request description: Uninstrumented libraries might cause false positives with msan. Fixes bitcoin#21632 ACKs for top commit: practicalswift: cr ACK fa3bbcf: patch looks correct fanquake: ACK fa3bbcf Tree-SHA512: 55af961f3d422074bed4eec15df6b8d03cf5d7687a2ca5ea04c2591c0292ad3749db821d6160ba0e4458f0dcf448f0b2f05cc496314839264ddc8b89cced9e14 Co-authored-by: fanquake <fanquake@gmail.com>
7e02adf ci: add missing sqlite_cflags to MSAN fuzz job (fanquake) 54d817c ci: remove boost_cxxflags from MSAN CIs (fanquake) Pull request description: No-longer needed after bitcoin#24301. Add missing sqlite_cflags. ACKs for top commit: MarcoFalke: cr ACK 7e02adf Tree-SHA512: df7588e1380661d2777f2d2d0ba25546eb25fb9e398e37d69d888f5e5250abbbeabc5b92df6a7a781743ad0b7ab5acb9b0110de247f8dbb1b1862b424bcbeadd Co-authored-by: MarcoFalke <falke.marco@gmail.com>
3566353 ci: remove compiled-but-unused BDB from MSAN job (fanquake) Pull request description: Self-compiled BDB was added to this job as opposed to using depends BDB [due to linking issues](bitcoin#18288 (comment)), however the compiled BDB is not actually used. Remove it for now, given we don't actually lose any coverage (note that BDB is also not currently used in the naitve MSAN fuzz job or for [OSS Fuzz](https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh#L32) builds). In future, we can use depends BDB, however introducing it now will cause false positives, which can be fixed by upgrading the versions of Clang / LLVM we use, however upgrading to those newer versions causes other issues, which appear in standard library code, and require more involved suppressing, which can be solved in a follow up or another PR i.e bitcoin#23008. Top commit has no ACKs. Tree-SHA512: 9e8fdd95246cafa27cda7bcf0641b428d4573f6748ecdf07cc6205a64351db22ba383ec943e88a69df3694ccb9f125d994b64345a4e44fb6fea4a014504760d1 Co-authored-by: MarcoFalke <falke.marco@gmail.com>
527eeaf ci: note why BDB is disabled for MSAN jobs (fanquake) d6c71b0 ci: remove explicit --enable-wallet from msan job (fanquake) Pull request description: Closes bitcoin#24703. Top commit has no ACKs. Tree-SHA512: 231a52a0a1f55ecabf5b4f816dbc9ff4bc349bf3a247939fc75fee95454aff9fde04c9723b620a24e5a7993bd9bad7de5de1b0fd3c6cacc6297b7a64606e3a29 Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fa73f8a ci: Use clang-12 and libcxx-12 for msan (MarcoFalke) Pull request description: Run the latest sanitizers to get the most implemented features ACKs for top commit: fanquake: ACK fa73f8a - `--disable-hardening` matches what was just added to oss-fuzz. Tree-SHA512: 2e533bb9273c97600176be2e41069a03f425aa586f9f32b8ed5f0c9844215a3a41e95a8edd58d044386e350807d6a1df09008a7da35428abd185a509ca71bd82 Co-authored-by: fanquake <fanquake@gmail.com>
54dd8f5 ci: use ci_exec_root for clang install (josibake) Pull request description: fixes a bug introduced in bitcoin#25900 ; see bitcoin#25900 (comment) the general idea of bitcoin#25900 was to use a non-root user as much as possible to avoid modifying the user's local filesystem. however, it appears the root user is needed to correctly install clang. ACKs for top commit: hebasto: ACK 54dd8f5, tested on Ubuntu 22.04. Tree-SHA512: beb01d4b6127fbba3c8d18e85cf7ec7d1b2ec93ea05c475ab51bcaa04ef1b0591d886f1a7e0732c5ae86806013f022c0b44027380d2b0cfb1bfdc843e40f99b4 Co-authored-by: MarcoFalke <falke.marco@gmail.com>
…efix` option properly BACKPORT NOTE: Taking that PR in full broke every target that needs depends. aarch64-linux fails at configure with configure: error: Boost is not available! ci/test/00_setup_env_aarch64.sh passes no --host and no depends paths; it sets HOST and leaves the rest to depends' config.site, which supplies host_alias, cross_compiling and with_boost. Moving --prefix off the depends prefix and dropping --bindir/--libdir removes what the cross builds rely on, so configure looks for a native Boost in a container that has none for arm64. The rewrite was not needed by the msan work either. The compiler reaches configure through the msan target's own BITCOIN_CONFIG. Upstream can use CONFIG_SITE with --prefix=$BASE_OUTDIR because its CI copies the tree into the container and its paths line up. Adopting that here is a prerequisite change in its own right - it is what would let Dash run a NO_DEPENDS=1 build the way upstream's asan job does - and it needs the depends and workflow paths sorted out first, not a drive-by inside an msan series. -------------- d3a8434 ci: remove --prefix from msan job (fanquake) 574e50a ci: Use `CONFIG_SITE` variable and `--prefix` option properly (Hennadii Stepanov) Pull request description: When running CI scripts locally, they attempt to use a `$DEPENDS_DIR/$HOST` directory even `NO_DEPENDS=1` is provided. This PR fixes this broken behavior. Top commit has no ACKs. Tree-SHA512: 5e83b921763e6d463e520bbee2ed1599e9f4de36668d19b23dd9d2d7e4441c415e275f588c585b72cadda8bfab5a938979acc1ee4963230aa47081785c741e98 Co-authored-by: fanquake <fanquake@gmail.com>
fa4e98c ci: Fix ci_native_fuzz_msan CONTAINER_NAME (MarcoFalke) Pull request description: This avoids a duplicate name with the other msan task, which will lead to errors when running locally: > Error: creating container storage: the container name "ci_native_msan" is already in use by 77350e26f9c36abbb601140cd0b485ead093ff118803c720ca8b10f6bdfa37d2. You have to remove that container to be able to reuse that name: that name is already in use ACKs for top commit: fanquake: ACK fa4e98c hebasto: ACK fa4e98c, I've verified that there are no other duplicated `CONTAINER_NAME`'s values. Tree-SHA512: f1b28b21302c0947912d642c12c2ccad236af6824fd27e68341baddedec24087af738f3226028a0eeb6e0fc7e9f90713fc680855eeb07adc113c4f6e8b03a545 Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
…ages a3a2bd9 ci: Drop no longer needed package-specific flags (Hennadii Stepanov) 071eef1 build: Propagate user-defined flags to host packages (Hennadii Stepanov) Pull request description: On master (4f8b1f8) `{CPP,C,CXX,LD}FLAGS` that are specified in the command line are not propagated to packages: ``` $ make --no-print-directory -C depends print-libevent_cxxflags CXXFLAGS=-some-fancy-flag libevent_cxxflags=-pipe -O2 ``` This PR: - propagates `{CPP,C,CXX,LD}FLAGS` to host packages: ``` $ make --no-print-directory -C depends print-libevent_cxxflags CXXFLAGS=-some-fancy-flag libevent_cxxflags= -some-fancy-flag ``` - does not propagate `{CPP,C,CXX,LD}FLAGS` to native packages: ``` $ make --no-print-directory -C depends print-native_b2_cxxflags CXXFLAGS=-some-fancy-flag native_b2_cxxflags= ``` - actually addresses the bitcoin#23551 (comment) ACKs for top commit: TheCharlatan: Code review ACK a3a2bd9 Tree-SHA512: 243d6b1b0e9c5de46debc36de62a77b6b4d6f638940fd530040c219956ec624e321b0c25290fed164e3a8c88befa7b97b20f765d7b9a428c269b3720f21da099 Co-authored-by: fanquake <fanquake@gmail.com>
6766715 test: LLVM/Clang 16 for MSAN jobs (fanquake) Pull request description: Similar to other CI infra changes we've made recently. Move to LLVM/Clang 16 for the MSAN jobs (which is currently using LLVM 12). See also: https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html#sanitizers: > `-fsanitize-memory-param-retval` is turned on by default. With `-fsanitize=memory`, passing uninitialized variables to functions and returning uninitialized variables from functions is more aggressively reported. `-fno-sanitize-memory-param-retval` restores the previous behavior. ACKs for top commit: dergoegge: utACK 6766715 Tree-SHA512: a105bd1bf7f4e3ede50bb119fd8ab7f308919dc46e093eb3e94351484d65a13220e2449c40d80b8103b9ac0f4b1c8ca29576ab83e2083c26b9d8060c5802b64d Co-authored-by: fanquake <fanquake@gmail.com>
BACKPORT NOTE: Changes in 01_base_install.sh is emitted; this file doesn't exist in dash core repo more over this code become obsole in the future commits. ---- 5763b23 ci: return to using Ubuntu 22.04 in MSAN jobs (fanquake) d3cbcbf ci: compile clang and compiler-rt in MSAN jobs (fanquake) 796bd1d ci: use LLVM 16.0.4 in MSAN jobs (fanquake) 883bc9f ci: remove extra CC & CXX from MSAN jobs (fanquake) 2d4f4b8 ci: standardize custom libc++ usage in MSAN jobs (fanquake) Pull request description: This reworks the MSAN CIs, to first compile Clang and compiler-rt (using GCC 12), and then, compile an MSAN instrumented libc++ using the just-built Clang 16. This fixes the `native_fuzz_with_msan` job, working around https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1005341, by not using the Debian provided Clang/LLVM. Also included are changes to streamline how we use our "custom libc++", according to upstream: https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#using-a-custom-built-libc, as well as other minor cleanups in the CI configs. An example job is currently running in the qa-assets repo: bitcoin-core/qa-assets#129 (https://cirrus-ci.com/task/4632561431871488). ACKs for top commit: dergoegge: utACK 5763b23 Tree-SHA512: 4f2a6e0b796bb1830b8346dd1e55eaa86a79037b8b4f16a336c1e29f4fc460acca2ecba076635459370bcbb4009333cb79d27ef1521c1fb5db7599cd5bdf558c Co-authored-by: fanquake <fanquake@gmail.com>
…N jobs BACKPORT NOTE: Changes in 01_base_install.sh is emitted; this file doesn't exist in dash core repo. More over this code become obsole in the future commits. ---- 8531e1e ci: Use DEBUG=1 in depends for MSAN jobs (fanquake) 800ddef ci: use LLVM 17.0.6 in MSAN jobs (fanquake) Pull request description: Switch to using LLVM 17.0.6 and `DEBUG=1` in MSAN CI jobs. ACKs for top commit: maflcko: lgtm ACK 8531e1e Tree-SHA512: 819889762aeb78f95c4f955978890c6d98884bed0c7ff97ec072f4c7c1119ee3e3268ccab795bb1c801d36a206e16c6c1195e7a2bc7af94b580d17e49c632161 Co-authored-by: fanquake <fanquake@gmail.com>
…ainers (bugfix) BACKPORT NOTE: In full it breaks the depends targets: configure: error: Boost is not available! As well it breaks msan job: ./ci/test/00_setup_env.sh: line 42: /ci_container_base/depends/config.guess: No such file or directory Upstream can hard-code BASE_ROOT_DIR=/ci_container_base because its CI starts its own container and rsyncs the read-only source tree into that path first; ci/test/04_install.sh still carries that step here. Dash's GitHub Actions workflows do not use it. They declare the image in the job's container: block and run ci/dash/*.sh directly, with the checkout left where actions/checkout put it, so /ci_container_base never exists and nothing sets DANGER_RUN_CI_ON_HOST to opt out. Everything derived from BASE_ROOT_DIR then points into a directory that is not there. DEPENDS_DIR is the one that shows: configure is given --prefix=$DEPENDS_DIR/$HOST, finds no share/config.site, and so never learns where depends put Boost, or that a cross build is a cross build. ------------- fafa17c ci: Use hard-coded root path for CI containers (MarcoFalke) fa084f5 ci: Only create folders when needed (MarcoFalke) fab2712 ci: Drop BASE_SCRATCH_DIR from LIBCXX_DIR (MarcoFalke) Pull request description: Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another. Fix this by using a single hard-coded root path *inside* the CI system containers. Steps to test: * Run the CI system: `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh` * Move the git folder: `pwd && cd .. && mv bitcoin_core_folder_1 bitcoin_core_folder_2 && cd ./bitcoin_core_folder_2 && pwd` * Run the CI system again: (same cmd as above) On master (error): ``` STRIPPROG="x86_64-w64-mingw32-strip" /bin/bash /bitcoin_core_folder_2/ci/scratch/build/bitcoin-x86_64-w64-mingw32/build-aux/install-sh -c -s ./src/qt/bitcoin-qt.exe ./release /bitcoin_core_folder_2/ci/scratch/build/bitcoin-x86_64-w64-mingw32/build-aux/install-sh: ./src/qt/bitcoin-qt.exe does not exist. make: *** [Makefile:1258: bitcoin-25.99.0-win64-setup.exe] Error 1 ``` On this pull: (pass). ACKs for top commit: fanquake: ACK bitcoin@fafa17c - somewhat tested. MSAN changes are the same as what we did for tidy. Tree-SHA512: 2ce693a3773c70fcfca062c2a6f0e5a16b94960b34a6145d10cee1a28f79154829d59d014465ccbb80e1cb9dcd5aa043729cee9afd2c4175b05e9bc945364b79 Co-authored-by: fanquake <fanquake@gmail.com>
BACKPORT NOTE: missing changes belongs to ci/test/01_base_install.sh which is not presented in Dash Core ---- faecf3a ci: Bump msan to llvm-18 (MarcoFalke) Pull request description: Last one: bitcoin#28476 ACKs for top commit: fanquake: ACK faecf3a - There is now a 18.1.2, but given it doesn't fix the instrumenting in libunwind, we don't need that here. I've tested that both jobs are now working on both arches. Tree-SHA512: 489c0b343bdc732687131317a570f3efbb18a3f548736d739da90d1a1e784df1dbb208c2da8a2a7740f27f961a841c477487a14c4d59910368f651225f0779b2 Co-authored-by: fanquake <fanquake@gmail.com>
a3485af ci: Drop duplicated compiler flags (Hennadii Stepanov) Pull request description: On the master branch @ 0d509ba, it is easy to check the _"Options used to compile and link"_ section in the `configure` script output and observe duplicated compiler flags. This PR cleans such cases up. ACKs for top commit: maflcko: re-ACK a3485af fanquake: ACK a3485af - no-longer a change in behaviour. Tree-SHA512: 7e644fcfad7be48af3b18edd2994c0c78a21ac3f9fff497724be80f74c9e859d156de15ca4024c5c50d1080435576ce63402b48aba5c2fd556e2ed7e318e0e34 Co-authored-by: fanquake <fanquake@gmail.com>
BACKPORT NOTE: changes in 01_base_install.sh is omitted here intentionally ---- 61641e2 ci: remove --with-asm usage (secp256k1) (fanquake) c7efee5 ci: use LLVM 18.1.3 in MSAN jobs (fanquake) Pull request description: Bumps LLVM to `18.1.3`: * Includes llvm/llvm-project#86201, which is useful as it removes the need to (possibly) apply a work around when running the CI locally. Drops `--with-asm=no` (only being passed to secp256k1) from the MSAN CI. New MSAN annotations were pulled in as part of bitcoin#29803. ACKs for top commit: maflcko: lgtm ACK 61641e2 hebasto: ACK 61641e2. Tree-SHA512: da51c9f08a9aacb9dd936c47ef47777a8c84234e4df5b9776647ac94ebe88084b5e7b8182af90cfa01ae183072f6ce5915b73825f66b2567214ab270b2ff7837 Co-authored-by: fanquake <fanquake@gmail.com>
08ff17d ci: disable _FORTIFY_SOURCE with MSAN (fanquake) Pull request description: By undefining `_FORTIFY_SOURCE` we can drop`--disable-hardening`. ACKs for top commit: maflcko: lgtm ACK 08ff17d hernanmarino: utACK 08ff17d . Relevant CI test seems to be working OK. Tree-SHA512: 948fd075aa648a7e34c37376fb913074ebc07d1c3cb0737d5fcbe7eac0b35c4152139773e4515ccb80f2d11b1ced6c6984da1757c2bcf8dd90e8ff6f664dae8e Co-authored-by: merge-script <fanquake@gmail.com>
fa0411e ci: Run functional tests in msan task (MarcoFalke) Pull request description: Now that the CI machines have a bit more CPU, it seems good to run the functional tests as well under msan. (Also, bump the llvm minor version) ACKs for top commit: TheCharlatan: ACK fa0411e Tree-SHA512: 0dbb2b934485ed54b8caafb5bcd96ddef87088b148dab72a584f721c398bb7fda4095fb720b9ad602dc71f8f40a1e0f29e1b08b2879b78b90b29d46604df36c3 Co-authored-by: merge-script <fanquake@gmail.com>
5228223 ci: remove MSAN getrandom syscall workaround (fanquake) d5e0691 random: switch to using getrandom() directly (fanquake) c2ba3f5 random: add [[maybe_unused]] to GetDevURandom (fanquake) c13c97d random: getentropy on macOS does not need unistd.h (fanquake) Pull request description: This requires a linux kernel of `3.17`+, which seems entirely reasonable. `3.17` went EOL in 2015, and the last supported `3.x` kernel (`3.16`) went EOL > 4 years ago, in 2020. For reference, the current oldest maintained kernel is `4.14` (released 2017, going EOL Jan 2024). Support for `getrandom()` (and `getentropy()`) was added to glibc `2.25` https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html: > * The getentropy and getrandom functions, and the <sys/random.h> header file have been added. and we already require `2.27` or later. All that being said, I don't think you would encounter a current day (+~6 months from now) system, running with kernel headers older than 3.17 (released 2014) but also having a glibc of 2.27+ (released 2018)? Removing this (our only) use of `syscall()` also means we can drop a workaround in our MSAN jobs. If this is merged, I'll drop the [same workaround in oss-fuzz](https://github.com/google/oss-fuzz/blob/25946a544856413d31d9cbb3a366a4aef5a8fd60/projects/bitcoin-core/build.sh#L49-L56). ACKs for top commit: josibake: ACK bitcoin@5228223 hebasto: ACK 5228223, I've tested build system changes on Ubuntu 22.04 and macOS Monterey 12.6.6 (x86_64). Tree-SHA512: cc978e08510c461b875ca8c08ae176b4519fa1108f0efd74dcb7474518945357e0184e54423282c9a496de195e4ddc3e221ee78623bd63e24c50cc86acdf32e2 Co-authored-by: fanquake <fanquake@gmail.com>
The preceding backports land upstream's msan config verbatim, which by the
end is CMake-shaped and assumes ci/test/01_base_install.sh. Neither holds
here, so adapt it:
- BITCOIN_CONFIG back to autotools. Upstream's last autotools form was
--with-sanitizers=memory CPPFLAGS='-U_FORTIFY_SOURCE' (bitcoin#27766);
everything after that is -DSANITIZERS= and CMAKE_*_FLAGS_DEBUG, which
only exist because CMake's Debug config would otherwise overwrite the
-g -O1 in MSAN_FLAGS. Autotools puts user CXXFLAGS last, so that whole
problem is absent.
- LIBCXX_DIR points at /cxx_build/, where the instrumented libc++ is
baked into the CI image, rather than at a scratch dir populated by
01_base_install.sh.
- HOST is set, since Dash's depends is always built explicitly.
- clang-19 to match LLVM_VERSION in the CI image.
- Drop CI_IMAGE_NAME_TAG and PACKAGES, which nothing reads under GitHub
Actions; the toolchain comes from the container.
Three options are Dash-specific and have no upstream counterpart:
--with-asm=no --with-backend=easy MSan cannot see through hand-written
assembly. Dash reaches it twice, via
secp256k1 and via relic's GMP
arithmetic backend, so both need a
pure C path. Upstream dropped
--with-asm in bitcoin#27699 because
it has no equivalent of relic.
--disable-mimalloc mimalloc seeds itself from
syscall(SYS_getrandom) in
mi_process_attach, before main().
MSan intercepts libc calls, not raw
syscalls, so every process aborted at
startup until this was selectable.
TEST_RUNNER_TIMEOUT_FACTOR is 15 rather than the default 4. MSan with
origin tracking is far slower than ASan, and the LLMQ tests time out
waiting on quorum formation well before anything is actually wrong.
MemorySanitizer reports every read of memory it did not watch being written, so an uninstrumented C++ standard library produces a stream of reports that cannot be distinguished from real findings. libc++ therefore has to be rebuilt with -fsanitize=memory. Upstream does this in ci/test/01_base_install.sh, which runs inside its docker build. Dash has no equivalent script, which is why the hunks touching it were dropped from the preceding backports. Their content is carried here instead, dashified into a Dockerfile so the result lands in a cached image layer keyed on the file's hash rather than being rebuilt on every run: bitcoin#27448 LIBCXX_HARDENING_MODE=debug bitcoin#27824 LLVM_TARGETS_TO_BUILD=Native rather than X86 bitcoin#27922 llvm-symbolizer available, so reports are symbolized bitcoin#27436 the LLVM version tracks the compiler in use fab2712 /cxx_build/ rather than a scratch directory Two details are load-bearing. Only libcxx and libcxxabi are built, with LIBCXXABI_USE_LLVM_UNWINDER=OFF, because libunwind mishandles exceptions under MSan (llvm/llvm-project#84348). And the LLVM tag matches the clang in the image; a mismatched libc++ ABI produces failures that look like sanitizer findings. Upstream compiled clang itself from source for a while (bitcoin#27737) to work around Debian packaging, and reverted that once it was fixed. The distro clang is used here for the same reason it is used upstream now. Also raise TEST_RUNNER_TIMEOUT_FACTOR to 40. MSan with origin tracking is several times slower than ASan, and locally 15 was not enough for p2p_quorum_data and feature_llmq_signing, which time out waiting on quorum formation well before anything is wrong.
bls.cpp already picks its secure allocator at compile time and falls back
from mimalloc to libsodium to plain malloc:
#if BLSALLOC_MIMALLOC
SetSecureAllocator(mi_malloc, mi_free);
#elif BLSALLOC_SODIUM
...
#else
SetSecureAllocator(malloc, free);
Only the build wiring was unconditional, so there was no way to select the
fallback. Add --disable-mimalloc, following the existing tests and bench
options, and guard the define, the sources and the LIBADD entry with the
resulting USE_MIMALLOC conditional. Default is unchanged, so nothing about
a normal build moves.
Needed because mimalloc cannot run under MemorySanitizer. mi_process_attach
runs from __libc_start_main before main(), and its entropy init calls
syscall(SYS_getrandom, ...) directly, with a /dev/urandom fallback that also
goes through raw syscalls. MSan intercepts libc functions rather than
syscalls, so the key buffer stays poisoned and every process aborts at
startup. There is no way out at runtime: MSan has no general suppression
mechanism, and halt_on_error=0, keep_going=1 and exitcode=0 all still
terminate. mimalloc's own track.h supports Valgrind and ASan but not MSan.
Note this belongs upstream in dashpay/bls-signatures; carrying it here is a
stopgap so the MSan job has something to build against.
Making mimalloc optional moved $(LIBMIMALLOC) from the front of
libdashbls_la_LIBADD to the end, inside the USE_MIMALLOC conditional. For
static archives that changes symbol resolution: mimalloc compiles
alloc-posix.c and runs mi_process_attach from a constructor, so it provides
allocator symbols of its own. Linked after librelic and gmp, some objects
resolve malloc and free to mimalloc and others to libc, and the process
aborts with "free(): invalid size" as soon as memory allocated by one is
released by the other.
The conditional was unnecessary anyway. LIBMIMALLOC and MIMALLOC_H are only
defined by Makefile.mimalloc.include, which is already included
conditionally, so both expand to nothing when mimalloc is disabled and the
original unconditional lines are correct as they stand.
Wires up the MSan target added by the preceding backports. Unlike the asan
job, this one cannot reuse anything:
- its own container, because the MSan-instrumented libc++ is only in
dashcore-ci-msan
- its own depends, because every dependency has to be compiled with
-fsanitize=memory. Sharing the linux64 depends would leave MSan
reporting uninitialised reads out of libevent, sqlite and zeromq that
are indistinguishable from real findings.
Both build and test jobs run, matching upstream since bitcoin#31592.
Gated on SKIP_LINUX64_MSAN so a repository can turn it off, and the
container job is gated too, since building the instrumented libc++ is the
expensive part and there is no reason to pay for it when the job is off.
Not included: upstream sets vm.mmap_rnd_bits=28 on the runner for its MSan
and TSan jobs, to stop high ASLR entropy breaking the sanitizer's shadow
mappings. That cannot be done here. vm.* sysctls are not namespaced, so it
is not settable from a Dockerfile, is rejected by docker --sysctl, and would
need a privileged container mutating the host to apply at runtime.
It was not needed locally on a 6.12 kernel, so nothing is done for now. If
CI does hit shadow mapping failures, the fix is to wrap the test invocation
in setarch -R, which disables ASLR for that process tree only and needs no
privileges and no host changes.
This was referenced Aug 27, 2026
Collaborator
Author
|
Due to low performance of asan [address sanitizer] and extra load for our CI queue, enabling of msan is postponed until we will have bigger resources / faster CI or msan's jobs suddenly will become much faster. Closed as DNM but it works as tested. #7642 is mergeable part of this PR. |
|
🕓 Ready for review — 1 ahead in queue (commit 96abb12) |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
WalkthroughThe PR adds Linux x86_64 MemorySanitizer CI coverage with dedicated container, dependency, source, and test jobs. It makes arm64 container image builds optional and updates manifest creation for single-architecture builds. It adds configurable mimalloc support to dashbls. It changes Linux entropy detection and acquisition to use libc random APIs. It documents Linux Kernel requirements. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96abb12826
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with: | ||
| build-target: linux64_msan | ||
| container-path: ${{ needs.container-msan.outputs.path }} | ||
| base-image-digest: ${{ needs.check-skip.outputs.base-image-digest }} |
There was a problem hiding this comment.
When contrib/containers/ci/ci-msan.Dockerfile changes without changing the generic CI Dockerfiles or Ubuntu base image, this job can restore a stale linux64_msan depends prefix: the cache key in .github/workflows/build-depends.yml hashes only ci.Dockerfile and ci-slim.Dockerfile. That can mix dependencies compiled against an earlier instrumented libc++ with the current /cxx_build runtime, causing ABI failures or undermining MSan coverage; incorporate the MSan Dockerfile hash or image digest into this target's cache key.
AGENTS.md reference: AGENTS.md:L227-L229
Useful? React with 👍 / 👎.
| if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then | ||
| CI_EXEC_ROOT "update-alternatives --install /usr/bin/clang++ clang++ \$(which clang++-12) 100" | ||
| CI_EXEC_ROOT "update-alternatives --install /usr/bin/clang clang \$(which clang-12) 100" | ||
| DOCKER_EXEC "mkdir -p ${BASE_SCRATCH_DIR}/msan/build/" |
There was a problem hiding this comment.
When either new MSan environment file is run through ci/test_run_all.sh, execution reaches this block with USE_MEMORY_SANITIZER=true, but DOCKER_EXEC is not defined anywhere in the repository; with the runner's errexit, the first invocation terminates setup before libc++ is built. Replace these stale calls with the available CI_EXEC helper or remove the obsolete build block now that the MSan image supplies libc++.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Bitcoin Core has msan [memory sanitizer] on CI.
It depends on #7642
This PR meant to bring msan to Dash Core. Though, due to low performance of asan [address sanitizer] and extra load for our CI queue, enabling of msan is postponed until we will have bigger resources / faster CI or msan's jobs suddenly will become much faster].
What was done?
Created 2 new jobs with msan.
How Has This Been Tested?
This PR is not meant to be merged currently.
It has been tested on fork of repo: It has been tested on CI with fork of repo https://github.com/knst/dash/actions/runs/33092725515/job/98611087267
Breaking Changes
N/A
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.