diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a13a39991f57..7755f61e7b5dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +#### Changed + - Forbade cloning or destroying `secp256k1_context_static`. Create a new context instead of cloning the static context. (If this change breaks your code, your code is probably wrong.) + ## [0.2.0] - 2022-12-12 #### Added diff --git a/include/secp256k1.h b/include/secp256k1.h index 3d169ecce20c2..a228ef638c423 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -291,8 +291,11 @@ SECP256K1_API secp256k1_context* secp256k1_context_create( * called at most once for every call of this function. If you need to avoid dynamic * memory allocation entirely, see the functions in secp256k1_preallocated.h. * + * Cloning secp256k1_context_static is not possible, and should not be emulated by + * the caller (e.g., using memcpy). Create a new context instead. + * * Returns: a newly created context object. - * Args: ctx: an existing context to copy + * Args: ctx: an existing context to copy (not secp256k1_context_static) */ SECP256K1_API secp256k1_context* secp256k1_context_clone( const secp256k1_context* ctx @@ -310,6 +313,7 @@ SECP256K1_API secp256k1_context* secp256k1_context_clone( * * Args: ctx: an existing context to destroy, constructed using * secp256k1_context_create or secp256k1_context_clone + * (i.e., not secp256k1_context_static). */ SECP256K1_API void secp256k1_context_destroy( secp256k1_context* ctx diff --git a/include/secp256k1_preallocated.h b/include/secp256k1_preallocated.h index ed846f75f9de6..ffa96dd339f7b 100644 --- a/include/secp256k1_preallocated.h +++ b/include/secp256k1_preallocated.h @@ -88,8 +88,11 @@ SECP256K1_API size_t secp256k1_context_preallocated_clone_size( * the lifetime of this context object, see the description of * secp256k1_context_preallocated_create for details. * + * Cloning secp256k1_context_static is not possible, and should not be emulated by + * the caller (e.g., using memcpy). Create a new context instead. + * * Returns: a newly created context object. - * Args: ctx: an existing context to copy. + * Args: ctx: an existing context to copy (not secp256k1_context_static). * In: prealloc: a pointer to a rewritable contiguous block of memory of * size at least secp256k1_context_preallocated_size(flags) * bytes, as detailed above. @@ -117,7 +120,8 @@ SECP256K1_API secp256k1_context* secp256k1_context_preallocated_clone( * * Args: ctx: an existing context to destroy, constructed using * secp256k1_context_preallocated_create or - * secp256k1_context_preallocated_clone. + * secp256k1_context_preallocated_clone + * (i.e., not secp256k1_context_static). */ SECP256K1_API void secp256k1_context_preallocated_destroy( secp256k1_context* ctx diff --git a/src/secp256k1.c b/src/secp256k1.c index 6c91a76119946..3e9e7ccc94e68 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -109,9 +109,9 @@ size_t secp256k1_context_preallocated_size(unsigned int flags) { } size_t secp256k1_context_preallocated_clone_size(const secp256k1_context* ctx) { - size_t ret = sizeof(secp256k1_context); VERIFY_CHECK(ctx != NULL); - return ret; + ARG_CHECK(secp256k1_context_is_proper(ctx)); + return sizeof(secp256k1_context); } secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigned int flags) { @@ -152,6 +152,7 @@ secp256k1_context* secp256k1_context_preallocated_clone(const secp256k1_context* secp256k1_context* ret; VERIFY_CHECK(ctx != NULL); ARG_CHECK(prealloc != NULL); + ARG_CHECK(secp256k1_context_is_proper(ctx)); ret = (secp256k1_context*)prealloc; *ret = *ctx; @@ -163,6 +164,8 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) { size_t prealloc_size; VERIFY_CHECK(ctx != NULL); + ARG_CHECK(secp256k1_context_is_proper(ctx)); + prealloc_size = secp256k1_context_preallocated_clone_size(ctx); ret = (secp256k1_context*)checked_malloc(&ctx->error_callback, prealloc_size); ret = secp256k1_context_preallocated_clone(ctx, ret); @@ -170,17 +173,26 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) { } void secp256k1_context_preallocated_destroy(secp256k1_context* ctx) { - ARG_CHECK_VOID(ctx != secp256k1_context_static); - if (ctx != NULL) { - secp256k1_ecmult_gen_context_clear(&ctx->ecmult_gen_ctx); + ARG_CHECK_VOID(ctx == NULL || secp256k1_context_is_proper(ctx)); + + /* Defined as noop */ + if (ctx == NULL) { + return; } + + secp256k1_ecmult_gen_context_clear(&ctx->ecmult_gen_ctx); } void secp256k1_context_destroy(secp256k1_context* ctx) { - if (ctx != NULL) { - secp256k1_context_preallocated_destroy(ctx); - free(ctx); + ARG_CHECK_VOID(ctx == NULL || secp256k1_context_is_proper(ctx)); + + /* Defined as noop */ + if (ctx == NULL) { + return; } + + secp256k1_context_preallocated_destroy(ctx); + free(ctx); } void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)(const char* message, void* data), const void* data) { diff --git a/src/tests.c b/src/tests.c index 9cc4d1763a5f8..c33d1dc42cc6a 100644 --- a/src/tests.c +++ b/src/tests.c @@ -32,6 +32,18 @@ static int COUNT = 64; static secp256k1_context *CTX = NULL; static secp256k1_context *STATIC_CTX = NULL; +static int all_bytes_equal(const void* s, unsigned char value, size_t n) { + const unsigned char *p = s; + size_t i; + + for (i = 0; i < n; i++) { + if (p[i] != value) { + return 0; + } + } + return 1; +} + static void counting_illegal_callback_fn(const char* str, void* data) { /* Dummy callback function that just counts. */ int32_t *p; @@ -229,20 +241,47 @@ static void run_ec_illegal_argument_tests(void) { secp256k1_context_set_illegal_callback(CTX, NULL, NULL); } -static void run_static_context_tests(void) { - int32_t dummy = 0; - +static void run_static_context_tests(int use_prealloc) { /* 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(STATIC_CTX) >= sizeof(secp256k1_context)); + { + int ecount = 0; + secp256k1_context_set_illegal_callback(STATIC_CTX, counting_illegal_callback_fn, &ecount); + /* Destroying or cloning secp256k1_context_static is not supported. */ + if (use_prealloc) { + CHECK(secp256k1_context_preallocated_clone_size(STATIC_CTX) == 0); + CHECK(ecount == 1); + { + secp256k1_context *my_static_ctx = malloc(sizeof(*STATIC_CTX)); + CHECK(my_static_ctx != NULL); + memset(my_static_ctx, 0x2a, sizeof(*my_static_ctx)); + CHECK(secp256k1_context_preallocated_clone(STATIC_CTX, my_static_ctx) == NULL); + CHECK(all_bytes_equal(my_static_ctx, 0x2a, sizeof(*my_static_ctx))); + CHECK(ecount == 2); + free(my_static_ctx); + } + secp256k1_context_preallocated_destroy(STATIC_CTX); + CHECK(ecount == 3); + } else { + CHECK(secp256k1_context_clone(STATIC_CTX) == NULL); + CHECK(ecount == 1); + secp256k1_context_destroy(STATIC_CTX); + CHECK(ecount == 2); + } + secp256k1_context_set_illegal_callback(STATIC_CTX, NULL, NULL); + } - /* Verify that setting and resetting illegal callback works */ - secp256k1_context_set_illegal_callback(STATIC_CTX, counting_illegal_callback_fn, &dummy); - CHECK(STATIC_CTX->illegal_callback.fn == counting_illegal_callback_fn); - secp256k1_context_set_illegal_callback(STATIC_CTX, NULL, NULL); - CHECK(STATIC_CTX->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + { + /* Verify that setting and resetting illegal callback works */ + int32_t dummy = 0; + secp256k1_context_set_illegal_callback(STATIC_CTX, counting_illegal_callback_fn, &dummy); + CHECK(STATIC_CTX->illegal_callback.fn == counting_illegal_callback_fn); + CHECK(STATIC_CTX->illegal_callback.data == &dummy); + secp256k1_context_set_illegal_callback(STATIC_CTX, NULL, NULL); + CHECK(STATIC_CTX->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + CHECK(STATIC_CTX->illegal_callback.data == NULL); + } } static void run_proper_context_tests(int use_prealloc) { @@ -300,8 +339,10 @@ static void run_proper_context_tests(int use_prealloc) { /* Verify that setting and resetting illegal callback works */ secp256k1_context_set_illegal_callback(my_ctx, counting_illegal_callback_fn, &dummy); CHECK(my_ctx->illegal_callback.fn == counting_illegal_callback_fn); + CHECK(my_ctx->illegal_callback.data == &dummy); secp256k1_context_set_illegal_callback(my_ctx, NULL, NULL); CHECK(my_ctx->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + CHECK(my_ctx->illegal_callback.data == NULL); /*** attempt to use them ***/ random_scalar_order_test(&msg); @@ -327,6 +368,7 @@ static void run_proper_context_tests(int use_prealloc) { } else { secp256k1_context_destroy(my_ctx); } + /* Defined as no-op. */ secp256k1_context_destroy(NULL); secp256k1_context_preallocated_destroy(NULL); @@ -7389,9 +7431,8 @@ int main(int argc, char **argv) { run_selftest_tests(); /* context tests */ - run_proper_context_tests(0); - run_proper_context_tests(1); - run_static_context_tests(); + run_proper_context_tests(0); run_proper_context_tests(1); + run_static_context_tests(0); run_static_context_tests(1); run_deprecated_context_flags_test(); /* scratch tests */