autotools: Disable eager MSan in ctime_tests #1517

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?

Concept ACK

Concept ACK.

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

Sure. I am :)

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

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

--- a/configure.ac
+++ b/configure.ac
@@ -142,7 +142,7 @@ if test "x$GCC" = "xyes"; then
   # We need to wrap the argument in a --(start/end)-no-unused-arguments block to suppress a
   # -Wunused-command-line-argument warning, which clang emits if MSan is not actually enabled, i.e.,
   # if -fsanitize=memory does not appear on the command line.
-  SECP_TRY_APPEND_CFLAGS([--start-no-unused-arguments -fno-sanitize-memory-param-retval --end-no-unused-arguments], ctime_tests_CFLAGS)
+  SECP_TRY_APPEND_CFLAGS([--start-no-unused-arguments -fno-sanitize-memory-param-retval --end-no-unused-arguments], SECP_CFLAGS)
 fi
 
 ###

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?

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.

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])
])

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.

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

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

ACK ebfb82ee2f8c15ae6129932380b1dfb0942ea35a, tested on Ubuntu 24.04 with different clang versions (from 15 to 18) and different build configurations. CI changes look OK as well.

@real-or-random

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

Please consider #1532.

Thx