Allow overriding secp context allocation

This can be used to make the library thread-safe, for example by using
thread local storage to hold the context to be used by each thread.

CHANGES.md

@@ -1,8 +1,16 @@
 # Changes
 
+## Version 0.8.2
+
+- struct wally_operations has changed to hold the size of the struct
+  and has an additional member to allow overriding the lib secp context
+  used internally. Users must recompile their applications against this
+  version as a result (re-linking or simply upgrading the shared library
+  is insufficient).
+
 ## Version 0.8.1
 
-- Build: Note that the secp256k1-zkp library is now as git submodule rather
+- Build: Note that the secp256k1-zkp library is now a git submodule rather
   than being directly checked in to the source tree. Run
   `git submodule sync --recursive` then `git submodule update --init --recursive`
   from the source tree in order to clone the secp source and build the library.

include/wally.hpp

@@ -1628,6 +1628,14 @@ inline struct secp256k1_context_struct *get_secp_context() {
     return ::wally_get_secp_context();
 }
 
+inline struct secp256k1_context_struct *get_new_secp_context() {
+    return ::wally_get_new_secp_context();
+}
+
+inline void secp_context_free(struct secp256k1_context_struct *ctx) {
+    ::wally_secp_context_free(ctx);
+}
+
 inline int clear(void *p, size_t n) {
     return ::wally_bzero(p, n);
 }

include/wally_core.h

@@ -31,8 +31,7 @@ extern "C" {
 /**
  * Initialize wally.
  *
- * As wally is not currently threadsafe, this function should be called once
- * before threads are created by the application.
+ * This function must be called once before threads are created by the application.
  *
  * :param flags: Flags controlling what to initialize. Currently must be zero.
  */
@@ -49,9 +48,27 @@ WALLY_CORE_API int wally_cleanup(uint32_t flags);
 /**
  * Fetch the wally internal secp256k1 context object.
  *
- * The context is created on demand.
+ * By default, a single global context is created on demand. This behaviour
+ * can be overriden by providing a custom context fetching function when
+ * calling `wally_set_operations`.
  */
-struct secp256k1_context_struct *wally_get_secp_context(void);
+WALLY_CORE_API struct secp256k1_context_struct *wally_get_secp_context(void);
+
+/**
+ * Create a new wally-suitable secp256k1 context object.
+ *
+ * The created context is initialised to be usable by all wally functions.
+ */
+WALLY_CORE_API struct secp256k1_context_struct *wally_get_new_secp_context(void);
+
+/**
+ * Free a secp256k1 context object created by `wally_get_new_secp_context`.
+ *
+ * This function must only be called on context objects returned from
+ * `wally_get_new_secp_context`, it should not be called on the default
+ * context returned from `wally_get_secp_context`.
+ */
+WALLY_CORE_API void wally_secp_context_free(struct secp256k1_context_struct *ctx);
 #endif
 
 /**
@@ -79,13 +96,17 @@ WALLY_CORE_API int wally_free_string(
  * Provide entropy to randomize the libraries internal libsecp256k1 context.
  *
  * Random data is used in libsecp256k1 to blind the data being processed,
- * making side channel attacks more difficult. Wally uses a single
+ * making side channel attacks more difficult. By default, Wally uses a single
  * internal context for secp functions that is not initially randomized.
+ *
  * The caller should call this function before using any functions that rely on
- * libsecp256k1 (i.e. Anything using public/private keys).
+ * libsecp256k1 (i.e. Anything using public/private keys). If the caller
+ * has overriden the library's default libsecp context fetching using
+ * `wally_set_operations`, then it may be necessary to call this function
+ * before calling wally functions in each thread created by the caller.
  *
- * As wally is not currently threadsafe, this function should either be
- * called before threads are created or access to wally functions wrapped
+ * If wally is used in its default configuration, this function should either
+ * be called before threads are created or access to wally functions wrapped
  * in an application level mutex.
  *
  * :param bytes: Entropy to use.
@@ -262,12 +283,18 @@ typedef int (*wally_ec_nonce_t)(
     unsigned int attempt
     );
 
+/** The type of an overridable function to return a secp context */
+typedef struct secp256k1_context_struct *(*secp_context_t)(
+    void);
+
 /** Structure holding function pointers for overridable wally operations */
 struct wally_operations {
+    uintptr_t struct_size; /* Must be initialised to sizeof(wally_operations) */
     wally_malloc_t malloc_fn;
     wally_free_t free_fn;
     wally_bzero_t bzero_fn;
     wally_ec_nonce_t ec_nonce_fn;
+    secp_context_t secp_context_fn;
 };
 
 /**
@@ -282,6 +309,8 @@ WALLY_CORE_API int wally_get_operations(
  * Set the current overridable operations used by wally.
  *
  * :param ops: The overridable operations to set.
+ *
+ * .. note:: Any NULL members in the passed structure are ignored.
  */
 WALLY_CORE_API int wally_set_operations(
     const struct wally_operations *ops);

src/Makefile.am

@@ -306,6 +306,7 @@ if RUN_PYTHON_TESTS
 	$(AM_V_at)$(PYTHON_TEST) test/test_hash.py
 	$(AM_V_at)$(PYTHON_TEST) test/test_hex.py
 	$(AM_V_at)$(PYTHON_TEST) test/test_hmac.py
+	$(AM_V_at)$(PYTHON_TEST) test/test_internal.py
 	$(AM_V_at)$(PYTHON_TEST) test/test_mnemonic.py
 	$(AM_V_at)$(PYTHON_TEST) test/test_psbt.py
 	$(AM_V_at)$(PYTHON_TEST) test/test_pbkdf2.py

src/internal.c

@@ -19,16 +19,6 @@
 /* Caller is responsible for thread safety */
 static secp256k1_context *global_ctx = NULL;
 
-const secp256k1_context *secp_ctx(void)
-{
-    const uint32_t flags = SECP256K1_CONTEXT_VERIFY | SECP256K1_CONTEXT_SIGN;
-
-    if (!global_ctx)
-        global_ctx = secp256k1_context_create(flags);
-
-    return global_ctx;
-}
-
 int privkey_tweak_add(unsigned char *seckey, const unsigned char *tweak)
 {
     return secp256k1_ec_privkey_tweak_add(secp256k1_context_no_precomp, seckey, tweak);
@@ -251,13 +241,34 @@ static int wally_internal_ec_nonce_fn(unsigned char *nonce32,
     return secp256k1_nonce_function_default(nonce32, msg32, key32, algo16, data, attempt);
 }
 
+struct secp256k1_context_struct *wally_get_new_secp_context(void)
+{
+    return secp256k1_context_create(SECP256K1_CONTEXT_VERIFY | SECP256K1_CONTEXT_SIGN);
+}
+
+struct secp256k1_context_struct *wally_internal_secp_context(void)
+{
+    /* Default implementation uses a non-threadsafe, lazy-initialized global context */
+    if (!global_ctx)
+        global_ctx = wally_get_new_secp_context();
+
+    return global_ctx;
+}
+
 static struct wally_operations _ops = {
+    sizeof(struct wally_operations),
     wally_internal_malloc,
     wally_internal_free,
     wally_internal_bzero,
-    wally_internal_ec_nonce_fn
+    wally_internal_ec_nonce_fn,
+    wally_internal_secp_context
 };
 
+const secp256k1_context *secp_ctx(void)
+{
+    return (const secp256k1_context *)_ops.secp_context_fn();
+}
+
 void *wally_malloc(size_t size)
 {
     return _ops.malloc_fn(size);
@@ -291,21 +302,22 @@ const struct wally_operations *wally_ops(void)
 
 int wally_get_operations(struct wally_operations *output)
 {
-    if (!output)
+    if (!output || output->struct_size != sizeof(struct wally_operations))
         return WALLY_EINVAL;
     memcpy(output, &_ops, sizeof(_ops));
     return WALLY_OK;
 }
 
 int wally_set_operations(const struct wally_operations *ops)
 {
-    if (!ops)
-        return WALLY_EINVAL;
+    if (!ops || ops->struct_size != sizeof(struct wally_operations))
+        return WALLY_EINVAL; /* Null or invalid version of ops */
 #define COPY_FN_PTR(name) if (ops->name) _ops.name = ops->name
     COPY_FN_PTR(malloc_fn);
     COPY_FN_PTR(free_fn);
     COPY_FN_PTR (bzero_fn);
     COPY_FN_PTR (ec_nonce_fn);
+    COPY_FN_PTR (secp_context_fn);
 #undef COPY_FN_PTR
     return WALLY_OK;
 }
@@ -374,13 +386,18 @@ int wally_cleanup(uint32_t flags)
     if (flags)
         return WALLY_EINVAL;
     if (global_ctx) {
-#undef secp256k1_context_destroy
-        secp256k1_context_destroy(global_ctx);
+        wally_secp_context_free(global_ctx);
         global_ctx = NULL;
     }
     return WALLY_OK;
 }
 
+void wally_secp_context_free(struct secp256k1_context_struct *ctx)
+{
+#undef secp256k1_context_destroy
+    if (ctx)
+        secp256k1_context_destroy(ctx);
+}
 
 #ifdef __ANDROID__
 #define malloc(size) wally_malloc(size)

src/test/test_internal.py

@@ -0,0 +1,54 @@
+import unittest
+from util import *
+
+class InternalTests(unittest.TestCase):
+
+    def test_secp_context(self):
+        """Tests for secp context functions"""
+        # Allocate and free a secp context
+        ctx = wally_get_new_secp_context()
+        wally_secp_context_free(ctx)
+
+        # Freeing a NULL context is a no-op
+        wally_secp_context_free(None)
+
+    def test_operations(self):
+        """Tests for overriding the libraries default operations"""
+        # get_operations
+        # NULL output
+        self.assertEqual(wally_get_operations(None), WALLY_EINVAL)
+        # Incorrect struct size
+        ops = wally_operations()
+        ops.struct_size = 0
+        self.assertEqual(wally_get_operations(byref(ops)), WALLY_EINVAL)
+        # Correct struct size succeeds
+        ops.struct_size = sizeof(wally_operations)
+        self.assertEqual(wally_get_operations(byref(ops)), WALLY_OK)
+
+        # set_operations
+        # NULL input
+        self.assertEqual(wally_set_operations(None), WALLY_EINVAL)
+        # Incorrect struct size
+        ops.struct_size = 0
+        self.assertEqual(wally_set_operations(byref(ops)), WALLY_EINVAL)
+        # Correct struct size succeeds
+        ops.struct_size = sizeof(wally_operations)
+        # Set a secp context function that returns NULL
+        def null_secp_context():
+            return None
+        secp_context_fn_t = CFUNCTYPE(c_void_p)
+        ops.secp_context_fn = secp_context_fn_t(null_secp_context)
+        self.assertEqual(wally_set_operations(byref(ops)), WALLY_OK)
+
+        # Verify that the function was set correctly
+        self.assertEqual(wally_secp_randomize(urandom(32), 32), WALLY_ENOMEM)
+
+        # Verify that NULL members are unchanged when setting
+        # TODO: OSX function casting results in a non-null pointer on OSX
+        #ops.secp_context_fn = cast(0, secp_context_fn_t)
+        #self.assertEqual(wally_set_operations(byref(ops)), WALLY_OK)
+        #self.assertEqual(wally_secp_randomize(urandom(32), 32), WALLY_ENOMEM)
+
+
+if __name__ == '__main__':
+    unittest.main()

src/test/util.py

@@ -26,12 +26,15 @@
 _free_fn_t = CFUNCTYPE(c_void_p)
 _bzero_fn_t = CFUNCTYPE(c_void_p, c_ulong)
 _ec_nonce_fn_t = CFUNCTYPE(c_int, c_void_p, c_void_p, c_void_p, c_void_p, c_void_p, c_uint)
+_secp_context_fn_t = CFUNCTYPE(c_void_p)
 
 class wally_operations(Structure):
-    _fields_ = [('malloc_fn', _malloc_fn_t),
+    _fields_ = [('struct_size', c_size_t),
+                ('malloc_fn', _malloc_fn_t),
                 ('free_fn', _free_fn_t),
                 ('bzero_fn', _bzero_fn_t),
-                ('ec_nonce_fn', _ec_nonce_fn_t)]
+                ('ec_nonce_fn', _ec_nonce_fn_t),
+                ('secp_context_fn', _secp_context_fn_t)]
 
 class ext_key(Structure):
     _fields_ = [('chain_code', c_ubyte * 32),
@@ -201,6 +204,9 @@ class wally_psbt(Structure):
     ('wordlist_lookup_index', c_char_p, [c_void_p, c_ulong]),
     ('wordlist_lookup_word', c_ulong, [c_void_p, c_char_p]),
     # Exported functions
+    ('wally_get_secp_context', c_void_p, []),
+    ('wally_get_new_secp_context', c_void_p, []),
+    ('wally_secp_context_free', None, [c_void_p]),
     # BEGIN AUTOGENERATED
     ('bip32_key_free', c_int, [POINTER(ext_key)]),
     ('bip32_key_from_base58', c_int, [c_char_p, POINTER(ext_key)]),
@@ -559,6 +565,7 @@ def make_cbuffer(hex_in):
 _original_ops = wally_operations()
 _new_ops = wally_operations()
 for ops in (_original_ops, _new_ops):
+    ops.struct_size = sizeof(wally_operations)
     assert wally_get_operations(byref(ops)) == WALLY_OK
 
 # Disable internal tests if not available

src/wrap_js/makewrappers/templates/nan.py

@@ -218,6 +218,7 @@
 !!nan_impl!!
 
 NAN_MODULE_INIT(Init) {
+    w_ops.struct_size = sizeof(w_ops);
     wally_get_operations(&w_ops);
     !!nan_decl!!
 }