ci: replace UBSAN job by ASAN and add extra sanitizers: detect_leaks=1:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1 - #7503
Conversation
Mainstream it has been done in bitcoin#14673 but not properly backported so far as we hadn't use travis Sanitizers that cannot find it print raw addresses instead of source locations, and a suppression file cannot match a stack of raw addresses. Current CI image registers update-alternatives slaves for llvm-ar, llvm-cov, llvm-nm, llvm-objdump, llvm-ranlib and llvm-strip, but not llvm-symbolizer, so /usr/bin/llvm-symbolizer does not exist. That fixes issue discovered in dashpay#5379: ********* 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>)
Mainstream it has been done in bitcoin#15022 and later in bitcoin#16582 but hasn't been properly backported. LeakSanitizer suspends every thread through ptrace to walk the heap. In a container without CAP_SYS_PTRACE that call fails and the process dies before any leak is reported, so the job fails without saying why. ci/test/04_install.sh already encodes this for the legacy local path, but GitHub Actions builds and runs the unit tests inside the container declared by these two reusable workflows, which never granted the capability.
…p256k1` code cbea49c build: Pass sanitize flags to instrument `libsecp256k1` code (Hennadii Stepanov) Pull request description: This PR is a revived bitcoin#27991 with an addressed [comment](bitcoin#27991 (comment)). Fixes bitcoin#27990. Might be tested as follows: ``` $ ./autogen.sh && ./configure --enable-fuzz --with-sanitizers=fuzzer CC=clang-13 CXX=clang++-13 $ make clean > /dev/null && make $ objdump --disassemble=secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep __sanitizer_cov 1953bd0:e8 bb c6 05 ff call 9b0290 <__sanitizer_cov_trace_const_cmp8> 1953d32:e8 69 c4 05 ff call 9b01a0 <__sanitizer_cov_trace_pc_indir> 1953d58:e8 43 c4 05 ff call 9b01a0 <__sanitizer_cov_trace_pc_indir> 1953d82:e8 19 c4 05 ff call 9b01a0 <__sanitizer_cov_trace_pc_indir> ``` ACKs for top commit: fanquake: ACK cbea49c dergoegge: reACK cbea49c Tree-SHA512: 801994e75b711d20eaf0d675f378da07d693f4a7de026efd93860f5f1deabed855a83eca3561725263e4fe605fcc5f91eb73c021ec91c831864e6deb575e3885 (cherry picked from commit e3b68b3) Co-authored-by: fanquake <fanquake@gmail.com>
5be31b2 lsan: add more Qt suppressions (fanquake) Pull request description: Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and: ```bash export CC=clang export CXX=clang++ cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address cmake --build build export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan" ctest --test-dir build ``` ```bash Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms ********* Finished testing of AddressBookTests ********* ================================================================= ==21869==ERROR: LeakSanitizer: detected memory leaks Direct leak of 88 byte(s) in 1 object(s) allocated from: #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872) #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa) #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa) #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18 #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5 #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75 #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13 #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5 #8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5 #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7) dashpay#10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7) dashpay#11 0xffff8cf57c54 (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) dashpay#12 0xffff8cf5fa18 (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) dashpay#13 0xffff8cf6067c (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) dashpay#14 0xffff8cf610a4 (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) dashpay#15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) dashpay#16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) dashpay#17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30 dashpay#18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) dashpay#19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) dashpay#20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872) ``` This happens when building using depends: ```bash Indirect leak of 24 byte(s) in 1 object(s) allocated from: #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #1 0xfbff97f8c164 (<unknown module>) #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) dashpay#10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) dashpay#11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) dashpay#12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) dashpay#13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) dashpay#14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o dashpay#15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s). ``` ACKs for top commit: maflcko: lgtm ACK 5be31b2 Tree-SHA512: 0c33661c7ec83ea9b874c1ee4ee2de513131690287363e216a88560dfb31a59ef563a50af756c86a991583aa64a600a74e20fd5d6a104cf4c0a27532de8d2211 (cherry picked from commit 67e6746) Co-authored-by: merge-script <fanquake@gmail.com>
5161c26 ci: add second_deadlock_stack=1 to TSAN options (fanquake) Pull request description: This is mentioned in the developer notes, but isn't present in `TSAN_OPTIONS`, resulting in: ```bash WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508) Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0 <snip> Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message ``` Add it, for (potentially) more informative output, when failures occur. Checked that adding does output more information. ACKs for top commit: maflcko: lgtm ACK 5161c26 hebasto: ACK 5161c26. Tree-SHA512: 57bfa24d248ed76ba0db537edff425453a0765c4768fc1b6f59a87876d4acf63ed38bb3c20f369a008ae256472d9d24e58d76729d423f662dfdb2952afc46cb0 Co-authored-by: merge-script <fanquake@gmail.com>
Original changes has been added to ci/test/04_install.sh and hasn't been propogated to matrix.sh where it's actually used
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
🔍 Review in progress — actively reviewing now (commit d996850) |
WalkthroughThe changes replace Linux64 UBSAN build and test jobs with ASAN jobs. They configure ASAN runtime checks and native compiler dependencies. CI containers gain the Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant depends-linux64
participant src-linux64_asan
participant test-linux64_asan
GitHubActions->>depends-linux64: Evaluate SKIP_LINUX64_ASAN
depends-linux64->>src-linux64_asan: Provide linux64 dependency outputs
src-linux64_asan->>test-linux64_asan: Provide ASAN build bundle
test-linux64_asan->>GitHubActions: Run and report ASAN tests
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: e80afc7a20
ℹ️ 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".
| [SANITIZER_CXXFLAGS="-fsanitize=$use_sanitizers" | ||
| SANITIZER_CFLAGS="-fsanitize=$use_sanitizers"], |
There was a problem hiding this comment.
This assigns SANITIZER_CFLAGS, but it still is not substituted or consumed by the main C build: src/Makefile.am keeps AM_CFLAGS without sanitizer flags, and the X11 SPH target overrides CFLAGS with only $(SPHLIB_FLAGS). In the new linux64_asan CI job this means Dash C hashing objects are linked into sanitized binaries without ASan/UBSan instrumentation, leaving that Dash-specific path blind while replacing the UBSan job; please propagate the C sanitizer flags to the relevant AM_CFLAGS/per-target CFLAGS as well.
AGENTS.md reference: AGENTS.md:L150-L152
Useful? React with 👍 / 👎.
| with: | ||
| bundle-key: ${{ needs.src-linux64_asan.outputs.key }} | ||
| build-target: linux64_asan | ||
| container-path: ${{ needs.container-slim.outputs.path }} |
There was a problem hiding this comment.
The new ASan functional-test job runs through test-src.yml in container-slim, but the only image updated to register an unversioned /usr/bin/llvm-symbolizer is contrib/containers/ci/ci.Dockerfile; ci-slim.Dockerfile still lacks the matching alternative setup. When ASan/LSan/UBSan reports in these functional tests, the runtime can fall back to raw addresses, which makes suppression matching unreliable and leaves the new job noisy or failing; use the full CI image here or add the same symbolizer setup to the slim image.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The advertised sanitizer backports and CI migration are otherwise consistent, with no blocking correctness issues found. One non-blocking gap remains: the new ASAN functional-test job uses the slim container, but the advertised bitcoin#14673 symbolizer repair is applied only to the full build container.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `contrib/containers/ci/ci.Dockerfile`:
- [SUGGESTION] contrib/containers/ci/ci.Dockerfile:51-52: Missing bitcoin#14673 counterpart in the slim test image
The bitcoin#14673 repair registers the unversioned `llvm-symbolizer` command only in the full CI image. The newly added `test-linux64_asan` job runs in the separately built `ci-slim.Dockerfile` image, where `llvm-19-dev` is installed but neither `/usr/lib/llvm-19/bin` is added to `PATH` nor an unversioned alternative is created. LLVM 19's sanitizer runtime searches `PATH` specifically for `llvm-symbolizer`, so ASAN processes launched by the functional tests cannot find the versioned executable. Their reports will contain raw addresses, and symbol-based leak suppressions cannot match those stacks. Add the equivalent symbolizer setup to `ci-slim.Dockerfile`, or export an explicit path to `/usr/bin/llvm-symbolizer-19` from sanitizer configuration shared by both images.
| for binName in clang clang++ clang-apply-replacements clang-format clang-tidy clangd dsymutil lld lldb lldb-server llvm-ar llvm-cov llvm-nm llvm-objdump llvm-ranlib llvm-strip llvm-symbolizer run-clang-tidy; do \ | ||
| llvmUpdAltArgs="${llvmUpdAltArgs} --slave /usr/bin/${binName} ${binName} /usr/bin/${binName}-${LLVM_VERSION}"; \ |
There was a problem hiding this comment.
🟡 Suggestion: Missing bitcoin#14673 counterpart in the slim test image
Commit f71735d ("fix: make sanitizer reports symbolizable") adds llvm-symbolizer to the update-alternatives loop here, explicitly framed as finishing an incomplete backport of upstream bitcoin#14673. That fixes symbolization for anything built/run in the full CI image (ci.Dockerfile, used by src-linux64_asan in build-src.yml). But .github/workflows/build.yml's test-linux64_asan job (lines 345-352) runs the functional-test half of the ASAN job in container-slim — built from ci-slim.Dockerfile — which installs llvm-19-dev but never runs the update-alternatives step or exports PATH/ASAN_SYMBOLIZER_PATH to expose any llvm-symbolizer binary (versioned or not). ASAN/LSan look for an unversioned llvm-symbolizer on PATH by default, so functional-test sanitizer failures under the new job will still print raw addresses instead of symbolized stacks, and suppression-file entries that rely on symbol names (e.g. the newly-added LSan Qt suppressions from bitcoin#32780) cannot match those stacks. This is the same root cause codex-general independently flagged at ci/dash/matrix.sh line 14 (proposing an explicit ASAN_SYMBOLIZER_PATH export there as a workaround) — the actual gap is architectural: the fix landed only in the build container, not the test-execution container. The real fix belongs in ci-slim.Dockerfile (add the llvm-symbolizer alternative there too, or export an explicit symbolizer path valid in both images) rather than in ci.Dockerfile itself; the finding is anchored here because this is where the (incomplete) fix was applied.
source: ['codex']
|
|
||
| # Configure sanitizers options | ||
| export ASAN_OPTIONS="" | ||
| export ASAN_OPTIONS="detect_leaks=1:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1" |
There was a problem hiding this comment.
Retracted — not actionable for this PR
The cumulative verifier recheck confirmed this belongs to the separate, unchanged fuzz path. PR #7503 does not advertise bitcoin#30665 as a backport, and leak detection remains enabled by default there, so requiring this hunk would expand the PR into adjacent work. No change is requested. The final review summary has been corrected.
…n CI before PR is merged
Reworked the env file for the GitHub Actions flow:
- NO_DEPENDS=1 is gone. ci/dash/build_src.sh always configures against
depends and ignores it, so leaving it in only misleads.
- DEP_OPTS is empty to match the linux64 depends this job reuses.
build-src.yml takes DEP_OPTS from the depends build rather than from
the build target, so a different value here would never take effect.
- Pinned clang-19 to match LLVM_VERSION in the CI image instead of
unversioned clang.
- Picked up -ftrivial-auto-var-init=pattern and -DARENA_DEBUG from
upstream v28.0, the last release before the CMake migration and so
the last one directly comparable to our autotools build.
- Dropped RUN_BENCH and FUNCTIONAL_TESTS_CONFIG, which nothing reads.
The job reuses the linux64 depends artifact, so it costs a source build
and a test run, not a dependency build.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d996850944
ℹ️ 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".
| # TODO: remove it when #7503 will get merged. That's a temporary workaround to check asan on CI | ||
| # Compatibility for pull_request_target workflows that still request the legacy target. | ||
| # Their default-branch container setup does not grant SYS_PTRACE. | ||
| export ASAN_OPTIONS="detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1" |
There was a problem hiding this comment.
In the legacy linux64_ubsan compatibility path used by pull_request_target workflows still coming from the default branch, the nearby comment says those containers lack SYS_PTRACE, but this override only removes the explicit detect_leaks=1. On Linux, ASan still enables LeakSanitizer by default when detect_leaks is unset, so these legacy jobs can still hit LSan's stop-the-world ptrace requirement and fail or produce noisy reports; set detect_leaks=0 here or ensure the legacy workflow grants the capability.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/dash/matrix.sh (1)
14-16: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve
second_deadlock_stack=1in the final TSan environment.
ci/dash/matrix.shsourcesci/test/04_install.sh, andci/test/04_install.shlater assignsTSAN_OPTIONSwithout this option. Addsecond_deadlock_stack=1to that final assignment or append it so the TSan job keeps the diagnostic setting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/dash/matrix.sh` around lines 14 - 16, Update the final TSAN_OPTIONS assignment in ci/test/04_install.sh, which overrides the value from ci/dash/matrix.sh, to include second_deadlock_stack=1. Preserve the existing TSan options and ensure the resulting environment retains this diagnostic setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/dash/matrix.sh`:
- Around line 36-40: Update the legacy linux64_ubsan fallback’s ASAN_OPTIONS
before sourcing 00_setup_env_native_asan.sh to include detect_leaks=0, while
preserving the existing ASAN options and setup flow.
---
Outside diff comments:
In `@ci/dash/matrix.sh`:
- Around line 14-16: Update the final TSAN_OPTIONS assignment in
ci/test/04_install.sh, which overrides the value from ci/dash/matrix.sh, to
include second_deadlock_stack=1. Preserve the existing TSan options and ensure
the resulting environment retains this diagnostic setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb2438e6-5b08-4510-add6-cc7b15f53632
📒 Files selected for processing (4)
.github/workflows/build.ymlci/dash/matrix.shci/test/00_setup_env_native_asan.shci/test/00_setup_env_native_ubsan.sh
💤 Files with no reviewable changes (1)
- ci/test/00_setup_env_native_ubsan.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
| # TODO: remove it when #7503 will get merged. That's a temporary workaround to check asan on CI | ||
| # Compatibility for pull_request_target workflows that still request the legacy target. | ||
| # Their default-branch container setup does not grant SYS_PTRACE. | ||
| export ASAN_OPTIONS="detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1" | ||
| source ./ci/test/00_setup_env_native_asan.sh |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## files"
git ls-files | rg '(^|/)ci/(dash/matrix\.sh|test/0(0|4)_setup_env_native_asan\.sh|test/04_install\.sh)$' || true
echo "## matrix.sh relevant lines"
if [ -f ci/dash/matrix.sh ]; then
nl -ba ci/dash/matrix.sh | sed -n '1,90p'
fi
echo "## setup native asan relevant lines"
if [ -f ci/test/00_setup_env_native_asan.sh ]; then
nl -ba ci/test/00_setup_env_native_asan.sh | sed -n '1,180p'
fi
echo "## install.sh relevant lines"
if [ -f ci/test/04_install.sh ]; then
rg -n "ASAN_OPTIONS|SYS_PTRACE|LSan|leak|detect_leaks|ptrace" ci/test/04_install.sh -C 3
fi
echo "## tracked workflow refs"
rg -n "pull_request_target|ASAN_OPTIONS|detect_leaks|detect_stack_use_after_return|SYS_PTRACE|00_setup_env_native_asan|04_install" ci -C 2 || trueRepository: dashpay/dash
Length of output: 303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## matrix.sh relevant lines"
if [ -f ci/dash/matrix.sh ]; then
awk '{printf "%6d\t%s\n", NR, $0}' ci/dash/matrix.sh | sed -n '1,90p'
fi
echo "## setup native asan relevant lines"
if [ -f ci/test/00_setup_env_native_asan.sh ]; then
awk '{printf "%6d\t%s\n", NR, $0}' ci/test/00_setup_env_native_asan.sh | sed -n '1,220p'
fi
echo "## install.sh relevant lines"
if [ -f ci/test/04_install.sh ]; then
awk '{printf "%6d\t%s\n", NR, $0}' ci/test/04_install.sh | rg -n "ASAN_OPTIONS|SYS_PTRACE|LSan|leak|detect_leaks|ptrace|ASAN" -C 3 || true
fi
echo "## tracked workflow refs"
rg -n "pull_request_target|ASAN_OPTIONS|detect_leaks|detect_stack_use_after_return|SYS_PTRACE|00_setup_env_native_asan|04_install" ci -C 2 || trueRepository: dashpay/dash
Length of output: 8915
🌐 Web query:
Clang AddressSanitizer leak detection default on Linux detect_leaks
💡 Result:
When using Clang's AddressSanitizer (ASan) on Linux, leak detection is enabled by default [1][2]. This integration is provided by LeakSanitizer (LSan), which runs automatically at the end of the process to detect memory leaks [3][4]. Key points regarding this behavior: - Default Status: On Linux, you do not need to set any specific flags to enable leak detection when using ASan; it is active automatically [1][4]. - Disabling Leak Detection: If you wish to disable this feature while still using AddressSanitizer, you can set the environment variable ASAN_OPTIONS=detect_leaks=0 [4]. - Platform Support: While leak detection is enabled by default on Linux, it is not supported on all platforms where ASan is available [1][2]. For example, on macOS, it must be explicitly enabled using ASAN_OPTIONS=detect_leaks=1 [1][4]. - Configuration: You can further tune the behavior of the leak detector using the LSAN_OPTIONS environment variable [4]. If you are using LeakSanitizer in a stand-alone mode (without the full AddressSanitizer), you must compile and link your program with the -fsanitize=leak flag [3][5].
Citations:
- 1: https://clang.llvm.org/docs/AddressSanitizer.html
- 2: https://llvm.googlesource.com/llvm-project/+/9a553d3766aacb69e884823da92dedff264e3f0f/clang/docs/AddressSanitizer.rst
- 3: https://clang.llvm.org/docs/LeakSanitizer.html
- 4: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
- 5: https://releases.llvm.org/17.0.1/tools/clang/docs/LeakSanitizer.html
Disable LeakSanitizer in the legacy linux64_ubsan fallback.
This path skips the normal ASAN path and does not add SYS_PTRACE, while leak detection is enabled by default for AddressSanitizer on Linux. Set detect_leaks=0 before sourcing the ASAN environment setup:
Proposed fix
- export ASAN_OPTIONS="detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1"
+ export ASAN_OPTIONS="detect_leaks=0:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1"🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 40-40: Not following: ./ci/test/00_setup_env_native_asan.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/dash/matrix.sh` around lines 36 - 40, Update the legacy linux64_ubsan
fallback’s ASAN_OPTIONS before sourcing 00_setup_env_native_asan.sh to include
detect_leaks=0, while preserving the existing ASAN options and setup flow.
2 participants
Issue being fixed or feature implemented
It's to follow Bitcoin Core mainstream. asan job completely superseed ubsan job and expand it.
What was done?
Backports:
libsecp256k1code bitcoin/bitcoin#28875add second_deadlock_stack=1to TSAN options bitcoin/bitcoin#31232Fixes for old backports:
Enabled new sanitizers for dash core:
detect_leaks=1
detect_stack_use_after_return=1
check_initialization_order=1
strict_init_order=1
How Has This Been Tested?
Tested locally; extra fixes [discovered issues / new supressions] are in related PR: XXXX
To be tested on CI [waiting]
Breaking Changes
N/A
Checklist: