Repository: bitcoin-core/secp256k1 (c)

Issue:
Rename memczero
The list of reserved identifiers in C includes all functions names beginning with `mem` followed by a lowercase letter.  We are using the reserved function name `memczero`, which is undefined behaviour.

Pull request description:
Don't use reserved identifiers memczero and benchmark_verify_t
As identified in #829 and #833. Fixes #829.

Since we touch this anyway, this commit additionally makes the
identifiers in the benchmark files a little bit more consistent.

This is necessary before we can merge #833. I preferred a separate PR because it makes it easier to see the results of Travis in #833. 

Patch:
diff --git a/.travis.yml b/.travis.yml
index ce8d639..bcc8c21 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -33,7 +33,7 @@ env:
     - CFLAGS=-O0 CTIMETEST=no
     - ECMULTGENPRECISION=2
     - ECMULTGENPRECISION=8
-    - RUN_VALGRIND=yes BIGNUM=no ASM=x86_64 ECDH=yes  RECOVERY=yes EXPERIMENTAL=yes SCHNORRSIG=yes EXTRAFLAGS="--disable-openssl-tests" BUILD=
+    - RUN_VALGRIND=yes BIGNUM=no ASM=x86_64 EXPERIMENTAL=yes ECDH=yes  RECOVERY=yes EXTRAFLAGS="--disable-openssl-tests" BUILD=
 matrix:
   fast_finish: true
   include:
@@ -81,7 +81,7 @@ matrix:
             - libc6-dbg:i386
     # S390x build (big endian system)
     - compiler: gcc
-      env: HOST=s390x-unknown-linux-gnu ECDH=yes RECOVERY=yes EXPERIMENTAL=yes SCHNORRSIG=yes CTIMETEST=
+      env: HOST=s390x-unknown-linux-gnu ECDH=yes RECOVERY=yes EXPERIMENTAL=yes CTIMETEST=
       arch: s390x
 
 # We use this to install macOS dependencies instead of the built in `homebrew` plugin,
diff --git a/README.md b/README.md
index e070937..2602475 100644
--- a/README.md
+++ b/README.md
@@ -16,7 +16,7 @@ Features:
 * Very efficient implementation.
 * Suitable for embedded systems.
 * Optional module for public key recovery.
-* Optional module for ECDH key exchange.
+* Optional module for ECDH key exchange (experimental).
 
 Experimental features have not received enough scrutiny to satisfy the standard of quality of this library but are made available for testing and review by the community. The APIs of these features should not be considered stable.
 
diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4
index ece3d65..57595f4 100644
--- a/build-aux/m4/bitcoin_secp.m4
+++ b/build-aux/m4/bitcoin_secp.m4
@@ -36,39 +36,16 @@ if test x"$has_libcrypto" = x"yes" && test x"$has_openssl_ec" = x; then
   CPPFLAGS_TEMP="$CPPFLAGS"
   CPPFLAGS="$CRYPTO_CPPFLAGS $CPPFLAGS"
   AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-    #include <openssl/bn.h>
     #include <openssl/ec.h>
     #include <openssl/ecdsa.h>
     #include <openssl/obj_mac.h>]],[[
-    # if OPENSSL_VERSION_NUMBER < 0x10100000L
-    void ECDSA_SIG_get0(const ECDSA_SIG *sig, const BIGNUM **pr, const BIGNUM **ps) {(void)sig->r; (void)sig->s;}
-    # endif
-
-    unsigned int zero = 0;
-    const unsigned char *zero_ptr = (unsigned char*)&zero;
-    EC_KEY_free(EC_KEY_new_by_curve_name(NID_secp256k1));
-    EC_KEY *eckey = EC_KEY_new();
-    EC_GROUP *group = EC_GROUP_new_by_curve_name(NID_secp256k1);
-    EC_KEY_set_group(eckey, group);
-    ECDSA_sign(0, NULL, 0, NULL, &zero, eckey);
+    EC_KEY *eckey = EC_KEY_new_by_curve_name(NID_secp256k1);
+    ECDSA_sign(0, NULL, 0, NULL, NULL, eckey);
     ECDSA_verify(0, NULL, 0, NULL, 0, eckey);
-    o2i_ECPublicKey(&eckey, &zero_ptr, 0);
-    d2i_ECPrivateKey(&eckey, &zero_ptr, 0);
-    EC_KEY_check_key(eckey);
     EC_KEY_free(eckey);
-    EC_GROUP_free(group);
     ECDSA_SIG *sig_openssl;
     sig_openssl = ECDSA_SIG_new();
-    d2i_ECDSA_SIG(&sig_openssl, &zero_ptr, 0);
-    i2d_ECDSA_SIG(sig_openssl, NULL);
-    ECDSA_SIG_get0(sig_openssl, NULL, NULL);
     ECDSA_SIG_free(sig_openssl);
-    const BIGNUM *bignum = BN_value_one();
-    BN_is_negative(bignum);
-    BN_num_bits(bignum);
-    if (sizeof(zero) >= BN_num_bytes(bignum)) {
-        BN_bn2bin(bignum, (unsigned char*)&zero);
-    }
   ]])],[has_openssl_ec=yes],[has_openssl_ec=no])
   AC_MSG_RESULT([$has_openssl_ec])
   CPPFLAGS="$CPPFLAGS_TEMP"
diff --git a/configure.ac b/configure.ac
index eb3b449..5a078e6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -122,7 +122,7 @@ AC_ARG_ENABLE(ecmult_static_precomputation,
     [use_ecmult_static_precomputation=auto])
 
 AC_ARG_ENABLE(module_ecdh,
-    AS_HELP_STRING([--enable-module-ecdh],[enable ECDH shared secret computation]),
+    AS_HELP_STRING([--enable-module-ecdh],[enable ECDH shared secret computation (experimental)]),
     [enable_module_ecdh=$enableval],
     [enable_module_ecdh=no])
 
@@ -395,8 +395,8 @@ esac
 
 if test x"$use_tests" = x"yes"; then
   SECP_OPENSSL_CHECK
-  if test x"$enable_openssl_tests" != x"no" && test x"$has_openssl_ec" = x"yes"; then
-      enable_openssl_tests=yes
+  if test x"$has_openssl_ec" = x"yes"; then
+    if test x"$enable_openssl_tests" != x"no"; then
       AC_DEFINE(ENABLE_OPENSSL_TESTS, 1, [Define this symbol if OpenSSL EC functions are available])
       SECP_TEST_INCLUDES="$SSL_CFLAGS $CRYPTO_CFLAGS $CRYPTO_CPPFLAGS"
       SECP_TEST_LIBS="$CRYPTO_LIBS"
@@ -406,17 +406,16 @@ if test x"$use_tests" = x"yes"; then
         SECP_TEST_LIBS="$SECP_TEST_LIBS -lgdi32"
         ;;
       esac
+    fi
   else
     if test x"$enable_openssl_tests" = x"yes"; then
       AC_MSG_ERROR([OpenSSL tests requested but OpenSSL with EC support is not available])
     fi
-    enable_openssl_tests=no
   fi
 else
   if test x"$enable_openssl_tests" = x"yes"; then
     AC_MSG_ERROR([OpenSSL tests requested but tests are not enabled])
   fi
-  enable_openssl_tests=no
 fi
 
 if test x"$set_bignum" = x"gmp"; then
@@ -459,10 +458,14 @@ 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([Building ECDH module: $enable_module_ecdh])
   AC_MSG_NOTICE([Building extrakeys module: $enable_module_extrakeys])
   AC_MSG_NOTICE([Building schnorrsig module: $enable_module_schnorrsig])
   AC_MSG_NOTICE([******])
 else
+  if test x"$enable_module_ecdh" = x"yes"; then
+    AC_MSG_ERROR([ECDH module is experimental. Use --enable-experimental to allow.])
+  fi
   if test x"$enable_module_extrakeys" = x"yes"; then
     AC_MSG_ERROR([extrakeys module is experimental. Use --enable-experimental to allow.])
   fi
@@ -504,8 +507,6 @@ echo "Build Options:"
 echo "  with ecmult precomp     = $set_precomp"
 echo "  with external callbacks = $use_external_default_callbacks"
 echo "  with benchmarks         = $use_benchmark"
-echo "  with tests              = $use_tests"
-echo "  with openssl tests      = $enable_openssl_tests"
 echo "  with coverage           = $enable_coverage"
 echo "  module ecdh             = $enable_module_ecdh"
 echo "  module recovery         = $enable_module_recovery"
diff --git a/src/bench_sign.c b/src/bench_sign.c
index c6b2942..0fd6c00 100644
--- a/src/bench_sign.c
+++ b/src/bench_sign.c
@@ -12,11 +12,11 @@ typedef struct {
     secp256k1_context* ctx;
     unsigned char msg[32];
     unsigned char key[32];
-} bench_sign;
+} bench_sign_data;
 
 static void bench_sign_setup(void* arg) {
     int i;
-    bench_sign *data = (bench_sign*)arg;
+    bench_sign_data *data = (bench_sign_data*)arg;
 
     for (i = 0; i < 32; i++) {
         data->msg[i] = i + 1;
@@ -28,7 +28,7 @@ static void bench_sign_setup(void* arg) {
 
 static void bench_sign_run(void* arg, int iters) {
     int i;
-    bench_sign *data = (bench_sign*)arg;
+    bench_sign_data *data = (bench_sign_data*)arg;
 
     unsigned char sig[74];
     for (i = 0; i < iters; i++) {
@@ -45,7 +45,7 @@ static void bench_sign_run(void* arg, int iters) {
 }
 
 int main(void) {
-    bench_sign data;
+    bench_sign_data data;
 
     int iters = get_iters(20000);
 
diff --git a/src/bench_verify.c b/src/bench_verify.c
index 272d3e5..c9efa5f 100644
--- a/src/bench_verify.c
+++ b/src/bench_verify.c
@@ -29,11 +29,11 @@ typedef struct {
 #ifdef ENABLE_OPENSSL_TESTS
     EC_GROUP* ec_group;
 #endif
-} benchmark_verify_t;
+} bench_verify_data;
 
-static void benchmark_verify(void* arg, int iters) {
+static void bench_verify(void* arg, int iters) {
     int i;
-    benchmark_verify_t* data = (benchmark_verify_t*)arg;
+    bench_verify_data* data = (bench_verify_data*)arg;
 
     for (i = 0; i < iters; i++) {
         secp256k1_pubkey pubkey;
@@ -51,9 +51,9 @@ static void benchmark_verify(void* arg, int iters) {
 }
 
 #ifdef ENABLE_OPENSSL_TESTS
-static void benchmark_verify_openssl(void* arg, int iters) {
+static void bench_verify_openssl(void* arg, int iters) {
     int i;
-    benchmark_verify_t* data = (benchmark_verify_t*)arg;
+    bench_verify_data* data = (bench_verify_data*)arg;
 
     for (i = 0; i < iters; i++) {
         data->sig[data->siglen - 1] ^= (i & 0xFF);
@@ -84,7 +84,7 @@ int main(void) {
     int i;
     secp256k1_pubkey pubkey;
     secp256k1_ecdsa_signature sig;
-    benchmark_verify_t data;
+    bench_verify_data data;
 
     int iters = get_iters(20000);
 
@@ -103,10 +103,10 @@ int main(void) {
     data.pubkeylen = 33;
     CHECK(secp256k1_ec_pubkey_serialize(data.ctx, data.pubkey, &data.pubkeylen, &pubkey, SECP256K1_EC_COMPRESSED) == 1);
 
-    run_benchmark("ecdsa_verify", benchmark_verify, NULL, NULL, &data, 10, iters);
+    run_benchmark("ecdsa_verify", bench_verify, NULL, NULL, &data, 10, iters);
 #ifdef ENABLE_OPENSSL_TESTS
     data.ec_group = EC_GROUP_new_by_curve_name(NID_secp256k1);
-    run_benchmark("ecdsa_verify_openssl", benchmark_verify_openssl, NULL, NULL, &data, 10, iters);
+    run_benchmark("ecdsa_verify_openssl", bench_verify_openssl, NULL, NULL, &data, 10, iters);
     EC_GROUP_free(data.ec_group);
 #endif
 
diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h
index a9e8b3c..057a69c 100644
--- a/src/ecmult_impl.h
+++ b/src/ecmult_impl.h
@@ -443,7 +443,7 @@ struct secp256k1_strauss_state {
     struct secp256k1_strauss_point_state* ps;
 };
 
-static void secp256k1_ecmult_strauss_wnaf(const secp256k1_ecmult_context *ctx, const struct secp256k1_strauss_state *state, secp256k1_gej *r, size_t num, const secp256k1_gej *a, const secp256k1_scalar *na, const secp256k1_scalar *ng) {
+static void secp256k1_ecmult_strauss_wnaf(const secp256k1_ecmult_context *ctx, const struct secp256k1_strauss_state *state, secp256k1_gej *r, int num, const secp256k1_gej *a, const secp256k1_scalar *na, const secp256k1_scalar *ng) {
     secp256k1_ge tmpa;
     secp256k1_fe Z;
     /* Splitted G factors. */
@@ -454,8 +454,8 @@ static void secp256k1_ecmult_strauss_wnaf(const secp256k1_ecmult_context *ctx, c
     int bits_ng_128 = 0;
     int i;
     int bits = 0;
-    size_t np;
-    size_t no = 0;
+    int np;
+    int no = 0;
 
     for (np = 0; np < num; ++np) {
         if (secp256k1_scalar_is_zero(&na[np]) || secp256k1_gej_is_infinity(&a[np])) {
diff --git a/src/modules/extrakeys/main_impl.h b/src/modules/extrakeys/main_impl.h
index 5378d2f..e365f92 100644
--- a/src/modules/extrakeys/main_impl.h
+++ b/src/modules/extrakeys/main_impl.h
@@ -180,7 +180,7 @@ int secp256k1_keypair_create(const secp256k1_context* ctx, secp256k1_keypair *ke
 
     ret = secp256k1_ec_pubkey_create_helper(&ctx->ecmult_gen_ctx, &sk, &pk, seckey32);
     secp256k1_keypair_save(keypair, &sk, &pk);
-    memczero(keypair, sizeof(*keypair), !ret);
+    secp256k1_memczero(keypair, sizeof(*keypair), !ret);
 
     secp256k1_scalar_clear(&sk);
     return ret;
diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h
index b0d8481..da747fe 100644
--- a/src/modules/schnorrsig/main_impl.h
+++ b/src/modules/schnorrsig/main_impl.h
@@ -179,7 +179,7 @@ int secp256k1_schnorrsig_sign(const secp256k1_context* ctx, unsigned char *sig64
     secp256k1_scalar_add(&e, &e, &k);
     secp256k1_scalar_get_b32(&sig64[32], &e);
 
-    memczero(sig64, 64, !ret);
+    secp256k1_memczero(sig64, 64, !ret);
     secp256k1_scalar_clear(&k);
     secp256k1_scalar_clear(&sk);
     memset(seckey, 0, sizeof(seckey));
diff --git a/src/secp256k1.c b/src/secp256k1.c
index dae506d..628ba56 100644
--- a/src/secp256k1.c
+++ b/src/secp256k1.c
@@ -580,7 +580,7 @@ int secp256k1_ec_pubkey_create(const secp256k1_context* ctx, secp256k1_pubkey *p
 
     ret = secp256k1_ec_pubkey_create_helper(&ctx->ecmult_gen_ctx, &seckey_scalar, &p, seckey);
     secp256k1_pubkey_save(pubkey, &p);
-    memczero(pubkey, sizeof(*pubkey), !ret);
+    secp256k1_memczero(pubkey, sizeof(*pubkey), !ret);
 
     secp256k1_scalar_clear(&seckey_scalar);
     return ret;
diff --git a/src/util.h b/src/util.h
index 3a88a41..2a9eb5a 100644
--- a/src/util.h
+++ b/src/util.h
@@ -202,7 +202,7 @@ static SECP256K1_INLINE void *manual_alloc(void** prealloc_ptr, size_t alloc_siz
 #endif
 
 /* Zero memory if flag == 1. Flag must be 0 or 1. Constant time. */
-static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
+static SECP256K1_INLINE void secp256k1_memczero(void *s, size_t len, int flag) {
     unsigned char *p = (unsigned char *)s;
     /* Access flag with a volatile-qualified lvalue.
        This prevents clang from figuring out (after inlining) that flag can
@@ -260,14 +260,20 @@ static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag)
 # define SECP256K1_WIDEMUL_INT128 1
 #elif defined(USE_FORCE_WIDEMUL_INT64)
 # define SECP256K1_WIDEMUL_INT64 1
-#elif defined(__SIZEOF_INT128__)
+#elif defined(UINT128_MAX) || defined(__SIZEOF_INT128__)
 # define SECP256K1_WIDEMUL_INT128 1
 #else
 # define SECP256K1_WIDEMUL_INT64 1
 #endif
 #if defined(SECP256K1_WIDEMUL_INT128)
+# if !defined(UINT128_MAX) && defined(__SIZEOF_INT128__)
 SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t;
 SECP256K1_GNUC_EXT typedef __int128 int128_t;
+#define UINT128_MAX ((uint128_t)(-1))
+#define INT128_MAX ((int128_t)(UINT128_MAX >> 1))
+#define INT128_MIN (-INT128_MAX - 1)
+/* No (U)INT128_C macros because compilers providing __int128 do not support 128-bit literals.  */
+# endif
 #endif
 
 #endif /* SECP256K1_UTIL_H */


Test patch:
diff --git a/src/tests.c b/src/tests.c
index bb4b5b4..8ad1a08 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -5444,18 +5444,18 @@ void run_ecdsa_openssl(void) {
 # include "modules/schnorrsig/tests_impl.h"
 #endif
 
-void run_memczero_test(void) {
+void run_secp256k1_memczero_test(void) {
     unsigned char buf1[6] = {1, 2, 3, 4, 5, 6};
     unsigned char buf2[sizeof(buf1)];
 
-    /* memczero(..., ..., 0) is a noop. */
+    /* secp256k1_memczero(..., ..., 0) is a noop. */
     memcpy(buf2, buf1, sizeof(buf1));
-    memczero(buf1, sizeof(buf1), 0);
+    secp256k1_memczero(buf1, sizeof(buf1), 0);
     CHECK(secp256k1_memcmp_var(buf1, buf2, sizeof(buf1)) == 0);
 
-    /* memczero(..., ..., 1) zeros the buffer. */
+    /* secp256k1_memczero(..., ..., 1) zeros the buffer. */
     memset(buf2, 0, sizeof(buf2));
-    memczero(buf1, sizeof(buf1) , 1);
+    secp256k1_memczero(buf1, sizeof(buf1) , 1);
     CHECK(secp256k1_memcmp_var(buf1, buf2, sizeof(buf1)) == 0);
 }
 
@@ -5723,7 +5723,7 @@ int main(int argc, char **argv) {
 #endif
 
     /* util tests */
-    run_memczero_test();
+    run_secp256k1_memczero_test();
 
     run_cmov_tests();