Add support for msan instead of valgrind (for memcheck and ctime test) #1169

pull sipa wants to merge 10 commits into bitcoin-core:master from sipa:202212_msan changing 16 files +279 −191
  1. sipa commented at 11:54 PM on December 6, 2022: contributor

    This introduces an abstraction layer src/checkmem.h, which defines macros for interacting with memory checking tools. Depending on the environment, they're mapped to MemorySanitizer builtins, Valgrind integration macros, or nothing at all.

    This means that msan builds immediately benefit from existing undefined memory checks in the tests. It also means those builds result in a ctime_tests (new name for valgrind_ctime_test) binary that can usefully test constant-timeness (not inside Valgrind, and with the downside that it's not running against a production library build, but it's faster and available on more platforms).

    Such an msan-ctime test is added to the Linux x86_64 msan CI job, as an example. More CI cases could be added (e.g. for MacOs or ARM Linux) later.

  2. sipa force-pushed on Dec 6, 2022
  3. sipa force-pushed on Dec 7, 2022
  4. sipa force-pushed on Dec 7, 2022
  5. sipa force-pushed on Dec 7, 2022
  6. sipa force-pushed on Dec 7, 2022
  7. sipa force-pushed on Dec 7, 2022
  8. sipa force-pushed on Dec 7, 2022
  9. sipa force-pushed on Dec 7, 2022
  10. sipa force-pushed on Dec 20, 2022
  11. sipa commented at 3:49 AM on December 20, 2022: contributor

    Rebased after #1178 merge.

  12. sipa force-pushed on Dec 20, 2022
  13. in src/secp256k1.c:125 in f9e36d4c1f outdated 
     119 | @@ -116,7 +120,9 @@ secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigne
 120 |      /* Flags have been checked by secp256k1_context_preallocated_size. */
 121 |      VERIFY_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_CONTEXT);
 122 |      secp256k1_ecmult_gen_context_build(&ret->ecmult_gen_ctx);
 123 | +#if SECP256K1_CHECKMEM_ENABLED
 124 |      ret->declassify = !!(flags & SECP256K1_FLAGS_BIT_CONTEXT_DECLASSIFY);
 125 | +#endif
    real-or-random commented at 3:26 PM on December 20, 2022:

    Should this raise the illegal callback if SECP256K1_FLAGS_BIT_CONTEXT_DECLASSIFY && !SECP256K1_CHECKMEM_ENABLED?

    sipa commented at 5:27 PM on December 20, 2022:

    Added (as a runtime check, rather than a compile-time one) in a separate commit, in secp256k1_context_preallocated_size.

  14. in .cirrus.yml:331 in 6e99c5abfc outdated 
     327 | @@ -328,10 +328,11 @@ task:
 328 |      ECDH: yes
 329 |      RECOVERY: yes
 330 |      SCHNORRSIG: yes
 331 | -    CTIMETEST: no
 332 | +    CTIMETEST: yes
    real-or-random commented at 3:27 PM on December 20, 2022:

    nit: maybe rename this variable to plural, too

    sipa commented at 5:28 PM on December 20, 2022:

    Done.

  15. in src/secp256k1.c:74 in 6e99c5abfc outdated 
      71 | -    { secp256k1_default_error_callback_fn, 0 },
  72 | -    0
  73 | +    { secp256k1_default_error_callback_fn, 0 }
  74 | +#if SECP256K1_CHECKMEM_ENABLED
  75 | +    , 0
  76 | +#endif
    real-or-random commented at 3:33 PM on December 20, 2022:

    (mostly educational) nit. Trailing commas are allowed in initializer lists, even back in C89:

        { secp256k1_default_error_callback_fn, 0 },
#if SECP256K1_CHECKMEM_ENABLED
    0,
#endif
    sipa commented at 5:27 PM on December 20, 2022:

    TIL, thanks.

  16. real-or-random commented at 3:49 PM on December 20, 2022: contributor

    I'm not entirely sure about dropping the declassify member from the struct. I don't see anything immediately wrong about dropping it, but I think the reason why we currently always have it is to make sure the compiled code with valgrind support is as close as possible to the code without valgrind support. This makes testing easier because it (attempts to) avoid yet another config option (plus we save a few preprocessing directives).

  17. sipa force-pushed on Dec 20, 2022
  18. sipa commented at 5:28 PM on December 20, 2022: contributor

    @real-or-random I've removed the declassify compile-time dependence; it does make sense to minimize the differences with and without.

  19. in src/checkmem.h:66 in 2ce78dea5d outdated 
      61 | +#    include <valgrind/memcheck.h>
  62 | +#    define SECP256K1_CHECKMEM_ENABLED 1
  63 | +#    define SECP256K1_CHECKMEM_UNDEFINE(p, len) VALGRIND_MAKE_MEM_UNDEFINED((p), (len))
  64 | +#    define SECP256K1_CHECKMEM_DEFINE(p, len) VALGRIND_MAKE_MEM_DEFINED((p), (len))
  65 | +#    define SECP256K1_CHECKMEM_CHECK(p, len) VALGRIND_CHECK_MEM_IS_DEFINED((p), (len))
  66 | +#    define SECP256K1_CHECKMEM_RUNNING() (RUNNING_ON_VALGRIND)
    real-or-random commented at 8:21 PM on December 20, 2022:

    When playing around with this on valgrind and reading the memcheck docs, I figured out this trick, which you may or may not take.

         /* VALGRIND_MAKE_MEM_DEFINED returns 0 iff not running on memcheck.
      * This is more precise than the RUNNING_ON_VALGRIND macro, which
      * checks for valgrind in general instead of memcheck specifically. */
#    define SECP256K1_CHECKMEM_RUNNING() (VALGRIND_MAKE_MEM_DEFINED(NULL, 0) != 0)
    sipa commented at 8:26 PM on December 20, 2022:

    Done!

    I looked over the valgrind/*.h files briefly to find something like this, but missed this possibility.

  20. real-or-random commented at 8:21 PM on December 20, 2022: contributor

    Changes look good. I still want to test this locally on msan. I'll probably do this tomorrow and ACK it then.

  21. sipa force-pushed on Dec 20, 2022
  22. real-or-random approved
  23. real-or-random commented at 6:24 PM on December 21, 2022: contributor

    ACK 2788ae70c2bb444365881ce4727e5325b132b0f3 I also tested that the ctimetest fails on valgrind and msan when I introduce variable-time code

    Next step (after this PR) would then to run this on macos.

    Fwiw, if anyone wonders: Unfortunately, this doesn't work on mingw, which doesn't support sanitizers.

  24. sipa commented at 6:29 PM on December 21, 2022: contributor

    @real-or-random There appears to be some work on a clang/mingw crossover (https://github.com/mstorsjo/llvm-mingw), but I'm not sure it's mature enough to try (or whether it support msan; only ubsan and asan are listed).

  25. real-or-random commented at 8:32 PM on December 21, 2022: contributor

    @real-or-random There appears to be some work on a clang/mingw crossover (mstorsjo/llvm-mingw), but I'm not sure it's mature enough to try (or whether it support msan; only ubsan and asan are listed).

    It's probably not worth the effort. I'd expect that the differences between different LLVM OS targets are rather small, or at least in areas such as binary formats, linkage, symbols, etc. and not in actual code generation. This is even more true in our case because we don't really interact with the OS.

  26. in src/checkmem.h:47 in 2788ae70c2 outdated 
      42 | +/* If compiling under msan, map the SECP256K1_CHECKMEM_* functionality to msan.
  43 | + * Choose this preferentially, even when VALGRIND is defined, as msan-compiled
  44 | + * binaries can't be run under valgrind anyway. */
  45 | +#if defined(__has_feature)
  46 | +#  if __has_feature(memory_sanitizer)
  47 | +#    include <stddef.h>
    real-or-random commented at 2:23 PM on January 3, 2023:

    nit: Do we need this here? I think no. But we need it below where we use NULL.

    sipa commented at 3:16 PM on January 3, 2023:

    Fixed.

  27. sipa force-pushed on Jan 3, 2023
  28. real-or-random approved
  29. real-or-random commented at 3:25 PM on January 3, 2023: contributor

    ACK 6ef1afa3ff1437d47ce21aace4409a51ebf81aa5 I also tested that the ctimetest fails on valgrind and msan when I introduce variable-time code

  30. in Makefile.am:125 in 6ef1afa3ff outdated 
     115 | @@ -119,18 +116,19 @@ if USE_TESTS
 116 |  noinst_PROGRAMS += tests
 117 |  tests_SOURCES = src/tests.c
 118 |  tests_CPPFLAGS = $(SECP_INCLUDES) $(SECP_TEST_INCLUDES) $(SECP_CONFIG_DEFINES)
 119 | -if VALGRIND_ENABLED
    hebasto commented at 4:12 PM on January 3, 2023:

    fc2ff99cfdc7005598826d0485aabe3abb349525 removes the last usage of the VALGRIND_ENABLED. The following line can be removed now as well https://github.com/bitcoin-core/secp256k1/blob/31ed5386e8450845c2cdbf1af9dd257937c46e48/configure.ac#L228

    sipa commented at 9:35 PM on January 3, 2023:

    Fixed.

  31. hebasto commented at 4:58 PM on January 3, 2023: member

    Testing 6ef1afa3ff1437d47ce21aace4409a51ebf81aa5 on Ubuntu 22.04:

    $ ./autogen.sh
$ ./configure --enable-ctime-tests CFLAGS="-fsanitize=memory -O1" CC=clang
...
Build Options:
  with external callbacks = no
  with benchmarks         = yes
  with tests              = yes
  with ctime tests        = yes
  with coverage           = no
  with examples           = no
  module ecdh             = yes
  module recovery         = no
  module extrakeys        = yes
  module schnorrsig       = yes

  asm                     = x86_64
  ecmult window size      = 15
  ecmult gen prec. bits   = 4

  valgrind                = no
  CC                      = clang
  CPPFLAGS                =  
  SECP_CFLAGS             = -O2  -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wconditional-uninitialized -fvisibility=hidden 
  CFLAGS                  = -fsanitize=memory -O1
  LDFLAGS 
$ make clean
$ make
$ ./libtool --mode=execute ./ctime_tests 
==346524==WARNING: MemorySanitizer: use-of-uninitialized-value
    [#0](/github-metadata-backup-bitcoin-core-secp256k1/0/) 0x7f87d91c34e2 in secp256k1_scalar_mul secp256k1.c
    [#1](/github-metadata-backup-bitcoin-core-secp256k1/1/) 0x7f87d9193de5 in secp256k1_ecdsa_sign (/home/hebasto/git/secp256k1/.libs/libsecp256k1.so.1+0xbde5) (BuildId: 8a99d7711aeabba7b1dc311e74fedcef767eb76b)
    [#2](/github-metadata-backup-bitcoin-core-secp256k1/2/) 0x5618435418c7 in run_tests (/home/hebasto/git/secp256k1/.libs/ctime_tests+0xa48c7) (BuildId: 013cc8d2ba72c4e385d011449343a9ea11a16616)
    [#3](/github-metadata-backup-bitcoin-core-secp256k1/3/) 0x56184354145b in main (/home/hebasto/git/secp256k1/.libs/ctime_tests+0xa445b) (BuildId: 013cc8d2ba72c4e385d011449343a9ea11a16616)
    [#4](/github-metadata-backup-bitcoin-core-secp256k1/4/) 0x7f87d8e66d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    [#5](/github-metadata-backup-bitcoin-core-secp256k1/5/) 0x7f87d8e66e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    [#6](/github-metadata-backup-bitcoin-core-secp256k1/6/) 0x5618434bb3a4 in _start (/home/hebasto/git/secp256k1/.libs/ctime_tests+0x1e3a4) (BuildId: 013cc8d2ba72c4e385d011449343a9ea11a16616)

SUMMARY: MemorySanitizer: use-of-uninitialized-value secp256k1.c in secp256k1_scalar_mul
Exiting

    Is it expected?

  32. real-or-random commented at 7:37 PM on January 3, 2023: contributor

    @hebasto Yes, msan is not clever enough to work with our asm. Try disabling asm.

  33. sipa force-pushed on Jan 3, 2023
  34. real-or-random approved
  35. real-or-random commented at 2:15 PM on January 4, 2023: contributor

    ACK 39c35e727719536878521fdb0e41e1df8e466d80 I also tested that the ctimetest fails on valgrind and msan when I introduce variable-time code

  36. sipa force-pushed on Jan 9, 2023
  37. sipa commented at 4:19 PM on January 9, 2023: contributor

    Rebased after #1188 merge.

  38. real-or-random approved
  39. real-or-random commented at 4:25 PM on January 10, 2023: contributor

    ACK 472baae02b5ade196fdc1d229d3d52783bbc3b6f

    the fourth time :)

  40. real-or-random commented at 2:00 PM on January 11, 2023: contributor

    the fourth time :)

    needs rebase, so sorry...

  41. Move valgrind CPPFLAGS into SECP_CONFIG_DEFINES 4f1a54e41d
  42. Abstract interactions with valgrind behind new checkmem.h 0db05a770e
  43. Add compile-time error to valgrind_ctime_test 8dc64079eb
  44. Add support for msan integration to checkmem.h 8e11f89a68
  45. Update error messages to suggest msan as well 6eed6c18de
  46. Rename valgrind_ctime_test -> ctime_tests 5048be17e9
  47. Make ctime tests building configurable 18974061a3
  48. Run ctime test in Linux MSan CI job 5e2e6fcfc0
  49. Add runtime checking for DECLASSIFY flag 74b026f05d
  50. Rename CTIMETEST -> CTIMETESTS 0f088ec112
  51. sipa force-pushed on Jan 11, 2023
  52. sipa commented at 9:09 PM on January 11, 2023: contributor

    Rebased after merge of #1187.

  53. real-or-random approved
  54. real-or-random commented at 1:05 PM on January 12, 2023: contributor

    ACK 0f088ec11263261497661215c110a4c395acc0ac

  55. real-or-random requested review from hebasto on Jan 12, 2023
  56. hebasto approved
  57. hebasto commented at 9:25 AM on January 14, 2023: member

    ACK 0f088ec11263261497661215c110a4c395acc0ac, I have reviewed the code and it looks OK. Able to build ctime_tests using MSan.

  58. real-or-random merged this on Jan 16, 2023
  59. real-or-random closed this on Jan 16, 2023
  60. hebasto referenced this in commit c9af5208f6 on Jan 19, 2023
  61. hebasto referenced this in commit 2cd4e3c0a9 on Jan 19, 2023
  62. sipa referenced this in commit ad7433b140 on Jan 19, 2023
  63. dhruv referenced this in commit 4d33046ce3 on Feb 1, 2023
  64. dhruv referenced this in commit 55e7f2cf2b on Feb 2, 2023
  65. stratospher referenced this in commit 647f63669e on Feb 6, 2023
  66. dhruv referenced this in commit a4351c0df6 on Feb 20, 2023
  67. stratospher referenced this in commit 23f825fc8b on Feb 21, 2023
  68. hebasto commented at 5:20 PM on February 22, 2023: member

    From https://www.amongbytes.com/post/20210709-testing-constant-time/ it follows that MSan, due to its speed-optimized design, is less accurate than Valgrind.

    Should it be mentioned in this repo's docs somehow?

  69. real-or-random commented at 8:46 AM on February 23, 2023: contributor

    Valgrind is also better for other reasons: Because it works on the normal binary without instrumentation, it also covers ASM, and checked code is exactly what people will run in production.

    We currently don't have any docs for constant-time tests. Creating them will be the first step. And yes, then it probably makes sense to mention limitations.

  70. hebasto referenced this in commit 7c0cc5d976 on Mar 7, 2023
  71. dhruv referenced this in commit a5df79db12 on Mar 7, 2023
  72. dhruv referenced this in commit 77b510d84c on Mar 7, 2023
  73. sipa referenced this in commit 763079a3f1 on Mar 8, 2023
  74. div72 referenced this in commit 945b094575 on Mar 14, 2023
  75. dderjoel referenced this in commit f66b1c0a5d on May 23, 2023
  76. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  77. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  78. delta1 referenced this in commit 3f32c20932 on Aug 8, 2023
  79. delta1 referenced this in commit 31ac0c1081 on Aug 31, 2023
  80. janus referenced this in commit 4816e2a921 on Sep 3, 2023
  81. real-or-random referenced this in commit d926510cf7 on Feb 27, 2024
  82. backpacker69 referenced this in commit 26f6d832fb on Mar 5, 2024
  83. str4d referenced this in commit b350ac56ac on Jun 4, 2025
  84. Fabcien referenced this in commit 6100b27268 on Apr 8, 2026
  85. Fabcien referenced this in commit 637ed29acd on Apr 8, 2026
  86. Fabcien referenced this in commit 3a79decf7c on Apr 8, 2026
  87. Fabcien referenced this in commit ee7631af6c on Apr 8, 2026
Contributors
sipareal-or-randomhebasto
Linked (view graph)
#1196 Drop no longer used variables from the build system#1496 msan: notate variable assignments from assembly code
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:52 UTC