crypto: disable ASan instrumentation of SSE4 SHA256 for GCC (matching Clang) #34953

pull deadmanoz wants to merge 1 commits into bitcoin:master from deadmanoz:fix/gcc-asan-sha256-sse4-only changing 1 files +14 −9

deadmanoz commented at 3:15 AM on March 30, 2026: contributor
Fix the runtime crash described in #34881.

Upstream already disables ASan instrumentation for sha256_sse4::Transform() under Clang. This extends the same workaround to GCC by adding an #elif branch for __GNUC__ / __SANITIZE_ADDRESS__ that applies the same no_sanitize("address") attribute.

Testing:
- reproduced the crash before the fix with GCC 13, 14, and 15 on Haswell-class machines / guests without SHA-NI (including by forcing the SSE4 implementation on GitHub CI)
- SEGV in debug builds regardless of optimization level (tested -O0, -O1, -O2, -O3)
- verified that the GCC + ASan debug configurations that previously crashed pass with this change
Issue #34881 has more details about the issue.

Note: the original Clang code placed the __attribute__ between the function declarator and the opening brace. GCC's Attribute Syntax documentation notes that this position in a function definition "may, in future, be permitted," so it is not currently supported. Placing the attribute at the start of the function definition is valid form for both GCC and Clang.
DrahtBot added the label Utils/log/libs on Mar 30, 2026
DrahtBot commented at 3:15 AM on March 30, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34953.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
in src/crypto/sha256_sse4.cpp:27 in 80160abe68
```
  29 | -  clang is unable to compile this with -O0 and -fsanitize=address.
  30 | -  See upstream bug: https://github.com/llvm/llvm-project/issues/92182.
  31 | -  This also fails to compile with -O2, -fcf-protection & -fsanitize=address.
  32 | -  See https://github.com/bitcoin/bitcoin/issues/31913.
  33 | -  */
  34 |  #if __has_feature(address_sanitizer)
```
maflcko commented at 11:48 AM on March 30, 2026:

Why not just use a check for __SANITIZE_ADDRESS__? According to https://clang.llvm.org/docs/AddressSanitizer.html#interaction-of-inlining-with-disabling-sanitizer-instrumentation this is deprecated:

// Note, has_feature test for sanitizers is deprecated, and Clang will support __SANITIZE_<sanitizer> similar to GCC.

https://godbolt.org/z/Ed7vMoKcP

Maybe a temporary fallback with this can be provided?

deadmanoz commented at 3:03 PM on March 31, 2026:

Thanks @maflcko, Godbolt is pretty rad!

It seems that __SANITIZE_ADDRESS__ was merged in LLVM August last year (https://github.com/llvm/llvm-project/pull/153888), is now supported since Clang 22.1.0 (released February this year).

But yes, earlier versions of Clang, e.g. 21.1.0 down to the minimum supported 17 you yourself specified (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md, #33555), would still need the _has_feature test for sanitizers.

https://godbolt.org/z/G17hjbh3b

So the change can be a more concise single unified check, will modify.
deadmanoz commented at 3:40 PM on March 31, 2026:

Ok, my earlier comment was wrong, a single unified check (e.g. with ||) doesn't work on GCC < 14 (minimum required GCC is 12.1 as per doc/dependencies):

// Fails on GCC 12/13: #if defined(__SANITIZE_ADDRESS__) || (defined(__clang__) && __has_feature(address_sanitizer))

See: https://godbolt.org/z/jzvnhj88a

So the guard needs a nested #elif to keep __has_feature hidden from GCC for < 14 versions:

#if defined(__SANITIZE_ADDRESS__) __attribute__((no_sanitize("address"))) #elif defined(__clang__) && __has_feature(address_sanitizer) __attribute__((no_sanitize("address"))) #endif

Works on Clang 17 and GCC 12.1: https://godbolt.org/z/cxv59c65K Through Clang 22 and GCC 15.2: https://godbolt.org/z/cf4e5nM6W
deadmanoz force-pushed on Mar 31, 2026

Type	Reviewers
ACK	maflcko, sedited

in src/crypto/sha256_sse4.cpp:27 in 09f0d0942e outdated

  31 | +  See https://github.com/bitcoin/bitcoin/issues/34881.
  32 | +*/
  33 | +#if defined(__SANITIZE_ADDRESS__)
  34 | +  __attribute__((no_sanitize("address")))
  35 | +#elif defined(__clang__) && __has_feature(address_sanitizer)
  36 |    __attribute__((no_sanitize("address")))

maflcko commented at 3:48 PM on March 31, 2026:

  __attribute__((no_sanitize("address")))  // fallback can be removed once support for clang 21 is dropped

Maybe document when the fallback can be dropped?

DrahtBot added the label CI failed on Mar 31, 2026

crypto: disable ASan instrumentation of SSE4 SHA256 for GCC

The existing Clang-only no_sanitize("address") guard is extended to
also cover GCC.  When GCC compiles this file with -fsanitize=address
in debug builds, the instrumented inline assembly causes a SEGV during
SHA256AutoDetect()'s self-test on CPUs that use the SSE4 code path
(i.e. those without SHA-NI support), regardless of optimization level.

The original Clang code placed the attribute between the function
declarator and the opening brace.  GCC's Attribute Syntax
documentation notes that this position in a function definition
"may, in future, be permitted," so it is not currently supported.
The attribute is moved to the start of the function definition,
which is valid form for both GCC and Clang.

The preprocessor guards are restructured so each compiler branch is
explicit: __clang__ with __has_feature, and __GNUC__ with
__SANITIZE_ADDRESS__.

fedeff7f20

deadmanoz force-pushed on Apr 1, 2026
deadmanoz commented at 7:21 AM on April 1, 2026: contributor

https://github.com/bitcoin/bitcoin/actions/runs/23806282813/job/69380539240?pr=34953

Eek, breaks the important case, when __SANITIZE_ADDRESS__ is NOT defined and GCC < 14.

Pushed a re-structure
DrahtBot removed the label CI failed on Apr 1, 2026
sedited requested review from maflcko on May 26, 2026

maflcko commented at 7:49 PM on May 28, 2026: member

lgtm ACK fedeff7f201df0206eefb744ad125e44a63a3ea0 🏒

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK fedeff7f201df0206eefb744ad125e44a63a3ea0 🏒
IXnL3+GPgsNM+C7vZW65SAbzbsYmhUQzrfq0+PKY9behvNEHlZQUDFh6JdiD87X1NbeYdC/4znbePeis3pUADA==

</details>

maflcko commented at 7:50 PM on May 28, 2026: member

tested via:

diff --git a/src/crypto/sha256.cpp b/src/crypto/sha256.cpp
index 54bd9a59f9..87c728920b 100644
--- a/src/crypto/sha256.cpp
+++ b/src/crypto/sha256.cpp
@@ -618,3 +618,3 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem
         if (use_implementation & sha256_implementation::USE_SHANI) {
-            have_x86_shani = (ebx >> 29) & 1;
+            have_x86_shani = false;
         }

and:

./bld-cmake/bin/test_bitcoin -t crypto_tests

sedited approved
sedited commented at 8:58 PM on May 28, 2026: contributor

tACK fedeff7f201df0206eefb744ad125e44a63a3ea0
sedited merged this on May 28, 2026
sedited closed this on May 28, 2026
fanquake added the label Needs Backport (31.x) on May 28, 2026
deadmanoz deleted the branch on May 29, 2026
fanquake removed the label Needs Backport (31.x) on May 29, 2026
fanquake commented at 8:53 AM on May 29, 2026: member

Backported to 31.x in #35331.

Contributors

Review Requested
maflcko

Labels

Utils/log/libs

Linked (view graph)

#34881 GCC + ASan debug builds: runtime SEGV in `sha256_sse4::Transform` on CPUs without SHA-NI #35331 [31.x] Backports

crypto: disable ASan instrumentation of SSE4 SHA256 for GCC (matching Clang) #34953

Code Coverage & Benchmarks

Reviews