From 55e5d975db9a641bcbeffcc3fc086e54d322fe26 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 15 Apr 2024 17:07:41 +0200 Subject: [PATCH 1/3] autotools: Disable eager MSan in ctime_tests Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> --- build-aux/m4/bitcoin_secp.m4 | 12 ++++++++++++ configure.ac | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4 index 11adef4f22..fee2d7b4db 100644 --- a/build-aux/m4/bitcoin_secp.m4 +++ b/build-aux/m4/bitcoin_secp.m4 @@ -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], [ diff --git a/configure.ac b/configure.ac index 8b62e1dbfd..085c360958 100644 --- a/configure.ac +++ b/configure.ac @@ -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" @@ -492,3 +506,10 @@ 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 From e1bef0961c53b23dbaf35f5fa87149a1aadfad37 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 22 May 2024 15:10:33 +0200 Subject: [PATCH 2/3] configure: Move "experimental" warning to bottom to make it more promiment --- configure.ac | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index 085c360958..9640a67399 100644 --- a/configure.ac +++ b/configure.ac @@ -440,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 @@ -513,3 +508,10 @@ if test x"$print_msan_notice" = x"yes"; then 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 From ebfb82ee2f8c15ae6129932380b1dfb0942ea35a Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 22 May 2024 16:51:51 +0200 Subject: [PATCH 3/3] ci: Add job with -fsanitize-memory-param-retval --- .github/workflows/ci.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d246682044..0d3ca20e93 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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'