Skip to content

build: revert passing sanitizer flags to libsecp256k1 #7930

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.

Sign up for GitHub

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

Merged
PastaPastaPasta merged 1 commit into from
Oct 21, 2026
Merged

build: revert passing sanitizer flags to libsecp256k1 #7930

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2062,9 +2062,6 @@ CPPFLAGS_TEMP="$CPPFLAGS"
unset CPPFLAGS
CPPFLAGS="$CPPFLAGS_TEMP"

if test -n "$use_sanitizers"; then
export SECP_CFLAGS="$SECP_CFLAGS $SANITIZER_CFLAGS"
fi
dnl The ECDH module is required by the wallet's Platform key provider

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Retain secp256k1 instrumentation in fuzz builds

When using the documented libFuzzer configuration (doc/fuzzing.md:11) or the CI fuzz configuration (ci/test/00_setup_env_native_fuzz.sh:18), removing this propagation means --with-sanitizers no longer reaches AM_CFLAGS in src/secp256k1/Makefile.am. Consequently libFuzzer gets no branch feedback, and ASan/UBSan add no checks, inside secp256k1 even for the dedicated secp256k1_* fuzz targets, silently losing coverage of the cryptographic code; retain the propagation when enable_fuzz=yes while omitting it from the ASan/TSan regression-test builds whose runtime this change targets.

Useful? React with 👍 / 👎.

@knst knst Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

intentionally done as this code is backported from bitcoin core as it is and instrumented there.

Comment on lines 2064 to 2065

@thepastaclaw thepastaclaw Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Suggestion: Preserve libsecp256k1 instrumentation in fuzz builds

The removed export supplies libsecp256k1's sanitizer flags through SECP_CFLAGS, which its Makefile uses in AM_CFLAGS. Removing it also affects the documented --enable-fuzz --with-sanitizers=address,fuzzer,undefined configuration and ci/test/00_setup_env_native_fuzz.sh: library internals reached by fuzz targets lose compiler-inserted coverage feedback and ASan/UBSan checks. The DER wrappers themselves remain instrumented because ec_seckey_import_der/ec_seckey_export_der and ecdsa_signature_parse_der_lax are implemented in src/key.cpp and src/pubkey.cpp, not in the separately compiled library; the regression is specifically in the libsecp256k1 calls beneath those wrappers and other fuzz targets. Restore the export only when enable_fuzz=yes and sanitizers are requested. This preserves library instrumentation for fuzzing without restoring its cost in the ASan/TSan unit-test builds this PR targets.

source: glm-5.3-flash (phase1-reviewer: general, dash-core-commit-history)

dnl (DashPay contact request encryption, wallet/platformkeys.cpp).
ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-ecdh --disable-openssl-tests"
Expand Down
Loading