Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 18, 2025

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 bitcoin/bitcoin#33284.

@hebasto
Copy link
Member Author

hebasto commented Sep 18, 2025

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

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

@hebasto hebasto marked this pull request as ready for review September 18, 2025 15:15
@hebasto
Copy link
Member Author

hebasto commented Sep 18, 2025

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

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

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

@real-or-random
Copy link
Contributor

I think that's a Clang bug.

SECP256K1_CHECKMEM_UNDEFINE expands to __msan_allocated_memory with signature:

/* Tell MSan about newly allocated memory (ex.: custom allocator).
   Memory will be marked uninitialized, with origin at the call site. */
void SANITIZER_CDECL __msan_allocated_memory(const volatile void *data,
                                             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:

  1. Drop the call entirely (it's a noop because pubkey is already uninitialized). But it's easy to overlook in the future.
  2. Initialize the variable.
  3. 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?

@hebasto
Copy link
Member Author

hebasto commented Sep 19, 2025

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.

  1. 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants