contexts: Forbid cloning/destroying secp256k1_context_static

sipa · Jan 18, 2023 · 4b6df5e · 4b6df5e
1 parent b1579cf
commit 4b6df5e
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,24 +164,35 @@ 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);
     return ret;
 }
 
 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
@@ -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 */