msan: notate variable assignments from assembly code #1496

msan isn't smart enough to see that these are set without some help.

This was pointed out here: #1169 (comment)

With this commit, msan output is clean even with x86 asm turned on.

utACK 8f237f6ff13f5654b1ca7d31481ca96e58fc7d02

Oh, that's nice. I run into this regularly, but I didn't think of this simple solution.

Nice. Concept ACK. I'll run this through out MSAN CI while using asm.

Concept ACK.

Is still there any specific reason to keep ASM=no in the CI?

FWIW, I've removed it everywhere in the .github/workflows/ci.yml and the CI passed.

Is still there any specific reason to keep ASM=no in the CI?

Yes, I think we want to test the non-ASM code even on x86_64. But you're right in principle. We should probably enable ASM in some more tasks. (Reworking the matrix something we should do anyway. It's one of the few boxes we haven't ticked in #1392 ... :/ )

ACK 8f237f6ff13f5654b1ca7d31481ca96e58fc7d02, tested on Ubuntu 23.10. Removing any of added macros triggers "MemorySanitizer: use-of-uninitialized-value" warning in the setup mentioned in the PR description.

@theuni Do you think it's worth making these conditional under __has_feature(memory_sanitizer) (or cleaner, a macro SECP256K1_CHECKMEM_MSAN to be added to checkmem.h) to make sure that this workaround applies only to MSan, and we never suppress positives omitted by Valgrind?

@real-or-random I didn't understand your question at first but I think maybe I know what you're asking now. I'll try to rephrase:

Because notation is necessary for msan but not valgrind (I don't know if that's true, I'm assuming based on your question), there should be a macro that is used only to notate for msan without affecting valgrind's detection? If that's right, sure, I can hook up a SECP256K1_CHECKMEM_MSAN. I'm unsure if other parts of the code would also benefit from one but not the other, but presumably if so we'd want to switch them over too.

edit: Like this, I think? (untested)

diff --git a/src/checkmem.h b/src/checkmem.h
index f2169de..68db9a7 100644
--- a/src/checkmem.h
+++ b/src/checkmem.h
@@ -48,11 +48,16 @@
 #    define SECP256K1_CHECKMEM_ENABLED 1
 #    define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len))
 #    define SECP256K1_CHECKMEM_DEFINE(p, len) __msan_unpoison((p), (len))
+#    define SECP256K1_CHECKMEM_MSAN(p, len) __msan_unpoison((p), (len))
 #    define SECP256K1_CHECKMEM_CHECK(p, len) __msan_check_mem_is_initialized((p), (len))
 #    define SECP256K1_CHECKMEM_RUNNING() (1)
 #  endif
 #endif

+#ifndef SECP256K1_CHECKMEM_MSAN
+#  define SECP256K1_CHECKMEM_MSAN(p, len) SECP256K1_CHECKMEM_NOOP((p), (len))
+#endif
+
 /* If valgrind integration is desired (through the VALGRIND define), implement the
  * SECP256K1_CHECKMEM_* macros using valgrind. */
 #if !defined SECP256K1_CHECKMEM_ENABLED

Logically, wrapping it in an #ifndef VALGRIND would make sense too, but I didn't because of the comment above it:

/* If compiling under msan, map the SECP256K1_CHECKMEM_* functionality to msan.
 * Choose this preferentially, even when VALGRIND is defined, as msan-compiled
 * binaries can't be run under valgrind anyway. */

Nit: I think SECP256K1_CHECKMEM_MSAN_DEFINE would be a better name so the other macros could get the same treatment in the future if necessary.

Yeah, sorry, my comment was not very detailed. :)

Because notation is necessary for msan but not valgrind (I don't know if that's true, I'm assuming based on your question),

Indeed. MSan's analysis needs instrumentation from the compiler, and it's not surprising that the compiler is not clever enough to understand our ASM and instrument the binary accordingly. Valgrind's analysis looks at the final (uninstrumented) binary only, and thus doesn't care at all if the code has been generated from C or asm or whatever.

there should be a macro that is used only to notate for msan without affecting valgrind's detection? If that's right, sure, I can hook up a SECP256K1_CHECKMEM_MSAN.

That's what I had in mind, yes.

I'm unsure if other parts of the code would also benefit from one but not the other, but presumably if so we'd want to switch them over too.

I grepped for SECP256K1_CHECKMEM_DEFINE, and it turns out that all current uses of the macro are related to constant-time testing. In these cases, we'll actually need it for both MSan and Valgrind. So no, no need to change any existing uses.

Nit: I think SECP256K1_CHECKMEM_MSAN_DEFINE would be a better name so the other macros could get the same treatment in the future if necessary.

Yeah, totally. Sorry, what I actually had in mind was that SECP256K1_CHECKMEM_MSAN (or perhaps better: ``SECP256K1_CHECKMEM_USES_MSAN`) is macro that indicates that we use MSan, and then we could do this after the asm code.

#if SECP256K1_CHECKMEM_USES_MSAN
    SECP256K1_CHECKMEM_DEFINE(...)
    ...
#endif

And there could also be a SECP256K1_CHECKMEM_USES_VALGRIND macro.

I think that's a tiny tiny bit simpler in a possible future with more than two backends, but really, either approach is fine in the end. And I agree, if we follow your approach, then SECP256K1_CHECKMEM_MSAN_DEFINE is a better name for sure.

Since you're ok with either approach, I pushed up the one I already had ready :)

utACK 31ba40494428dcbf2eb5eb6f2328eca91b0b0746

re-ACK 31ba40494428dcbf2eb5eb6f2328eca91b0b0746.

Just leaving here a reminder to reconsider disabling assembly in the CI jobs.

Have a followup to this here: https://github.com/bitcoin/bitcoin/pull/29742.