From 914276e4d27a5f21407744d8016b6d0789e676b1 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 28 Jan 2023 10:15:33 +0100 Subject: [PATCH 1/4] build: Add SECP256K1_API_VAR to fix importing variables from DLLs This fixes a build issue with MSVC. While MSVC imports *functions* from DLLs automatically when building a consumer of the DLL, it does not import *variables* automatically. In these cases, we need an explicit __declspec(dllimport). This commit simply changes our logic to what the libtool manual suggests, which has a very comprehensive writeup on the topic. Note that in particular, this solution is carefully designed not to break static linking. However, as described in the libtool manual, statically linking the library with MSVC will output warning LNK4217. This is still the best solution overall, because the warning is merely a cosmetic issue. --- include/secp256k1.h | 39 ++++++++++++++++++++-------------- include/secp256k1_ecdh.h | 4 ++-- include/secp256k1_schnorrsig.h | 2 +- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/include/secp256k1.h b/include/secp256k1.h index 3a75b05000c3e..325f35eb0488b 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -145,21 +145,28 @@ typedef int (*secp256k1_nonce_function)( # define SECP256K1_NO_BUILD #endif -/** At secp256k1 build-time DLL_EXPORT is defined when building objects destined - * for a shared library, but not for those intended for static libraries. - */ - -#ifndef SECP256K1_API -# if defined(_WIN32) -# if defined(SECP256K1_BUILD) && defined(DLL_EXPORT) -# define SECP256K1_API __declspec(dllexport) -# else -# define SECP256K1_API +/* Symbol visibility. See libtool manual, section "Windows DLLs". */ +#if defined(_WIN32) && !defined(__GNUC__) +# ifdef SECP256K1_BUILD +# ifdef DLL_EXPORT +# define SECP256K1_API __declspec (dllexport) +# define SECP256K1_API_VAR extern __declspec (dllexport) # endif -# elif defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD) -# define SECP256K1_API __attribute__ ((visibility ("default"))) +# elif defined _MSC_VER +# define SECP256K1_API +# define SECP256K1_API_VAR extern __declspec (dllimport) +# elif defined DLL_EXPORT +# define SECP256K1_API __declspec (dllimport) +# define SECP256K1_API_VAR extern __declspec (dllimport) +# endif +#endif +#ifndef SECP256K1_API +# if defined(__GNUC__) && (__GNUC__ >= 4) && defined(SECP256K1_BUILD) +# define SECP256K1_API __attribute__ ((visibility ("default"))) +# define SECP256K1_API_VAR extern __attribute__ ((visibility ("default"))) # else # define SECP256K1_API +# define SECP256K1_API_VAR extern # endif #endif @@ -231,10 +238,10 @@ typedef int (*secp256k1_nonce_function)( * * It is highly recommended to call secp256k1_selftest before using this context. */ -SECP256K1_API extern const secp256k1_context *secp256k1_context_static; +SECP256K1_API_VAR const secp256k1_context *secp256k1_context_static; /** Deprecated alias for secp256k1_context_static. */ -SECP256K1_API extern const secp256k1_context *secp256k1_context_no_precomp +SECP256K1_API_VAR const secp256k1_context *secp256k1_context_no_precomp SECP256K1_DEPRECATED("Use secp256k1_context_static instead"); /** Perform basic self tests (to be used in conjunction with secp256k1_context_static) @@ -631,10 +638,10 @@ SECP256K1_API int secp256k1_ecdsa_signature_normalize( * If a data pointer is passed, it is assumed to be a pointer to 32 bytes of * extra entropy. */ -SECP256K1_API extern const secp256k1_nonce_function secp256k1_nonce_function_rfc6979; +SECP256K1_API_VAR const secp256k1_nonce_function secp256k1_nonce_function_rfc6979; /** A default safe nonce generation function (currently equal to secp256k1_nonce_function_rfc6979). */ -SECP256K1_API extern const secp256k1_nonce_function secp256k1_nonce_function_default; +SECP256K1_API_VAR const secp256k1_nonce_function secp256k1_nonce_function_default; /** Create an ECDSA signature. * diff --git a/include/secp256k1_ecdh.h b/include/secp256k1_ecdh.h index c8577984b1a5e..625061b282ebf 100644 --- a/include/secp256k1_ecdh.h +++ b/include/secp256k1_ecdh.h @@ -27,11 +27,11 @@ typedef int (*secp256k1_ecdh_hash_function)( /** An implementation of SHA256 hash function that applies to compressed public key. * Populates the output parameter with 32 bytes. */ -SECP256K1_API extern const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_sha256; +SECP256K1_API_VAR const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_sha256; /** A default ECDH hash function (currently equal to secp256k1_ecdh_hash_function_sha256). * Populates the output parameter with 32 bytes. */ -SECP256K1_API extern const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default; +SECP256K1_API_VAR const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default; /** Compute an EC Diffie-Hellman secret in constant time * diff --git a/include/secp256k1_schnorrsig.h b/include/secp256k1_schnorrsig.h index e579e1b1d834b..4cd2d982565f6 100644 --- a/include/secp256k1_schnorrsig.h +++ b/include/secp256k1_schnorrsig.h @@ -61,7 +61,7 @@ typedef int (*secp256k1_nonce_function_hardened)( * Therefore, to create BIP-340 compliant signatures, algo must be set to * "BIP0340/nonce" and algolen to 13. */ -SECP256K1_API extern const secp256k1_nonce_function_hardened secp256k1_nonce_function_bip340; +SECP256K1_API_VAR const secp256k1_nonce_function_hardened secp256k1_nonce_function_bip340; /** Data structure that contains additional arguments for schnorrsig_sign_custom. * From 739c53b19a22bd8cd251e25ea286089664a2f0eb Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 6 Feb 2023 21:31:47 +0100 Subject: [PATCH 2/4] examples: Extend sig examples by call that uses static context Besides improving the examples, this makes sure that the examples import a variable (instead of a function), namely the static context, from the library. This is helpful when testing MSVC builds, because the MSVC linker tends to be awkward when importing variables. --- examples/ecdsa.c | 12 ++++++++++-- examples/schnorr.c | 11 ++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/examples/ecdsa.c b/examples/ecdsa.c index 7e4f1b13ac86d..01088e31035af 100644 --- a/examples/ecdsa.c +++ b/examples/ecdsa.c @@ -34,7 +34,7 @@ int main(void) { unsigned char compressed_pubkey[33]; unsigned char serialized_signature[64]; size_t len; - int is_signature_valid; + int is_signature_valid, is_signature_valid2; int return_val; secp256k1_pubkey pubkey; secp256k1_ecdsa_signature sig; @@ -116,10 +116,18 @@ int main(void) { printf("Signature: "); print_hex(serialized_signature, sizeof(serialized_signature)); - /* This will clear everything from the context and free the memory */ secp256k1_context_destroy(ctx); + /* Bonus example: if all we need is signature verification (and no key + generation or signing), we don't need to use a context created via + secp256k1_context_create(). We can simply use the static (i.e., global) + context secp256k1_context_static. See its description in + include/secp256k1.h for details. */ + is_signature_valid2 = secp256k1_ecdsa_verify(secp256k1_context_static, + &sig, msg_hash, &pubkey); + assert(is_signature_valid2 == is_signature_valid); + /* It's best practice to try to clear secrets from memory after using them. * This is done because some bugs can allow an attacker to leak memory, for * example through "out of bounds" array access (see Heartbleed), Or the OS diff --git a/examples/schnorr.c b/examples/schnorr.c index 207c45c42226e..535b59a177132 100644 --- a/examples/schnorr.c +++ b/examples/schnorr.c @@ -26,7 +26,7 @@ int main(void) { unsigned char auxiliary_rand[32]; unsigned char serialized_pubkey[32]; unsigned char signature[64]; - int is_signature_valid; + int is_signature_valid, is_signature_valid2; int return_val; secp256k1_xonly_pubkey pubkey; secp256k1_keypair keypair; @@ -135,6 +135,15 @@ int main(void) { /* This will clear everything from the context and free the memory */ secp256k1_context_destroy(ctx); + /* Bonus example: if all we need is signature verification (and no key + generation or signing), we don't need to use a context created via + secp256k1_context_create(). We can simply use the static (i.e., global) + context secp256k1_context_static. See its description in + include/secp256k1.h for details. */ + is_signature_valid2 = secp256k1_schnorrsig_verify(secp256k1_context_static, + signature, msg_hash, 32, &pubkey); + assert(is_signature_valid2 == is_signature_valid); + /* It's best practice to try to clear secrets from memory after using them. * This is done because some bugs can allow an attacker to leak memory, for * example through "out of bounds" array access (see Heartbleed), Or the OS From 9a5a611a21fcdf7bf2dab30964cd0208d8cdf444 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 6 Feb 2023 21:34:50 +0100 Subject: [PATCH 3/4] build: Suppress stupid MSVC linker warning ... and use correct format to pass linker flags --- .cirrus.yml | 2 +- configure.ac | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index 48b09df585df1..bbfbf71654ae8 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -258,7 +258,7 @@ task: # Set non-essential options that affect the CLI messages here. # (They depend on the user's taste, so we don't want to set them automatically in configure.ac.) CFLAGS: -nologo -diagnostics:caret - LDFLAGS: -XCClinker -nologo -XCClinker -diagnostics:caret + LDFLAGS: -Xlinker -Xlinker -Xlinker -nologo matrix: - name: "x86_64 (MSVC): Windows (Debian stable, Wine)" - name: "x86_64 (MSVC): Windows (Debian stable, Wine, int128_struct)" diff --git a/configure.ac b/configure.ac index fc51db2bda939..ec0738d053d1c 100644 --- a/configure.ac +++ b/configure.ac @@ -115,6 +115,12 @@ AC_DEFUN([SECP_TRY_APPEND_DEFAULT_CFLAGS], [ if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then SECP_TRY_APPEND_CFLAGS([-W2 -wd4146], $1) # Moderate warning level, disable warning C4146 "unary minus operator applied to unsigned type, result still unsigned" SECP_TRY_APPEND_CFLAGS([-external:anglebrackets -external:W0], $1) # Suppress warnings from #include <...> files + # We pass -ignore:4217 to the MSVC linker to suppress warning 4217 when + # importing variables from a statically linked secp256k1. + # (See the libtool manual, section "Windows DLLs" for background.) + # Unfortunately, libtool tries to be too clever and strips "-Xlinker arg" + # into "arg", so this will be " -Xlinker -ignore:4217" after stripping. + LDFLAGS="-Xlinker -Xlinker -Xlinker -ignore:4217 $LDFLAGS" fi ]) SECP_TRY_APPEND_DEFAULT_CFLAGS(SECP_CFLAGS) From e4330341bd648e93b60fe70c631e311a98bce549 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Sat, 28 Jan 2023 09:22:24 +0100 Subject: [PATCH 4/4] ci: Shutdown wineserver whenever CI script exits Before: CI times out when a wine task fails. After: Wine tasks exit properly when they fail. --- ci/cirrus.sh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ci/cirrus.sh b/ci/cirrus.sh index e1ca35e3e32c0..8495c39203fdf 100755 --- a/ci/cirrus.sh +++ b/ci/cirrus.sh @@ -34,6 +34,8 @@ print_environment # This speeds up jobs with many invocations of wine (e.g., ./configure with MSVC) tremendously. case "$WRAPPER_CMD" in *wine*) + # Make sure to shutdown wineserver whenever we exit. + trap "wineserver -k || true" EXIT INT HUP # This is apparently only reliable when we run a dummy command such as "hh.exe" afterwards. wineserver -p && wine hh.exe ;; @@ -111,9 +113,6 @@ then make precomp fi -# Shutdown wineserver again -wineserver -k || true - # Check that no repo files have been modified by the build. # (This fails for example if the precomp files need to be updated in the repo.) git diff --exit-code