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?
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:
0--- a/configure.ac
1+++ b/configure.ac
2@@ -142,7 +142,7 @@ if test "x$GCC" = "xyes"; then
3 # We need to wrap the argument in a --(start/end)-no-unused-arguments block to suppress a
4 # -Wunused-command-line-argument warning, which clang emits if MSan is not actually enabled, i.e.,
5 # if -fsanitize=memory does not appear on the command line.
6- SECP_TRY_APPEND_CFLAGS([--start-no-unused-arguments -fno-sanitize-memory-param-retval --end-no-unused-arguments], ctime_tests_CFLAGS)
7+ SECP_TRY_APPEND_CFLAGS([--start-no-unused-arguments -fno-sanitize-memory-param-retval --end-no-unused-arguments], SECP_CFLAGS)
8 fi
9
10 ###
To have any effect, the
ctime_tests_CFLAGSvariable has toAC_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-retvaloption 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:
-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)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_CFLAGSvariable has toAC_SUBSTed.No, this seems to happen automatically.
make V=1shows 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.
- disable the ctime_tests if we find
-fsanitize=memoryor similar on the command line (but parsing this is complex in cases such as-fsanitize=unreachable,memorythis may be fragile and overkill)
The following check might be used:
0AC_DEFUN([SECP_MSAN_CHECK], [
1AC_MSG_CHECKING(whether MemorySanitizer is enabled)
2AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
3 #if defined(__has_feature)
4 # if __has_feature(memory_sanitizer)
5 # error "MemorySanitizer is enabled."
6 # endif
7 #endif
8 ]])], [msan_enabled=no], [msan_enabled=yes])
9AC_MSG_RESULT([$msan_enabled])
10])
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.
-fsanitize-memory-param-retval).
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
to make it more promiment
