Skip to content

Commit 1791f6f

Browse files
Merge #1517: autotools: Disable eager MSan in ctime_tests
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
2 parents 06bff6d + ebfb82e commit 1791f6f

File tree

3 files changed

+48
-7
lines changed

3 files changed

+48
-7
lines changed

.github/workflows/ci.yml

+7-1
Original file line numberDiff line numberDiff line change
@@ -485,18 +485,24 @@ jobs:
485485
matrix:
486486
configuration:
487487
- env_vars:
488+
CTIMETESTS: 'yes'
488489
CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -g'
489490
- env_vars:
490491
ECMULTGENKB: 2
491492
ECMULTWINDOW: 2
493+
CTIMETESTS: 'yes'
492494
CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -g -O3'
495+
- env_vars:
496+
# -fsanitize-memory-param-retval is clang's default, but our build system disables it
497+
# when ctime_tests when enabled.
498+
CFLAGS: '-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g'
499+
CTIMETESTS: 'no'
493500

494501
env:
495502
ECDH: 'yes'
496503
RECOVERY: 'yes'
497504
SCHNORRSIG: 'yes'
498505
ELLSWIFT: 'yes'
499-
CTIMETESTS: 'yes'
500506
CC: 'clang'
501507
SECP256K1_TEST_ITERS: 32
502508
ASM: 'no'

build-aux/m4/bitcoin_secp.m4

+12
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,18 @@ fi
4545
AC_MSG_RESULT($has_valgrind)
4646
])
4747

48+
AC_DEFUN([SECP_MSAN_CHECK], [
49+
AC_MSG_CHECKING(whether MemorySanitizer is enabled)
50+
AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
51+
#if defined(__has_feature)
52+
# if __has_feature(memory_sanitizer)
53+
# error "MemorySanitizer is enabled."
54+
# endif
55+
#endif
56+
]])], [msan_enabled=no], [msan_enabled=yes])
57+
AC_MSG_RESULT([$msan_enabled])
58+
])
59+
4860
dnl SECP_TRY_APPEND_CFLAGS(flags, VAR)
4961
dnl Append flags to VAR if CC accepts them.
5062
AC_DEFUN([SECP_TRY_APPEND_CFLAGS], [

configure.ac

+29-6
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,20 @@ if test x"$enable_ctime_tests" = x"auto"; then
247247
enable_ctime_tests=$enable_valgrind
248248
fi
249249

250+
print_msan_notice=no
251+
if test x"$enable_ctime_tests" = x"yes" && test x"$GCC" = x"yes"; then
252+
SECP_MSAN_CHECK
253+
# MSan on Clang >=16 reports unitialized memory in function parameters and return values, even if
254+
# the uninitalized variable is never actually "used". This is called "eager" checking, and it's
255+
# sounds like good idea for normal use of MSan. However, it yields many false positives in the
256+
# ctime_tests because many return values depend on secret (i.e., "uninitialized") values, and
257+
# we're only interested in detecting branches (which count as "uses") on secret data.
258+
if test x"$msan_enabled" = x"yes"; then
259+
SECP_TRY_APPEND_CFLAGS([-fno-sanitize-memory-param-retval], SECP_CFLAGS)
260+
print_msan_notice=yes
261+
fi
262+
fi
263+
250264
if test x"$enable_coverage" = x"yes"; then
251265
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
252266
SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
@@ -426,12 +440,7 @@ fi
426440
### Check for --enable-experimental if necessary
427441
###
428442

429-
if test x"$enable_experimental" = x"yes"; then
430-
AC_MSG_NOTICE([******])
431-
AC_MSG_NOTICE([WARNING: experimental build])
432-
AC_MSG_NOTICE([Experimental features do not have stable APIs or properties, and may not be safe for production use.])
433-
AC_MSG_NOTICE([******])
434-
else
443+
if test x"$enable_experimental" = x"no"; then
435444
if test x"$set_asm" = x"arm32"; then
436445
AC_MSG_ERROR([ARM32 assembly is experimental. Use --enable-experimental to allow.])
437446
fi
@@ -492,3 +501,17 @@ echo " CPPFLAGS = $CPPFLAGS"
492501
echo " SECP_CFLAGS = $SECP_CFLAGS"
493502
echo " CFLAGS = $CFLAGS"
494503
echo " LDFLAGS = $LDFLAGS"
504+
505+
if test x"$print_msan_notice" = x"yes"; then
506+
echo
507+
echo "Note:"
508+
echo " MemorySanitizer detected, tried to add -fno-sanitize-memory-param-retval to SECP_CFLAGS"
509+
echo " to avoid false positives in ctime_tests. Pass --disable-ctime-tests to avoid this."
510+
fi
511+
512+
if test x"$enable_experimental" = x"yes"; then
513+
echo
514+
echo "WARNING: Experimental build"
515+
echo " Experimental features do not have stable APIs or properties, and may not be safe for"
516+
echo " production use."
517+
fi

0 commit comments

Comments
 (0)