-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7444a2b
75b5a22
6fea7b9
1a202b3
208e425
406c549
f7b15d4
3906c28
b8a6664
d1755bb
eacd9e0
0f7da3a
6c460f6
3aaf867
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,9 @@ export LC_ALL=C.UTF-8 | |
| source ./ci/test/00_setup_env.sh | ||
|
|
||
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in this update — Missing prerequisite hunk from bitcoin#30665 no longer present. Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. |
||
| export LSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/lsan" | ||
| export TSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/tsan:halt_on_error=1" | ||
| export TSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" | ||
| export UBSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" | ||
|
|
||
| if [ "$BUILD_TARGET" = "aarch64-linux" ]; then | ||
|
|
@@ -33,7 +33,11 @@ elif [ "$BUILD_TARGET" = "linux64_sqlite" ]; then | |
| elif [ "$BUILD_TARGET" = "linux64_tsan" ]; then | ||
| source ./ci/test/00_setup_env_native_tsan.sh | ||
| elif [ "$BUILD_TARGET" = "linux64_ubsan" ]; then | ||
| source ./ci/test/00_setup_env_native_ubsan.sh | ||
| # 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_leaks=0:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1" | ||
| source ./ci/test/00_setup_env_native_asan.sh | ||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
35
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Drop the temporary legacy-workflow workaround before merge This branch exists only so the pre-merge source: ['codex'] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in this update — Drop the temporary legacy-workflow workaround before merge no longer present. Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: the immediately preceding automated “resolved” reply was posted in error. This finding remains valid at exact head
Canonical final verifier status: STILL_VALID (suggestion, non-blocking). |
||
| elif [ "$BUILD_TARGET" = "linux64_valgrind" ]; then | ||
| source ./ci/test/00_setup_env_native_valgrind.sh | ||
| elif [ "$BUILD_TARGET" = "mac" ]; then | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -393,7 +393,8 @@ if test "$use_sanitizers" != ""; then | |
| dnl fail if a bad argument is passed, e.g. -fsanitize=undfeined | ||
| AX_CHECK_COMPILE_FLAG( | ||
| [-fsanitize=$use_sanitizers], | ||
| [SANITIZER_CXXFLAGS="-fsanitize=$use_sanitizers"], | ||
| [SANITIZER_CXXFLAGS="-fsanitize=$use_sanitizers" | ||
| SANITIZER_CFLAGS="-fsanitize=$use_sanitizers"], | ||
|
Comment on lines
+396
to
+397
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This assigns AGENTS.md reference: AGENTS.md:L150-L152 Useful? React with 👍 / 👎. |
||
| [AC_MSG_ERROR([compiler did not accept requested flags])]) | ||
|
|
||
| dnl Some compilers (e.g. GCC) require additional libraries like libasan, | ||
|
|
@@ -2073,6 +2074,9 @@ CPPFLAGS_TEMP="$CPPFLAGS" | |
| unset CPPFLAGS | ||
| CPPFLAGS="$CPPFLAGS_TEMP" | ||
|
|
||
| if test -n "$use_sanitizers"; then | ||
| export SECP_CFLAGS="$SECP_CFLAGS $SANITIZER_CFLAGS" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For Useful? React with 👍 / 👎. |
||
| fi | ||
| ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --disable-module-ecdh --disable-openssl-tests" | ||
| AC_CONFIG_SUBDIRS([src/dashbls src/secp256k1]) | ||
|
|
||
|
|
@@ -2133,7 +2137,7 @@ echo " target os = $host_os" | |
| echo " build os = $build_os" | ||
| echo | ||
| echo " CC = $CC" | ||
| echo " CFLAGS = $BACKTRACE_FLAGS $DEBUG_CFLAGS $PTHREAD_CFLAGS $CFLAGS" | ||
| echo " CFLAGS = $SANITIZER_CFLAGS $BACKTRACE_FLAGS $DEBUG_CFLAGS $PTHREAD_CFLAGS $CFLAGS" | ||
| echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CORE_CPPFLAGS $CPPFLAGS" | ||
| echo " CXX = $CXX" | ||
| echo " CXXFLAGS = $BACKTRACE_FLAGS $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $CORE_CXXFLAGS $CXXFLAGS" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,4 @@ | ||
| # Suppress warnings triggered in dependencies | ||
| leak:libQt5Widgets | ||
| leak:QDBusConnectionPrivate | ||
| leak:QLayoutPrivate |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,25 +12,27 @@ unsigned-integer-overflow:*/include/c++/ | |
| unsigned-integer-overflow:FuzzedDataProvider::ConsumeIntegralInRange | ||
| unsigned-integer-overflow:leveldb/ | ||
| unsigned-integer-overflow:minisketch/ | ||
| unsigned-integer-overflow:secp256k1/ | ||
| unsigned-integer-overflow:secp256k1* | ||
| unsigned-integer-overflow:test/fuzz/crypto_diff_fuzz_chacha20.cpp | ||
| implicit-integer-sign-change:*/include/boost/ | ||
| implicit-integer-sign-change:*/include/c++/ | ||
| implicit-integer-sign-change:*/new_allocator.h | ||
| implicit-integer-sign-change:crc32c/ | ||
| implicit-integer-sign-change:minisketch/ | ||
| implicit-integer-sign-change:secp256k1/ | ||
| implicit-integer-sign-change:secp256k1* | ||
| implicit-signed-integer-truncation:*/include/c++/ | ||
| implicit-signed-integer-truncation:leveldb/ | ||
| implicit-signed-integer-truncation:secp256k1/ | ||
| implicit-signed-integer-truncation:secp256k1* | ||
| implicit-signed-integer-truncation,implicit-integer-sign-change:secp256k1_modinv64_posdivsteps_62_var | ||
| implicit-unsigned-integer-truncation:*/include/c++/ | ||
| implicit-unsigned-integer-truncation:leveldb/ | ||
| implicit-unsigned-integer-truncation:secp256k1/ | ||
| implicit-unsigned-integer-truncation:secp256k1* | ||
| implicit-signed-integer-truncation,implicit-integer-sign-change:secp256k1* | ||
| implicit-unsigned-integer-truncation:test/fuzz/crypto_diff_fuzz_chacha20.cpp | ||
| shift-base:*/include/c++/ | ||
| shift-base:leveldb/ | ||
| shift-base:minisketch/ | ||
| shift-base:secp256k1/ | ||
| shift-base:secp256k1* | ||
|
Comment on lines
+15
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Keep secp256k1 suppressions scoped away from first-party fuzz files LLVM 19's source: ['claude', 'codex'] |
||
| shift-base:test/fuzz/crypto_diff_fuzz_chacha20.cpp | ||
| # Unsigned integer overflow occurs when the result of an unsigned integer | ||
| # computation cannot be represented in its type. Unlike signed integer overflow, | ||
|
|
@@ -46,6 +48,13 @@ unsigned-integer-overflow:compressor.cpp | |
| unsigned-integer-overflow:crypto/ | ||
| unsigned-integer-overflow:hash.cpp | ||
| unsigned-integer-overflow:lcg.h | ||
| # The quorum snapshot skip list stores offsets relative to the first skipped | ||
| # index, and the index wraps around the sorted MN list, so an offset can be | ||
| # negative. Encoding wraps size_t and narrows into vector<int>, decoding wraps | ||
| # it back; the two cancel and the absolute index is recovered exactly. Per | ||
| # symbol rather than per file so the rest of llmq/utils.cpp stays covered. | ||
| unsigned-integer-overflow:BuildNewQuorumQuarterMembers | ||
| unsigned-integer-overflow:GetQuorumQuarterMembersBySnapshot | ||
| unsigned-integer-overflow:policy/fees.cpp | ||
| unsigned-integer-overflow:prevector.h | ||
| unsigned-integer-overflow:EvalScript | ||
|
|
@@ -54,6 +63,7 @@ unsigned-integer-overflow:xoroshiro128plusplus.h | |
| implicit-integer-sign-change:addrman.h | ||
| implicit-integer-sign-change:compat/stdin.cpp | ||
| implicit-integer-sign-change:CBlockPolicyEstimator::processBlockTx | ||
| implicit-integer-sign-change:GetQuorumQuarterMembersBySnapshot | ||
| implicit-integer-sign-change:compressor.h | ||
| implicit-integer-sign-change:crypto/ | ||
| implicit-integer-sign-change:policy/fees.cpp | ||
|
|
@@ -66,6 +76,7 @@ implicit-integer-sign-change:util/strencodings.cpp | |
| implicit-integer-sign-change:util/strencodings.h | ||
| implicit-integer-sign-change:validation.cpp | ||
| implicit-signed-integer-truncation,implicit-integer-sign-change:test/skiplist_tests.cpp | ||
| implicit-signed-integer-truncation,implicit-integer-sign-change:BuildNewQuorumQuarterMembers | ||
| implicit-signed-integer-truncation:addrman.h | ||
| implicit-signed-integer-truncation:crypto/ | ||
| implicit-unsigned-integer-truncation:crypto/ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new ASan functional-test job runs through
test-src.ymlincontainer-slim, but the only image updated to register an unversioned/usr/bin/llvm-symbolizeriscontrib/containers/ci/ci.Dockerfile;ci-slim.Dockerfilestill 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 👍 / 👎.