Skip to content

Commit

Permalink
Merge #1517: autotools: Disable eager MSan in ctime_tests
Browse files Browse the repository at this point in the history
ebfb82e ci: Add job with -fsanitize-memory-param-retval (Tim Ruffing)
e1bef09 configure: Move "experimental" warning to bottom (Tim Ruffing)
55e5d97 autotools: Disable eager MSan in ctime_tests (Tim Ruffing)

Pull request description:

  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?

ACKs for top commit:
  hebasto:
    ACK ebfb82e, tested on Ubuntu 24.04 with different clang versions (from 15 to 18) and different build configurations. CI changes look OK as well.

Tree-SHA512: c083d778fd50bd35c2e29b7fe0d92b98d912ee5ac7809ae73067d050a0d3c42b3483260f1286d0023cdb802a3c3006bf932ecf60ce81b942de1c9824374c0132
  • Loading branch information
real-or-random committed May 27, 2024
2 parents 06bff6d + ebfb82e commit 1791f6f
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 7 deletions.
8 changes: 7 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -485,18 +485,24 @@ jobs:
matrix:
configuration:
- env_vars:
CTIMETESTS: 'yes'
CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -g'
- env_vars:
ECMULTGENKB: 2
ECMULTWINDOW: 2
CTIMETESTS: 'yes'
CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -g -O3'
- env_vars:
# -fsanitize-memory-param-retval is clang's default, but our build system disables it
# when ctime_tests when enabled.
CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g'
CTIMETESTS: 'no'

env:
ECDH: 'yes'
RECOVERY: 'yes'
SCHNORRSIG: 'yes'
ELLSWIFT: 'yes'
CTIMETESTS: 'yes'
CC: 'clang'
SECP256K1_TEST_ITERS: 32
ASM: 'no'
Expand Down
12 changes: 12 additions & 0 deletions build-aux/m4/bitcoin_secp.m4
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,18 @@ fi
AC_MSG_RESULT($has_valgrind)
])

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

dnl SECP_TRY_APPEND_CFLAGS(flags, VAR)
dnl Append flags to VAR if CC accepts them.
AC_DEFUN([SECP_TRY_APPEND_CFLAGS], [
Expand Down
35 changes: 29 additions & 6 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,20 @@ if test x"$enable_ctime_tests" = x"auto"; then
enable_ctime_tests=$enable_valgrind
fi

print_msan_notice=no
if test x"$enable_ctime_tests" = x"yes" && test x"$GCC" = x"yes"; then
SECP_MSAN_CHECK
# MSan on Clang >=16 reports unitialized memory in function parameters and return values, even if
# the uninitalized variable is never actually "used". This is called "eager" checking, and it's
# sounds like good idea for normal use of MSan. However, it yields many false positives in the
# ctime_tests because many return values depend on secret (i.e., "uninitialized") values, and
# we're only interested in detecting branches (which count as "uses") on secret data.
if test x"$msan_enabled" = x"yes"; then
SECP_TRY_APPEND_CFLAGS([-fno-sanitize-memory-param-retval], SECP_CFLAGS)
print_msan_notice=yes
fi
fi

if test x"$enable_coverage" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
Expand Down Expand Up @@ -426,12 +440,7 @@ fi
### Check for --enable-experimental if necessary
###

if test x"$enable_experimental" = x"yes"; then
AC_MSG_NOTICE([******])
AC_MSG_NOTICE([WARNING: experimental build])
AC_MSG_NOTICE([Experimental features do not have stable APIs or properties, and may not be safe for production use.])
AC_MSG_NOTICE([******])
else
if test x"$enable_experimental" = x"no"; then
if test x"$set_asm" = x"arm32"; then
AC_MSG_ERROR([ARM32 assembly is experimental. Use --enable-experimental to allow.])
fi
Expand Down Expand Up @@ -492,3 +501,17 @@ echo " CPPFLAGS = $CPPFLAGS"
echo " SECP_CFLAGS = $SECP_CFLAGS"
echo " CFLAGS = $CFLAGS"
echo " LDFLAGS = $LDFLAGS"

if test x"$print_msan_notice" = x"yes"; then
echo
echo "Note:"
echo " MemorySanitizer detected, tried to add -fno-sanitize-memory-param-retval to SECP_CFLAGS"
echo " to avoid false positives in ctime_tests. Pass --disable-ctime-tests to avoid this."
fi

if test x"$enable_experimental" = x"yes"; then
echo
echo "WARNING: Experimental build"
echo " Experimental features do not have stable APIs or properties, and may not be safe for"
echo " production use."
fi

0 comments on commit 1791f6f

Please sign in to comment.