Automatically check that VERIFY_CHECKs are side effect free

.cirrus.yml

@@ -16,6 +16,7 @@ env:
   BENCH: yes
   ITERS: 2
   MAKEFLAGS: -j2
+  CHECK_SIDE_EFFECT_FREE: yes
 
 cat_logs_snippet: &CAT_LOGS
   always:
@@ -62,7 +63,7 @@ task:
     - env: {                  STATICPRECOMPUTATION: no}
     - env: {BUILD: distcheck, WITH_VALGRIND: no, CTIMETEST: no, BENCH: no}
     - env: {CPPFLAGS: -DDETERMINISTIC}
-    - env: {CFLAGS: -O0, CTIMETEST: no}
+    - env: {CFLAGS: -O0, CTIMETEST: no, CHECK_SIDE_EFFECT_FREE: no}
     - env:
         CFLAGS:  "-fsanitize=undefined -fno-omit-frame-pointer"
         LDFLAGS: "-fsanitize=undefined -fno-omit-frame-pointer"
@@ -73,6 +74,7 @@ task:
         EXPERIMENTAL: yes
         SCHNORRSIG: yes
         CTIMETEST: no
+        CHECK_SIDE_EFFECT_FREE: auto
     - env: { ECMULTGENPRECISION: 2 }
     - env: { ECMULTGENPRECISION: 8 }
     - env:

build-aux/m4/bitcoin_secp.m4

@@ -9,6 +9,37 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
 AC_MSG_RESULT([$has_64bit_asm])
 ])
 
+AC_DEFUN([SECP_CHECK_SIDE_EFFECT_FREE],[
+AC_MSG_CHECKING(whether compiler detects side effect free statements)
+AC_LINK_IFELSE([AC_LANG_SOURCE([[
+  #include <stdlib.h>
+  static int my_memcmp(const void *s1, const void *s2, size_t n) {
+    const unsigned char *p1 = s1, *p2 = s2;
+    size_t i;
+    for (i = 0; i < n; i++) {
+        int diff = p1[i] - p2[i];
+        if (diff != 0) {
+            return diff;
+        }
+    }
+    return 0;
+  }
+
+  static int side_effect_free(int val, const int* ptr) {
+      static const unsigned char foo[32] = {1,2,3};
+      if (!my_memcmp(ptr, foo, val)) return 0;
+      return (((val + 13) * ptr[0] ^ 7) * ptr[ptr[1]] / 11) * (int)ptr == 0;
+  }
+
+  extern int should_not_survive;
+  int main(int argc, char** argv) {
+      (void)(should_not_survive || side_effect_free(argc, (const int*)argv));
+      return 0;
+  }
+  ]])],[has_check_side_effect_free=yes],[has_check_side_effect_free=no])
+AC_MSG_RESULT([$has_check_side_effect_free])
+])
+
 dnl
 AC_DEFUN([SECP_OPENSSL_CHECK],[
   has_libcrypto=no

ci/cirrus.sh

@@ -19,6 +19,7 @@ valgrind --version || true
     --enable-module-ecdh="$ECDH" --enable-module-recovery="$RECOVERY" \
     --enable-module-schnorrsig="$SCHNORRSIG" \
     --with-valgrind="$WITH_VALGRIND" \
+    --enable-side-effect-free-check="$CHECK_SIDE_EFFECT_FREE" \
     --host="$HOST" $EXTRAFLAGS
 
 # We have set "-j<n>" in MAKEFLAGS.

configure.ac

@@ -155,6 +155,11 @@ AC_ARG_ENABLE(external_default_callbacks,
     [use_external_default_callbacks=$enableval],
     [use_external_default_callbacks=no])
 
+AC_ARG_ENABLE(side_effect_free_check,
+    AS_HELP_STRING([--enable-side-effect-free-check],[enable checking assertions are side-effect free (yes/no/auto) [default=no]]),
+    [use_side_effect_free_check=$enableval],
+    [use_side_effect_free_check=no])
+
 # Test-only override of the (autodetected by the C code) "widemul" setting.
 # Legal values are int64 (for [u]int64_t), int128 (for [unsigned] __int128), and auto (the default).
 AC_ARG_WITH([test-override-wide-multiply], [] ,[set_widemul=$withval], [set_widemul=auto])
@@ -210,6 +215,18 @@ else
     CFLAGS="-O2 $CFLAGS"
 fi
 
+set_check_side_effect_free=no
+if test x"$use_side_effect_free_check" != x"no"; then
+  SECP_CHECK_SIDE_EFFECT_FREE
+  if test x"$has_check_side_effect_free" = x"yes"; then
+    set_check_side_effect_free=yes
+  else
+    if test x"$use_side_effect_free_check" != x"auto"; then
+      AC_MSG_ERROR([Side effect free checking requested but not compatible with compiler])
+    fi
+  fi
+fi
+
 if test x"$req_asm" = x"auto"; then
   SECP_64BIT_ASM_CHECK
   if test x"$has_64bit_asm" = x"yes"; then
@@ -258,6 +275,16 @@ if test x"$use_external_asm" = x"yes"; then
   AC_DEFINE(USE_EXTERNAL_ASM, 1, [Define this symbol if an external (non-inline) assembly implementation is used])
 fi
 
+case $set_check_side_effect_free in
+yes)
+  AC_DEFINE(CHECK_SIDE_EFFECT_FREE, 1, [Define this symbol to enable checks for side-effect free statements])
+  ;;
+no)
+  ;;
+*)
+  AC_MSG_ERROR([invalid selection for check_side_effect_free])
+  ;;
+esac
 
 # Select wide multiplication implementation
 case $set_widemul in

src/field_10x26_impl.h

@@ -275,10 +275,9 @@ SECP256K1_INLINE static void secp256k1_fe_set_int(secp256k1_fe *r, int a) {
 
 SECP256K1_INLINE static int secp256k1_fe_is_zero(const secp256k1_fe *a) {
     const uint32_t *t = a->n;
-#ifdef VERIFY
-    VERIFY_CHECK(a->normalized);
-    secp256k1_fe_verify(a);
-#endif
+    /* No secp256k1_fe_verify is needed here, because if all limbs are 0,
+     * the representation is always valid. This also allows using this function
+     * inside VERIFY_CHECK conditions itself without side effects. */
     return (t[0] | t[1] | t[2] | t[3] | t[4] | t[5] | t[6] | t[7] | t[8] | t[9]) == 0;
 }
 
@@ -455,11 +454,7 @@ void secp256k1_fe_sqr_inner(uint32_t *r, const uint32_t *a);
 
 #else
 
-#ifdef VERIFY
 #define VERIFY_BITS(x, n) VERIFY_CHECK(((x) >> (n)) == 0)
-#else
-#define VERIFY_BITS(x, n) do { } while(0)
-#endif
 
 SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint32_t *r, const uint32_t *a, const uint32_t * SECP256K1_RESTRICT b) {
     uint64_t c, d;
@@ -1135,9 +1130,7 @@ static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r,
 }
 
 static void secp256k1_fe_to_storage(secp256k1_fe_storage *r, const secp256k1_fe *a) {
-#ifdef VERIFY
-    VERIFY_CHECK(a->normalized);
-#endif
+    VERIFY_CHECK_ONLY(a->normalized);
     r->n[0] = a->n[0] | a->n[1] << 26;
     r->n[1] = a->n[1] >> 6 | a->n[2] << 20;
     r->n[2] = a->n[2] >> 12 | a->n[3] << 14;
@@ -1206,9 +1199,7 @@ static void secp256k1_fe_to_signed30(secp256k1_modinv32_signed30 *r, const secp2
     const uint64_t a0 = a->n[0], a1 = a->n[1], a2 = a->n[2], a3 = a->n[3], a4 = a->n[4],
                    a5 = a->n[5], a6 = a->n[6], a7 = a->n[7], a8 = a->n[8], a9 = a->n[9];
 
-#ifdef VERIFY
-    VERIFY_CHECK(a->normalized);
-#endif
+    VERIFY_CHECK_ONLY(a->normalized);
 
     r->v[0] = (a0       | a1 << 26) & M30;
     r->v[1] = (a1 >>  4 | a2 << 22) & M30;

src/field_5x52_impl.h

@@ -238,10 +238,9 @@ SECP256K1_INLINE static void secp256k1_fe_set_int(secp256k1_fe *r, int a) {
 
 SECP256K1_INLINE static int secp256k1_fe_is_zero(const secp256k1_fe *a) {
     const uint64_t *t = a->n;
-#ifdef VERIFY
-    VERIFY_CHECK(a->normalized);
-    secp256k1_fe_verify(a);
-#endif
+    /* No secp256k1_fe_verify is needed here, because if all limbs are 0,
+     * the representation is always valid. This also allows using this function
+     * inside VERIFY_CHECK conditions itself without side effects. */
     return (t[0] | t[1] | t[2] | t[3] | t[4]) == 0;
 }
 
@@ -478,9 +477,7 @@ static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r,
 }
 
 static void secp256k1_fe_to_storage(secp256k1_fe_storage *r, const secp256k1_fe *a) {
-#ifdef VERIFY
-    VERIFY_CHECK(a->normalized);
-#endif
+    VERIFY_CHECK_ONLY(a->normalized);
     r->n[0] = a->n[0] | a->n[1] << 52;
     r->n[1] = a->n[1] >> 12 | a->n[2] << 40;
     r->n[2] = a->n[2] >> 24 | a->n[3] << 28;
@@ -529,9 +526,7 @@ static void secp256k1_fe_to_signed62(secp256k1_modinv64_signed62 *r, const secp2
     const uint64_t M62 = UINT64_MAX >> 2;
     const uint64_t a0 = a->n[0], a1 = a->n[1], a2 = a->n[2], a3 = a->n[3], a4 = a->n[4];
 
-#ifdef VERIFY
-    VERIFY_CHECK(a->normalized);
-#endif
+    VERIFY_CHECK_ONLY(a->normalized);
 
     r->v[0] = (a0       | a1 << 52) & M62;
     r->v[1] = (a1 >> 10 | a2 << 42) & M62;
@@ -555,9 +550,7 @@ static void secp256k1_fe_inv(secp256k1_fe *r, const secp256k1_fe *x) {
     secp256k1_modinv64(&s, &secp256k1_const_modinfo_fe);
     secp256k1_fe_from_signed62(r, &s);
 
-#ifdef VERIFY
     VERIFY_CHECK(secp256k1_fe_normalizes_to_zero(r) == secp256k1_fe_normalizes_to_zero(&tmp));
-#endif
 }
 
 static void secp256k1_fe_inv_var(secp256k1_fe *r, const secp256k1_fe *x) {
@@ -570,9 +563,7 @@ static void secp256k1_fe_inv_var(secp256k1_fe *r, const secp256k1_fe *x) {
     secp256k1_modinv64_var(&s, &secp256k1_const_modinfo_fe);
     secp256k1_fe_from_signed62(r, &s);
 
-#ifdef VERIFY
     VERIFY_CHECK(secp256k1_fe_normalizes_to_zero(r) == secp256k1_fe_normalizes_to_zero(&tmp));
-#endif
 }
 
 #endif /* SECP256K1_FIELD_REPR_IMPL_H */

src/field_5x52_int128_impl.h

@@ -9,11 +9,7 @@
 
 #include <stdint.h>
 
-#ifdef VERIFY
 #define VERIFY_BITS(x, n) VERIFY_CHECK(((x) >> (n)) == 0)
-#else
-#define VERIFY_BITS(x, n) do { } while(0)
-#endif
 
 SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint64_t *r, const uint64_t *a, const uint64_t * SECP256K1_RESTRICT b) {
     uint128_t c, d;