From cf66f2357c6ad8c5fe219577ad56e6f51301ca5a Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Mon, 5 Dec 2022 14:35:54 +0100 Subject: [PATCH 1/4] refactor: Add helper function secp256k1_context_is_proper() --- src/secp256k1.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/secp256k1.c b/src/secp256k1.c index 5ed38241610ae..d9626865cb0f3 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -75,6 +75,15 @@ static const secp256k1_context secp256k1_context_static_ = { const secp256k1_context *secp256k1_context_static = &secp256k1_context_static_; const secp256k1_context *secp256k1_context_no_precomp = &secp256k1_context_static_; +/* Helper function that determines if a context is proper, i.e., is not the static context or a copy thereof. + * + * This is intended for "context" functions such as secp256k1_context_clone. Function which need specific + * features of a context should still check for these features directly. For example, a function that needs + * ecmult_gen should directly check for the existence of the ecmult_gen context. */ +static int secp256k1_context_is_proper(const secp256k1_context* ctx) { + return secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx); +} + void secp256k1_selftest(void) { if (!secp256k1_selftest_passes()) { secp256k1_callback_call(&default_error_callback, "self test failed"); @@ -171,6 +180,9 @@ void secp256k1_context_destroy(secp256k1_context* ctx) { } void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)(const char* message, void* data), const void* data) { + /* We compare pointers instead of checking secp256k1_context_is_proper() here + because setting callbacks is allowed on *copies* of the static context: + it's harmless and makes testing easier. */ ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static); if (fun == NULL) { fun = secp256k1_default_illegal_callback_fn; @@ -180,6 +192,9 @@ void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)( } void secp256k1_context_set_error_callback(secp256k1_context* ctx, void (*fun)(const char* message, void* data), const void* data) { + /* We compare pointers instead of checking secp256k1_context_is_proper() here + because setting callbacks is allowed on *copies* of the static context: + it's harmless and makes testing easier. */ ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static); if (fun == NULL) { fun = secp256k1_default_error_callback_fn; From c635c1bfd54417487745bbbf518114a962a47bcc Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Dec 2022 12:59:45 +0100 Subject: [PATCH 2/4] Change ARG_CHECK_NO_RETURN to ARG_CHECK_VOID which returns (void) --- src/secp256k1.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/secp256k1.c b/src/secp256k1.c index d9626865cb0f3..67c8019d48182 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -51,9 +51,10 @@ } \ } while(0) -#define ARG_CHECK_NO_RETURN(cond) do { \ +#define ARG_CHECK_VOID(cond) do { \ if (EXPECT(!(cond), 0)) { \ secp256k1_callback_call(&ctx->illegal_callback, #cond); \ + return; \ } \ } while(0) @@ -166,7 +167,7 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) { } void secp256k1_context_preallocated_destroy(secp256k1_context* ctx) { - ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static); + ARG_CHECK_VOID(ctx != secp256k1_context_static); if (ctx != NULL) { secp256k1_ecmult_gen_context_clear(&ctx->ecmult_gen_ctx); } @@ -183,7 +184,7 @@ void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)( /* We compare pointers instead of checking secp256k1_context_is_proper() here because setting callbacks is allowed on *copies* of the static context: it's harmless and makes testing easier. */ - ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static); + ARG_CHECK_VOID(ctx != secp256k1_context_static); if (fun == NULL) { fun = secp256k1_default_illegal_callback_fn; } @@ -195,7 +196,7 @@ void secp256k1_context_set_error_callback(secp256k1_context* ctx, void (*fun)(co /* We compare pointers instead of checking secp256k1_context_is_proper() here because setting callbacks is allowed on *copies* of the static context: it's harmless and makes testing easier. */ - ARG_CHECK_NO_RETURN(ctx != secp256k1_context_static); + ARG_CHECK_VOID(ctx != secp256k1_context_static); if (fun == NULL) { fun = secp256k1_default_error_callback_fn; } From 2551cdac903937c112357d4eb43bc194072a6cc2 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Dec 2022 16:26:36 +0100 Subject: [PATCH 3/4] tests: Fix code formatting --- src/tests.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tests.c b/src/tests.c index 53613f420a79b..0671e0df89d2a 100644 --- a/src/tests.c +++ b/src/tests.c @@ -156,8 +156,7 @@ int context_eq(const secp256k1_context *a, const secp256k1_context *b) { return a->declassify == b->declassify && ecmult_gen_context_eq(&a->ecmult_gen_ctx, &b->ecmult_gen_ctx) && a->illegal_callback.fn == b->illegal_callback.fn - && a->illegal_callback.data == b->illegal_callback. -data + && a->illegal_callback.data == b->illegal_callback.data && a->error_callback.fn == b->error_callback.fn && a->error_callback.data == b->error_callback.data; } From a49e0940ad671f96533d5a79f2ca1fa4020abc0a Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 8 Dec 2022 16:31:00 +0100 Subject: [PATCH 4/4] docs: Fix typo --- include/secp256k1.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/secp256k1.h b/include/secp256k1.h index 826ab75850c5e..3d169ecce20c2 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -849,7 +849,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_tweak_mul( * kind of elliptic curve point multiplication and thus does not benefit from * enhanced protection against side-channel leakage currently. * - * It is safe call this function on a copy of secp256k1_context_static in writable + * It is safe to call this function on a copy of secp256k1_context_static in writable * memory (e.g., obtained via secp256k1_context_clone). In that case, this * function is guaranteed to return 1, but the call will have no effect because * the static context (or a copy thereof) is not meant to be randomized.