autotools: Disable eager MSan in ctime_tests

[real-or-random]

This is the autotools solution for #1516.

Alternatively, we could have a full-blown --enable-msan option, but it's more work, and I'm not convinced that it's necessary or at least much better.

hebasto If you're Concept ACK, are you willing to work on an equivalent PR for CMake?

[sipa]

Concept ACK

[hebasto]

Concept ACK.

@hebasto If you're Concept ACK, are you willing to work on an equivalent PR for CMake?

Sure. I am :)

[real-or-random]

To have any effect, the ctime_tests_CFLAGS variable has to AC_SUBSTed.

No, this seems to happen automatically. make V=1 shows me that it's effective.

However, to achieve the goal, the -fno-sanitize-memory-param-retval option has to be applied to the library code as well.

Okay, true. Sorry, I thought I had tested this before opening a PR.

Then this is a bit annoying. Your suggestion will work for our "abuse" of MSan in the ctime_tests. But it will also disable the new eager checking behind the back of the user in the case of normal use, i.e., checking for actual memory issues.

So if we want to have both eager checking in "normal" MSan mode, and clean ctime_tests, I see no other way than having two builds (and adding a build to CI is not a problem). I think one of the following approaches will be sensible:

1. disable the ctime_tests if we find -fsanitize=memory or similar on the command line (but parsing this is complex in cases such as -fsanitize=unreachable,memory this may be fragile and overkill)
2. work with your suggestion to add this to SECP_CFLAGS, but only if constant-time tests are enabled. We could additionally print a warning that tells the users to disable the constant-time tests if they want to avoid that we add -fno-sanitize-memory-param-retval.

The second looks better to me. What do you think?

[hebasto]

To have any effect, the ctime_tests_CFLAGS variable has to AC_SUBSTed.

No, this seems to happen automatically. make V=1 shows me that it's effective.

I mean, only ctime_tests.c is compiled with ctime_tests_CFLAGS. However, all code is needed to be compiled with -fno-sanitize-memory-param-retval.

[hebasto]

1. disable the ctime_tests if we find -fsanitize=memory or similar on the command line (but parsing this is complex in cases such as -fsanitize=unreachable,memory this may be fragile and overkill)

The following check might be used:

AC_DEFUN([SECP_MSAN_CHECK], [
AC_MSG_CHECKING(whether MemorySanitizer is enabled)
AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
  #if defined(__has_feature)
  #  if __has_feature(memory_sanitizer)
  #    error "MemorySanitizer is enabled."
  #  endif
  #endif
  ]])], [msan_enabled=no], [msan_enabled=yes])
AC_MSG_RESULT([$msan_enabled])
])

[real-or-random]

2. work with your suggestion to add this to SECP_CFLAGS, but only if constant-time tests are enabled. We could additionally print a warning that tells the users to disable the constant-time tests if they want to avoid that we add -fno-sanitize-memory-param-retval.

I did this, using your suggested check for msan. Also rebased. Ready for review from my side.

Note: CI is currently failing on macOS. This should be resolved once the GitHub Actions image is updated to have brew 4.3.1, which has the fix (https://github.com/Homebrew/brew/pull/17336). The images are updated every week, so the issue should just disappear in a few days.

[real-or-random]

Pushed another commit that adds a CI job with eager checks enabled explicitly (-fsanitize-memory-param-retval).

[real-or-random]

Force pushed to fix a logic bug, should be really ready for review now. :)

[hebasto]

@real-or-random

@hebasto If you're Concept ACK, are you willing to work on an equivalent PR for CMake?

Please consider https://github.com/bitcoin-core/secp256k1/pull/1532.