From b19806122e9065c6f434fc6160cd0c57fa3fea8c Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Dec 2022 14:56:43 +0100 Subject: [PATCH 1/7] tests: Use global copy of secp256k1_context_static instead of clone --- src/modules/extrakeys/tests_impl.h | 5 +++-- src/modules/recovery/tests_impl.h | 4 ++-- src/modules/schnorrsig/tests_impl.h | 6 +++--- src/tests.c | 20 +++++++++++--------- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/modules/extrakeys/tests_impl.h b/src/modules/extrakeys/tests_impl.h index 8030aedad6..dd535b9ad2 100644 --- a/src/modules/extrakeys/tests_impl.h +++ b/src/modules/extrakeys/tests_impl.h @@ -336,7 +336,6 @@ void test_keypair(void) { secp256k1_xonly_pubkey xonly_pk, xonly_pk_tmp; int pk_parity, pk_parity_tmp; int ecount; - secp256k1_context *sttc = secp256k1_context_clone(secp256k1_context_static); set_counting_callbacks(ctx, &ecount); set_counting_callbacks(sttc, &ecount); @@ -440,7 +439,9 @@ void test_keypair(void) { memset(&keypair, 0, sizeof(keypair)); CHECK(secp256k1_keypair_sec(ctx, sk_tmp, &keypair) == 1); CHECK(secp256k1_memcmp_var(zeros96, sk_tmp, sizeof(sk_tmp)) == 0); - secp256k1_context_destroy(sttc); + + secp256k1_context_set_error_callback(sttc, NULL, NULL); + secp256k1_context_set_illegal_callback(sttc, NULL, NULL); } void test_keypair_add(void) { diff --git a/src/modules/recovery/tests_impl.h b/src/modules/recovery/tests_impl.h index 0ff9294e38..0769b961ba 100644 --- a/src/modules/recovery/tests_impl.h +++ b/src/modules/recovery/tests_impl.h @@ -30,7 +30,6 @@ static int recovery_test_nonce_function(unsigned char *nonce32, const unsigned c void test_ecdsa_recovery_api(void) { /* Setup contexts that just count errors */ - secp256k1_context *sttc = secp256k1_context_clone(secp256k1_context_static); secp256k1_pubkey pubkey; secp256k1_pubkey recpubkey; secp256k1_ecdsa_signature normal_sig; @@ -124,7 +123,8 @@ void test_ecdsa_recovery_api(void) { CHECK(ecount == 7); /* cleanup */ - secp256k1_context_destroy(sttc); + secp256k1_context_set_error_callback(sttc, NULL, NULL); + secp256k1_context_set_illegal_callback(sttc, NULL, NULL); } void test_ecdsa_recovery_end_to_end(void) { diff --git a/src/modules/schnorrsig/tests_impl.h b/src/modules/schnorrsig/tests_impl.h index 06cc097cc1..f79d7aa0fa 100644 --- a/src/modules/schnorrsig/tests_impl.h +++ b/src/modules/schnorrsig/tests_impl.h @@ -128,8 +128,7 @@ void test_schnorrsig_api(void) { secp256k1_schnorrsig_extraparams invalid_extraparams = {{ 0 }, NULL, NULL}; /** setup **/ - secp256k1_context *sttc = secp256k1_context_clone(secp256k1_context_static); - int ecount; + int ecount = 0; secp256k1_context_set_error_callback(ctx, counting_illegal_callback_fn, &ecount); secp256k1_context_set_illegal_callback(ctx, counting_illegal_callback_fn, &ecount); @@ -198,7 +197,8 @@ void test_schnorrsig_api(void) { CHECK(secp256k1_schnorrsig_verify(ctx, sig, msg, sizeof(msg), &zero_pk) == 0); CHECK(ecount == 4); - secp256k1_context_destroy(sttc); + secp256k1_context_set_error_callback(sttc, NULL, NULL); + secp256k1_context_set_illegal_callback(sttc, NULL, NULL); } /* Checks that hash initialized by secp256k1_schnorrsig_sha256_tagged has the diff --git a/src/tests.c b/src/tests.c index 7aa15c9467..0a09c0d969 100644 --- a/src/tests.c +++ b/src/tests.c @@ -29,6 +29,7 @@ static int count = 64; static secp256k1_context *ctx = NULL; +static secp256k1_context *sttc = NULL; static void counting_illegal_callback_fn(const char* str, void* data) { /* Dummy callback function that just counts. */ @@ -180,9 +181,7 @@ void run_context_tests(int use_prealloc) { unsigned char ctmp[32]; int32_t ecount; int32_t ecount2; - secp256k1_context *sttc; void *ctx_prealloc = NULL; - void *sttc_prealloc = NULL; secp256k1_gej pubj; secp256k1_ge pub; @@ -196,11 +195,7 @@ void run_context_tests(int use_prealloc) { ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(ctx_prealloc != NULL); ctx = secp256k1_context_preallocated_create(ctx_prealloc, SECP256K1_CONTEXT_NONE); - sttc_prealloc = malloc(secp256k1_context_preallocated_clone_size(secp256k1_context_static)); - CHECK(sttc_prealloc != NULL); - sttc = secp256k1_context_preallocated_clone(secp256k1_context_static, sttc_prealloc); } else { - sttc = secp256k1_context_clone(secp256k1_context_static); ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); } @@ -312,12 +307,9 @@ void run_context_tests(int use_prealloc) { /* cleanup */ if (use_prealloc) { secp256k1_context_preallocated_destroy(ctx); - secp256k1_context_preallocated_destroy(sttc); free(ctx_prealloc); - free(sttc_prealloc); } else { secp256k1_context_destroy(ctx); - secp256k1_context_destroy(sttc); } /* Defined as no-op. */ secp256k1_context_destroy(NULL); @@ -7357,6 +7349,15 @@ int main(int argc, char **argv) { secp256k1_testrand_init(argc > 2 ? argv[2] : NULL); /* initialize */ + /* Make a writable copy of secp256k1_context_static in order to test the effect of API functions + that write to the context. The API does not support cloning the static context, so we use + memcpy instead. The user is not supposed to copy a context but we should still ensure that + the API functions handle copies of the static context gracefully. */ + sttc = malloc(sizeof(*secp256k1_context_static)); + CHECK(sttc != NULL); + memcpy(sttc, secp256k1_context_static, sizeof(secp256k1_context)); + CHECK(!secp256k1_context_is_proper(sttc)); + run_selftest_tests(); run_context_tests(0); run_context_tests(1); @@ -7463,6 +7464,7 @@ int main(int argc, char **argv) { secp256k1_testrand_finish(); /* shutdown */ + free(sttc); secp256k1_context_destroy(ctx); printf("no problems found\n"); From 18e0db30cb4a89989f040a5f212d54b306ffd96e Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 4 Jan 2023 16:52:36 +0100 Subject: [PATCH 2/7] tests: Don't recreate global context in scratch space test --- src/tests.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tests.c b/src/tests.c index 0a09c0d969..d1ad8acedb 100644 --- a/src/tests.c +++ b/src/tests.c @@ -325,12 +325,10 @@ void run_scratch_tests(void) { secp256k1_scratch_space *scratch; secp256k1_scratch_space local_scratch; - ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); - - /* Test public API */ secp256k1_context_set_illegal_callback(ctx, counting_illegal_callback_fn, &ecount); secp256k1_context_set_error_callback(ctx, counting_illegal_callback_fn, &ecount); + /* Test public API */ scratch = secp256k1_scratch_space_create(ctx, 1000); CHECK(scratch != NULL); CHECK(ecount == 0); @@ -397,7 +395,9 @@ void run_scratch_tests(void) { /* cleanup */ secp256k1_scratch_space_destroy(ctx, NULL); /* no-op */ - secp256k1_context_destroy(ctx); + + secp256k1_context_set_illegal_callback(ctx, NULL, NULL); + secp256k1_context_set_error_callback(ctx, NULL, NULL); } From ce4f936c4fa077d0473985479c61bd6544172aae Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Dec 2022 15:57:57 +0100 Subject: [PATCH 3/7] tests: Tidy run_context_tests() by extracting functions --- src/tests.c | 113 +++++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 54 deletions(-) diff --git a/src/tests.c b/src/tests.c index d1ad8acedb..dd0b00dc27 100644 --- a/src/tests.c +++ b/src/tests.c @@ -158,27 +158,77 @@ int context_eq(const secp256k1_context *a, const secp256k1_context *b) { && a->error_callback.data == b->error_callback.data; } -void test_deprecated_flags(void) { +void run_deprecated_context_flags_test(void) { + /* Check that a context created with any of the flags in the flags array is + * identical to the NONE context. */ unsigned int flags[] = { SECP256K1_CONTEXT_SIGN, SECP256K1_CONTEXT_VERIFY, SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY }; + secp256k1_context *none_ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); int i; - /* Check that a context created with any of the flags in the flags array is - * identical to the NONE context. */ for (i = 0; i < (int)(sizeof(flags)/sizeof(flags[0])); i++) { secp256k1_context *tmp_ctx; CHECK(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE) == secp256k1_context_preallocated_size(flags[i])); tmp_ctx = secp256k1_context_create(flags[i]); - CHECK(context_eq(ctx, tmp_ctx)); + CHECK(context_eq(none_ctx, tmp_ctx)); secp256k1_context_destroy(tmp_ctx); } + secp256k1_context_destroy(none_ctx); } -void run_context_tests(int use_prealloc) { +void run_ec_illegal_argument_tests(void) { + int ecount = 0; + int ecount2 = 10; secp256k1_pubkey pubkey; secp256k1_pubkey zero_pubkey; secp256k1_ecdsa_signature sig; unsigned char ctmp[32]; + + /* Setup */ + secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &ecount); + secp256k1_context_set_illegal_callback(ctx, counting_illegal_callback_fn, &ecount2); + memset(ctmp, 1, 32); + memset(&zero_pubkey, 0, sizeof(zero_pubkey)); + + /* Verify context-type checking illegal-argument errors. */ + CHECK(secp256k1_ec_pubkey_create(sttc, &pubkey, ctmp) == 0); + CHECK(ecount == 1); + VG_UNDEF(&pubkey, sizeof(pubkey)); + CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, ctmp) == 1); + VG_CHECK(&pubkey, sizeof(pubkey)); + CHECK(secp256k1_ecdsa_sign(sttc, &sig, ctmp, ctmp, NULL, NULL) == 0); + CHECK(ecount == 2); + VG_UNDEF(&sig, sizeof(sig)); + CHECK(secp256k1_ecdsa_sign(ctx, &sig, ctmp, ctmp, NULL, NULL) == 1); + VG_CHECK(&sig, sizeof(sig)); + CHECK(ecount2 == 10); + CHECK(secp256k1_ecdsa_verify(ctx, &sig, ctmp, &pubkey) == 1); + CHECK(ecount2 == 10); + CHECK(secp256k1_ecdsa_verify(sttc, &sig, ctmp, &pubkey) == 1); + CHECK(ecount == 2); + CHECK(secp256k1_ec_pubkey_tweak_add(ctx, &pubkey, ctmp) == 1); + CHECK(ecount2 == 10); + CHECK(secp256k1_ec_pubkey_tweak_add(sttc, &pubkey, ctmp) == 1); + CHECK(ecount == 2); + CHECK(secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, ctmp) == 1); + CHECK(ecount2 == 10); + CHECK(secp256k1_ec_pubkey_negate(sttc, &pubkey) == 1); + CHECK(ecount == 2); + CHECK(secp256k1_ec_pubkey_negate(ctx, &pubkey) == 1); + CHECK(ecount == 2); + CHECK(secp256k1_ec_pubkey_negate(sttc, &zero_pubkey) == 0); + CHECK(ecount == 3); + CHECK(secp256k1_ec_pubkey_negate(ctx, NULL) == 0); + CHECK(ecount2 == 11); + CHECK(secp256k1_ec_pubkey_tweak_mul(sttc, &pubkey, ctmp) == 1); + CHECK(ecount == 3); + + /* Clean up */ + secp256k1_context_set_illegal_callback(sttc, NULL, NULL); + secp256k1_context_set_illegal_callback(ctx, NULL, NULL); +} + +void run_context_tests(int use_prealloc) { int32_t ecount; int32_t ecount2; void *ctx_prealloc = NULL; @@ -199,10 +249,6 @@ void run_context_tests(int use_prealloc) { ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); } - test_deprecated_flags(); - - memset(&zero_pubkey, 0, sizeof(zero_pubkey)); - ecount = 0; ecount2 = 10; secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &ecount); @@ -249,50 +295,6 @@ void run_context_tests(int use_prealloc) { secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pubj, &key); secp256k1_ge_set_gej(&pub, &pubj); - /* Verify context-type checking illegal-argument errors. */ - memset(ctmp, 1, 32); - CHECK(secp256k1_ec_pubkey_create(sttc, &pubkey, ctmp) == 0); - CHECK(ecount == 1); - VG_UNDEF(&pubkey, sizeof(pubkey)); - CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, ctmp) == 1); - VG_CHECK(&pubkey, sizeof(pubkey)); - CHECK(secp256k1_ecdsa_sign(sttc, &sig, ctmp, ctmp, NULL, NULL) == 0); - CHECK(ecount == 2); - VG_UNDEF(&sig, sizeof(sig)); - CHECK(secp256k1_ecdsa_sign(ctx, &sig, ctmp, ctmp, NULL, NULL) == 1); - VG_CHECK(&sig, sizeof(sig)); - CHECK(ecount2 == 10); - CHECK(secp256k1_ecdsa_verify(ctx, &sig, ctmp, &pubkey) == 1); - CHECK(ecount2 == 10); - CHECK(secp256k1_ecdsa_verify(sttc, &sig, ctmp, &pubkey) == 1); - CHECK(ecount == 2); - CHECK(secp256k1_ec_pubkey_tweak_add(ctx, &pubkey, ctmp) == 1); - CHECK(ecount2 == 10); - CHECK(secp256k1_ec_pubkey_tweak_add(sttc, &pubkey, ctmp) == 1); - CHECK(ecount == 2); - CHECK(secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, ctmp) == 1); - CHECK(ecount2 == 10); - CHECK(secp256k1_ec_pubkey_negate(sttc, &pubkey) == 1); - CHECK(ecount == 2); - CHECK(secp256k1_ec_pubkey_negate(ctx, &pubkey) == 1); - CHECK(ecount == 2); - CHECK(secp256k1_ec_pubkey_negate(ctx, NULL) == 0); - CHECK(ecount2 == 11); - CHECK(secp256k1_ec_pubkey_negate(sttc, &zero_pubkey) == 0); - CHECK(ecount == 3); - CHECK(secp256k1_ec_pubkey_tweak_mul(sttc, &pubkey, ctmp) == 1); - CHECK(ecount == 3); - CHECK(secp256k1_context_randomize(sttc, ctmp) == 1); - CHECK(ecount == 3); - CHECK(secp256k1_context_randomize(sttc, NULL) == 1); - CHECK(ecount == 3); - CHECK(secp256k1_context_randomize(ctx, ctmp) == 1); - CHECK(ecount2 == 11); - CHECK(secp256k1_context_randomize(ctx, NULL) == 1); - CHECK(ecount2 == 11); - secp256k1_context_set_illegal_callback(sttc, NULL, NULL); - secp256k1_context_set_illegal_callback(ctx, NULL, NULL); - /* obtain a working nonce */ do { random_scalar_order_test(&nonce); @@ -7361,7 +7363,7 @@ int main(int argc, char **argv) { run_selftest_tests(); run_context_tests(0); run_context_tests(1); - run_scratch_tests(); + run_deprecated_context_flags_test(); ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); /* Randomize the context only with probability 15/16 @@ -7373,6 +7375,8 @@ int main(int argc, char **argv) { CHECK(secp256k1_context_randomize(ctx, rand32)); } + run_scratch_tests(); + run_rand_bits(); run_rand_int(); @@ -7435,6 +7439,7 @@ int main(int argc, char **argv) { #endif /* ecdsa tests */ + run_ec_illegal_argument_tests(); run_pubkey_comparison(); run_random_pubkeys(); run_ecdsa_der_parse(); From f32a36f620e979b13040ffd2cd55cfc6fac5bad0 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Dec 2022 15:37:17 +0100 Subject: [PATCH 4/7] tests: Don't use global context for context tests --- src/tests.c | 60 ++++++++++++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/tests.c b/src/tests.c index dd0b00dc27..157be5d5b3 100644 --- a/src/tests.c +++ b/src/tests.c @@ -231,7 +231,8 @@ void run_ec_illegal_argument_tests(void) { void run_context_tests(int use_prealloc) { int32_t ecount; int32_t ecount2; - void *ctx_prealloc = NULL; + secp256k1_context *my_ctx; + void *my_ctx_prealloc = NULL; secp256k1_gej pubj; secp256k1_ge pub; @@ -242,24 +243,24 @@ void run_context_tests(int use_prealloc) { CHECK(secp256k1_context_no_precomp == secp256k1_context_static); if (use_prealloc) { - ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); - CHECK(ctx_prealloc != NULL); - ctx = secp256k1_context_preallocated_create(ctx_prealloc, SECP256K1_CONTEXT_NONE); + my_ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); + CHECK(my_ctx_prealloc != NULL); + my_ctx = secp256k1_context_preallocated_create(my_ctx_prealloc, SECP256K1_CONTEXT_NONE); } else { - ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); + my_ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); } ecount = 0; ecount2 = 10; secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &ecount); - secp256k1_context_set_illegal_callback(ctx, counting_illegal_callback_fn, &ecount2); + secp256k1_context_set_illegal_callback(my_ctx, counting_illegal_callback_fn, &ecount2); /* set error callback (to a function that still aborts in case malloc() fails in secp256k1_context_clone() below) */ - secp256k1_context_set_error_callback(ctx, secp256k1_default_illegal_callback_fn, NULL); - CHECK(ctx->error_callback.fn != sttc->error_callback.fn); - CHECK(ctx->error_callback.fn == secp256k1_default_illegal_callback_fn); + secp256k1_context_set_error_callback(my_ctx, secp256k1_default_illegal_callback_fn, NULL); + CHECK(my_ctx->error_callback.fn != sttc->error_callback.fn); + CHECK(my_ctx->error_callback.fn == secp256k1_default_illegal_callback_fn); /* check if sizes for cloning are consistent */ - CHECK(secp256k1_context_preallocated_clone_size(ctx) == secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); + CHECK(secp256k1_context_preallocated_clone_size(my_ctx) == secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(secp256k1_context_preallocated_clone_size(sttc) >= sizeof(secp256k1_context)); /*** clone and destroy all of them to make sure cloning was complete ***/ @@ -268,50 +269,50 @@ void run_context_tests(int use_prealloc) { if (use_prealloc) { /* clone into a non-preallocated context and then again into a new preallocated one. */ - ctx_tmp = ctx; ctx = secp256k1_context_clone(ctx); secp256k1_context_preallocated_destroy(ctx_tmp); - free(ctx_prealloc); ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(ctx_prealloc != NULL); - ctx_tmp = ctx; ctx = secp256k1_context_preallocated_clone(ctx, ctx_prealloc); secp256k1_context_destroy(ctx_tmp); + ctx_tmp = my_ctx; my_ctx = secp256k1_context_clone(my_ctx); secp256k1_context_preallocated_destroy(ctx_tmp); + free(my_ctx_prealloc); my_ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(my_ctx_prealloc != NULL); + ctx_tmp = my_ctx; my_ctx = secp256k1_context_preallocated_clone(my_ctx, my_ctx_prealloc); secp256k1_context_destroy(ctx_tmp); } else { /* clone into a preallocated context and then again into a new non-preallocated one. */ void *prealloc_tmp; prealloc_tmp = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(prealloc_tmp != NULL); - ctx_tmp = ctx; ctx = secp256k1_context_preallocated_clone(ctx, prealloc_tmp); secp256k1_context_destroy(ctx_tmp); - ctx_tmp = ctx; ctx = secp256k1_context_clone(ctx); secp256k1_context_preallocated_destroy(ctx_tmp); + ctx_tmp = my_ctx; my_ctx = secp256k1_context_preallocated_clone(my_ctx, prealloc_tmp); secp256k1_context_destroy(ctx_tmp); + ctx_tmp = my_ctx; my_ctx = secp256k1_context_clone(my_ctx); secp256k1_context_preallocated_destroy(ctx_tmp); free(prealloc_tmp); } } /* Verify that the error callback makes it across the clone. */ - CHECK(ctx->error_callback.fn != sttc->error_callback.fn); - CHECK(ctx->error_callback.fn == secp256k1_default_illegal_callback_fn); + CHECK(my_ctx->error_callback.fn != sttc->error_callback.fn); + CHECK(my_ctx->error_callback.fn == secp256k1_default_illegal_callback_fn); /* And that it resets back to default. */ - secp256k1_context_set_error_callback(ctx, NULL, NULL); - CHECK(ctx->error_callback.fn == sttc->error_callback.fn); + secp256k1_context_set_error_callback(my_ctx, NULL, NULL); + CHECK(my_ctx->error_callback.fn == sttc->error_callback.fn); /*** attempt to use them ***/ random_scalar_order_test(&msg); random_scalar_order_test(&key); - secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pubj, &key); + secp256k1_ecmult_gen(&my_ctx->ecmult_gen_ctx, &pubj, &key); secp256k1_ge_set_gej(&pub, &pubj); /* obtain a working nonce */ do { random_scalar_order_test(&nonce); - } while(!secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &sigr, &sigs, &key, &msg, &nonce, NULL)); + } while(!secp256k1_ecdsa_sig_sign(&my_ctx->ecmult_gen_ctx, &sigr, &sigs, &key, &msg, &nonce, NULL)); /* try signing */ - CHECK(secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &sigr, &sigs, &key, &msg, &nonce, NULL)); + CHECK(secp256k1_ecdsa_sig_sign(&my_ctx->ecmult_gen_ctx, &sigr, &sigs, &key, &msg, &nonce, NULL)); /* try verifying */ CHECK(secp256k1_ecdsa_sig_verify(&sigr, &sigs, &pub, &msg)); /* cleanup */ if (use_prealloc) { - secp256k1_context_preallocated_destroy(ctx); - free(ctx_prealloc); + secp256k1_context_preallocated_destroy(my_ctx); + free(my_ctx_prealloc); } else { - secp256k1_context_destroy(ctx); + secp256k1_context_destroy(my_ctx); } /* Defined as no-op. */ secp256k1_context_destroy(NULL); @@ -7360,11 +7361,6 @@ int main(int argc, char **argv) { memcpy(sttc, secp256k1_context_static, sizeof(secp256k1_context)); CHECK(!secp256k1_context_is_proper(sttc)); - run_selftest_tests(); - run_context_tests(0); - run_context_tests(1); - run_deprecated_context_flags_test(); - ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); /* Randomize the context only with probability 15/16 to make sure we test without context randomization from time to time. @@ -7375,6 +7371,10 @@ int main(int argc, char **argv) { CHECK(secp256k1_context_randomize(ctx, rand32)); } + run_selftest_tests(); + run_context_tests(0); + run_context_tests(1); + run_deprecated_context_flags_test(); run_scratch_tests(); run_rand_bits(); From fc90bb569564d552ec0b5706fde6e94bb5313f4e Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 4 Jan 2023 17:43:45 +0100 Subject: [PATCH 5/7] refactor: Tidy up main() --- src/tests.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/tests.c b/src/tests.c index 157be5d5b3..77280e3108 100644 --- a/src/tests.c +++ b/src/tests.c @@ -7351,16 +7351,9 @@ int main(int argc, char **argv) { /* find random seed */ secp256k1_testrand_init(argc > 2 ? argv[2] : NULL); - /* initialize */ - /* Make a writable copy of secp256k1_context_static in order to test the effect of API functions - that write to the context. The API does not support cloning the static context, so we use - memcpy instead. The user is not supposed to copy a context but we should still ensure that - the API functions handle copies of the static context gracefully. */ - sttc = malloc(sizeof(*secp256k1_context_static)); - CHECK(sttc != NULL); - memcpy(sttc, secp256k1_context_static, sizeof(secp256k1_context)); - CHECK(!secp256k1_context_is_proper(sttc)); + /*** Setup test environment ***/ + /* Create a global context available to all tests */ ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); /* Randomize the context only with probability 15/16 to make sure we test without context randomization from time to time. @@ -7370,16 +7363,33 @@ int main(int argc, char **argv) { secp256k1_testrand256(rand32); CHECK(secp256k1_context_randomize(ctx, rand32)); } + /* Make a writable copy of secp256k1_context_static in order to test the effect of API functions + that write to the context. The API does not support cloning the static context, so we use + memcpy instead. The user is not supposed to copy a context but we should still ensure that + the API functions handle copies of the static context gracefully. */ + sttc = malloc(sizeof(*secp256k1_context_static)); + CHECK(sttc != NULL); + memcpy(sttc, secp256k1_context_static, sizeof(secp256k1_context)); + CHECK(!secp256k1_context_is_proper(sttc)); + + /*** Run actual tests ***/ + /* selftest tests */ run_selftest_tests(); + + /* context tests */ run_context_tests(0); run_context_tests(1); run_deprecated_context_flags_test(); + + /* scratch tests */ run_scratch_tests(); + /* randomness tests */ run_rand_bits(); run_rand_int(); + /* integer arithmetic tests */ #ifdef SECP256K1_WIDEMUL_INT128 run_int128_tests(); #endif @@ -7387,6 +7397,7 @@ int main(int argc, char **argv) { run_modinv_tests(); run_inverse_tests(); + /* hash tests */ run_sha256_known_output_tests(); run_sha256_counter_tests(); run_hmac_sha256_tests(); @@ -7466,12 +7477,12 @@ int main(int argc, char **argv) { run_cmov_tests(); - secp256k1_testrand_finish(); - - /* shutdown */ + /*** Tear down test environment ***/ free(sttc); secp256k1_context_destroy(ctx); + secp256k1_testrand_finish(); + printf("no problems found\n"); return 0; } From a4a09379b1a6f65d5a1801cffae0992b49660d82 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 4 Jan 2023 18:03:04 +0100 Subject: [PATCH 6/7] tests: Clean up and improve run_context_tests() further --- src/tests.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/tests.c b/src/tests.c index 77280e3108..eda462499e 100644 --- a/src/tests.c +++ b/src/tests.c @@ -229,8 +229,7 @@ void run_ec_illegal_argument_tests(void) { } void run_context_tests(int use_prealloc) { - int32_t ecount; - int32_t ecount2; + int32_t dummy = 0; secp256k1_context *my_ctx; void *my_ctx_prealloc = NULL; @@ -250,13 +249,9 @@ void run_context_tests(int use_prealloc) { my_ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); } - ecount = 0; - ecount2 = 10; - secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &ecount); - secp256k1_context_set_illegal_callback(my_ctx, counting_illegal_callback_fn, &ecount2); /* set error callback (to a function that still aborts in case malloc() fails in secp256k1_context_clone() below) */ secp256k1_context_set_error_callback(my_ctx, secp256k1_default_illegal_callback_fn, NULL); - CHECK(my_ctx->error_callback.fn != sttc->error_callback.fn); + CHECK(my_ctx->error_callback.fn != secp256k1_default_error_callback_fn); CHECK(my_ctx->error_callback.fn == secp256k1_default_illegal_callback_fn); /* check if sizes for cloning are consistent */ @@ -284,11 +279,22 @@ void run_context_tests(int use_prealloc) { } /* Verify that the error callback makes it across the clone. */ - CHECK(my_ctx->error_callback.fn != sttc->error_callback.fn); + CHECK(my_ctx->error_callback.fn != secp256k1_default_error_callback_fn); CHECK(my_ctx->error_callback.fn == secp256k1_default_illegal_callback_fn); /* And that it resets back to default. */ secp256k1_context_set_error_callback(my_ctx, NULL, NULL); - CHECK(my_ctx->error_callback.fn == sttc->error_callback.fn); + CHECK(my_ctx->error_callback.fn == secp256k1_default_error_callback_fn); + + /* Verify that setting and resetting illegal callback works */ + secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &dummy); + CHECK(sttc->illegal_callback.fn == counting_illegal_callback_fn); + secp256k1_context_set_illegal_callback(sttc, NULL, NULL); + CHECK(sttc->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + + secp256k1_context_set_illegal_callback(my_ctx, counting_illegal_callback_fn, &dummy); + CHECK(my_ctx->illegal_callback.fn == counting_illegal_callback_fn); + secp256k1_context_set_illegal_callback(my_ctx, NULL, NULL); + CHECK(my_ctx->illegal_callback.fn == secp256k1_default_illegal_callback_fn); /*** attempt to use them ***/ random_scalar_order_test(&msg); From 39e8f0e3d7ba7924e9cc5f9e0c56747e942f1eab Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 4 Jan 2023 18:17:14 +0100 Subject: [PATCH 7/7] refactor: Separate run_context_tests into static vs proper contexts --- src/tests.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/tests.c b/src/tests.c index eda462499e..89246cfd58 100644 --- a/src/tests.c +++ b/src/tests.c @@ -228,7 +228,23 @@ void run_ec_illegal_argument_tests(void) { secp256k1_context_set_illegal_callback(ctx, NULL, NULL); } -void run_context_tests(int use_prealloc) { +void run_static_context_tests(void) { + int32_t dummy = 0; + + /* Check that deprecated secp256k1_context_no_precomp is an alias to secp256k1_context_static. */ + CHECK(secp256k1_context_no_precomp == secp256k1_context_static); + + /* check if sizes for cloning are consistent */ + CHECK(secp256k1_context_preallocated_clone_size(sttc) >= sizeof(secp256k1_context)); + + /* Verify that setting and resetting illegal callback works */ + secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &dummy); + CHECK(sttc->illegal_callback.fn == counting_illegal_callback_fn); + secp256k1_context_set_illegal_callback(sttc, NULL, NULL); + CHECK(sttc->illegal_callback.fn == secp256k1_default_illegal_callback_fn); +} + +void run_proper_context_tests(int use_prealloc) { int32_t dummy = 0; secp256k1_context *my_ctx; void *my_ctx_prealloc = NULL; @@ -237,10 +253,6 @@ void run_context_tests(int use_prealloc) { secp256k1_ge pub; secp256k1_scalar msg, key, nonce; secp256k1_scalar sigr, sigs; - - /* Check that deprecated secp256k1_context_no_precomp is an alias to secp256k1_context_static. */ - CHECK(secp256k1_context_no_precomp == secp256k1_context_static); - if (use_prealloc) { my_ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(my_ctx_prealloc != NULL); @@ -256,7 +268,6 @@ void run_context_tests(int use_prealloc) { /* check if sizes for cloning are consistent */ CHECK(secp256k1_context_preallocated_clone_size(my_ctx) == secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); - CHECK(secp256k1_context_preallocated_clone_size(sttc) >= sizeof(secp256k1_context)); /*** clone and destroy all of them to make sure cloning was complete ***/ { @@ -286,11 +297,6 @@ void run_context_tests(int use_prealloc) { CHECK(my_ctx->error_callback.fn == secp256k1_default_error_callback_fn); /* Verify that setting and resetting illegal callback works */ - secp256k1_context_set_illegal_callback(sttc, counting_illegal_callback_fn, &dummy); - CHECK(sttc->illegal_callback.fn == counting_illegal_callback_fn); - secp256k1_context_set_illegal_callback(sttc, NULL, NULL); - CHECK(sttc->illegal_callback.fn == secp256k1_default_illegal_callback_fn); - secp256k1_context_set_illegal_callback(my_ctx, counting_illegal_callback_fn, &dummy); CHECK(my_ctx->illegal_callback.fn == counting_illegal_callback_fn); secp256k1_context_set_illegal_callback(my_ctx, NULL, NULL); @@ -7384,8 +7390,9 @@ int main(int argc, char **argv) { run_selftest_tests(); /* context tests */ - run_context_tests(0); - run_context_tests(1); + run_proper_context_tests(0); + run_proper_context_tests(1); + run_static_context_tests(); run_deprecated_context_flags_test(); /* scratch tests */