ci: Use clang-snapshot in "MSan" job

hebasto commented at 12:53 pm on September 18, 2025: member

In Bitcoin Core, the “MSan” CI jobs use the latest tagged Clang available from http://apt.llvm.org.

This PR applies similar changes and switches the “MSan” CI jobs to clang-snapshot.

This exposes problematic code that was reported in https://github.com/bitcoin/bitcoin/issues/33284.

hebasto commented at 1:39 pm on September 18, 2025: member

From https://github.com/bitcoin-core/secp256k1/actions/runs/17829295048/job/50691626826:

0   CC       src/noverify_tests-tests.o
1src/tests.c:6051:34: error: variable 'pubkey' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
2 6051 |     SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
3      |                                  ^~~~~~
41 error generated.

hebasto force-pushed on Sep 18, 2025

hebasto marked this as ready for review on Sep 18, 2025

hebasto commented at 3:16 pm on September 18, 2025: member

From https://github.com/bitcoin-core/secp256k1/actions/runs/17829295048/job/50691626826:

0   CC       src/noverify_tests-tests.o
1src/tests.c:6051:34: error: variable 'pubkey' is uninitialized when passed as a const pointer argument here [-Werror,-Wuninitialized-const-pointer]
2 6051 |     SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
3      |                                  ^~~~~~
41 error generated.

Added a commit to silence the -Wuninitialized-const-pointer warning.

real-or-random commented at 4:24 pm on September 19, 2025: contributor

I think that’s a Clang bug.

SECP256K1_CHECKMEM_UNDEFINE expands to __msan_allocated_memory with signature:

0/* Tell MSan about newly allocated memory (ex.: custom allocator).
1   Memory will be marked uninitialized, with origin at the call site. */
2void SANITIZER_CDECL __msan_allocated_memory(const volatile void *data,
3                                             size_t size);

https://github.com/llvm/llvm-project/blob/24b03d3217e41536cee7c868860b5930160ad526/compiler-rt/include/sanitizer/msan_interface.h#L93-L96

So yes, we’re passing a const pointer to an uninitialized portion of memory, which is typically pointless: the callee can’t read it (UB) and it is not allowed to write it either.

But __msan_allocated_memory is special in this regard. The call is meaningful. And since it’s a part of the Clang runtime, Clang should understand that this is meaningful.

Are you willing to report this upstream?

On our side, I think there are some nicer options:

Drop the call entirely (it’s a noop because pubkey is already uninitialized). But it’s easy to overlook in the future.
Initialize the variable.
Wrap the suppression inside the macro to make sure we always avoid this bug.

I tend to 3. It’s perhaps the most complex option, but also the most future-proof?

real-or-random added the label assurance on Sep 19, 2025

real-or-random added the label tweak/refactor on Sep 19, 2025

hebasto commented at 4:58 pm on September 19, 2025: member

I think that’s a Clang bug.

I agree with your analysis. That’s why I chose to suppress the warning rather than take another approach.

Wrap the suppression inside the macro to make sure we always avoid this bug.

I tend to 3. It’s perhaps the most complex option, but also the most future-proof?

That was my initial approach, but I didn’t manage to implement it properly. Could you suggest your patch?

real-or-random commented at 10:47 am on September 22, 2025: contributor

That was my initial approach, but I didn’t manage to implement it properly. Could you suggest your patch?

Can you try this in checkmem.h?

 0#if defined(__has_feature)
 1#  if __has_feature(memory_sanitizer)
 2#    include <sanitizer/msan_interface.h>
 3#    define SECP256K1_CHECKMEM_ENABLED 1
 4#    if defined(__clang__)
 5#      define SECP256K1_CHECKMEM_UNDEFINE(p, len) do { \
 6         _Pragma("clang diagnostic push") \
 7         _Pragma("clang diagnostic ignored \"-Wuninitialized-const-pointer\"") \
 8         __msan_allocated_memory((p), (len)); \
 9         _Pragma("clang diagnostic pop") \
10         } while(0)
11#    else
12#      define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len))
13#    endif
14#    define SECP256K1_CHECKMEM_DEFINE(p, len) __msan_unpoison((p), (len))
15#    define SECP256K1_CHECKMEM_MSAN_DEFINE(p, len) __msan_unpoison((p), (len))
16#    define SECP256K1_CHECKMEM_CHECK(p, len) __msan_check_mem_is_initialized((p), (len))
17#    define SECP256K1_CHECKMEM_RUNNING() (1)
18#  endif
19#endif

I think that’s a Clang bug.

I agree with your analysis. That’s why I chose to suppress the warning rather than take another approach.

Would you be willing to report it upstream? Perhaps it will be fixed before the release of clang 21 and then we’ll be able to drop this workaround.

hebasto commented at 1:07 pm on September 22, 2025: member

That was my initial approach, but I didn’t manage to implement it properly. Could you suggest your patch?

Can you try this in checkmem.h?

Thanks! Taken.

UPD. _Pragma(...) is a C99 feature.

Would you be willing to report it upstream?

https://github.com/llvm/llvm-project/issues/160094.

Perhaps it will be fixed before the release of clang 21 and then we’ll be able to drop this workaround.

https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.1

hebasto force-pushed on Sep 22, 2025

hebasto commented at 1:11 pm on September 22, 2025: member

The feedback from @real-or-random has been addressed.

real-or-random commented at 1:51 pm on September 22, 2025: contributor

UPD. _Pragma(...) is a C99 feature.

Indeed. If I read correctly, clang has always supported it. https://clang.llvm.org/c_status.html

in src/checkmem.h:54 in d6832a176c outdated

47@@ -48,7 +48,16 @@
48 #  if __has_feature(memory_sanitizer)
49 #    include <sanitizer/msan_interface.h>
50 #    define SECP256K1_CHECKMEM_ENABLED 1
51-#    define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len))
52+#    if defined(__clang__)
53+#      define SECP256K1_CHECKMEM_UNDEFINE(p, len) do { \
54+         _Pragma("clang diagnostic push") \

real-or-random commented at 1:54 pm on September 22, 2025:

0         # Work around https://github.com/llvm/llvm-project/issues/160094
1         _Pragma("clang diagnostic push") \

hebasto commented at 2:00 pm on September 22, 2025:

Thanks! Done.

real-or-random approved

real-or-random commented at 1:54 pm on September 22, 2025: contributor

utACK mod nit

hebasto force-pushed on Sep 22, 2025

real-or-random added the label ci on Sep 24, 2025

real-or-random added the label side-channel on Sep 24, 2025

real-or-random removed the label assurance on Sep 24, 2025

in .github/workflows/ci.yml:443 in 7ca890d278

439@@ -440,7 +440,7 @@ jobs:
440       SCHNORRSIG: 'yes'
441       MUSIG: 'yes'
442       ELLSWIFT: 'yes'
443-      CC: 'clang'
444+      CC: 'clang-snapshot'

real-or-random commented at 8:39 am on October 14, 2025:

Now that I think about it, why not run these three CI jobs on both clang and clang-snapshot?

hebasto commented at 9:32 am on October 14, 2025:

Thanks! Reworked.

real-or-random approved

real-or-random commented at 8:39 am on October 14, 2025: contributor

This PR is nice because it’s an alternative to the Valgrind tests on clang-snapshot. That is, we can run the constant-time tests on clang-snapshot without Valgrind support.

hebasto force-pushed on Oct 14, 2025

hebasto commented at 9:31 am on October 14, 2025: member

Rebased to resolve a conflict with the merged #1756 and taken @real-or-random’s suggestion.

real-or-random approved

real-or-random commented at 9:32 am on October 14, 2025: contributor

utACK https://github.com/bitcoin-core/secp256k1/pull/1750/commits/ed9ea8889799dd291bbedb2e78ace469880dab8f but let’s see if CI succeeds

hebasto commented at 9:47 am on October 14, 2025: member

… but let’s see if CI succeeds

“Old” clang doesn’t understand “-Wuninitialized-const-pointer”…

hebasto force-pushed on Oct 14, 2025

Fix Clang 21+ `-Wuninitialized-const-pointer` warning when using MSan

Co-authored-by: Tim Ruffing <me@real-or-random.org>

6894c964f3

ci: Use clang-snapshot in "MSan" job 53585f93b7

hebasto force-pushed on Oct 14, 2025

hebasto commented at 11:15 am on October 14, 2025: member

… but let’s see if CI succeeds

It’s green now :)

real-or-random approved

real-or-random commented at 11:37 am on October 14, 2025: contributor

utACK 53585f93b7e6fe3933c77216f45e7d4f785436b5

real-or-random merged this on Oct 14, 2025

real-or-random closed this on Oct 14, 2025

hebasto deleted the branch on Oct 14, 2025

ci: Use clang-snapshot in “MSan” job #1750