From 4939f7bb233b1d96d281214d6e7c99e82c82e288 Mon Sep 17 00:00:00 2001
From: Jesper Louis Andersen <jesper.louis.andersen@gmail.com>
Date: Fri, 24 Jan 2020 15:18:04 +0100
Subject: [PATCH] Protect the signature ctx with a mutex

This is the same game as with the
generichash construction. We want
to protect it with a mutex so
different processes can safely do
work on the same resource.

While here, also move the _update
function onto the dirty scheduler.
It is by far the most expensive
operation, and why it wasn't there
in the first place is odd. This should
unblock the scheduler on long
sign-checks. It also move the
possible mutex block onto the
dirty scheduler thread, away from
the core schedulers, improving
latency in the system as a result.
---
 CHANGELOG.md        |  7 -------
 c_src/enacl_nif.c   |  3 ++-
 c_src/generichash.c |  2 +-
 c_src/sign.c        | 23 ++++++++++++++++++++++-
 4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5096bda..490c006 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,13 +17,6 @@ is a list of changes which are planned for a 1.0 release.
 Under errors, the current code can leak binaries. I don't want that to happen, so we
 are going to use a better cleanup scheme in the code base.
 
-- Play the mutex game with:
-  - Sign multi-part types
-
-The resource-variant structs need to be mutex-protected. Otherwise we run the risk of
-having multiple simultaneous updates to the hash state without having proper critical
-sections applied. The current code is subtly in error!
-
 ## [Unreleased]
 
 ### Compatibility
diff --git a/c_src/enacl_nif.c b/c_src/enacl_nif.c
index 6993db9..1e47712 100644
--- a/c_src/enacl_nif.c
+++ b/c_src/enacl_nif.c
@@ -286,7 +286,8 @@ static ErlNifFunc nif_funcs[] = {
     erl_nif_dirty_job_cpu_bound_macro("crypto_sign_verify_detached", 3,
                                       enacl_crypto_sign_verify_detached),
     {"crypto_sign_init", 0, enacl_crypto_sign_init},
-    {"crypto_sign_update", 2, enacl_crypto_sign_update},
+    erl_nif_dirty_job_cpu_bound_macro("crypto_sign_update", 2,
+                                      enacl_crypto_sign_update),
     erl_nif_dirty_job_cpu_bound_macro("crypto_sign_final_create", 2,
                                       enacl_crypto_sign_final_create),
     erl_nif_dirty_job_cpu_bound_macro("crypto_sign_final_verify", 3,
diff --git a/c_src/generichash.c b/c_src/generichash.c
index 222dd82..524def8 100644
--- a/c_src/generichash.c
+++ b/c_src/generichash.c
@@ -194,7 +194,7 @@ ERL_NIF_TERM enacl_crypto_generichash_init(ErlNifEnv *env, int argc,
   obj->outlen = hash_size;
 
   if ((obj->mtx = enif_mutex_create("enacl.generichash")) == NULL) {
-    ret = enacl_error_tuple(env, "generichash_mutex_error");
+    ret = enacl_error_tuple(env, "mutex_create");
     goto err;
   }
 
diff --git a/c_src/sign.c b/c_src/sign.c
index d836a54..07f1446 100644
--- a/c_src/sign.c
+++ b/c_src/sign.c
@@ -6,6 +6,7 @@
 #include "sign.h"
 
 typedef struct enacl_sign_ctx {
+  ErlNifMutex *mtx;
   crypto_sign_state *state; // The underlying signature state
   int alive; // Is the context still valid for updates/finalization
 } enacl_sign_ctx;
@@ -35,6 +36,9 @@ void enacl_sign_ctx_dtor(ErlNifEnv *env, enacl_sign_ctx *obj) {
     enif_free(obj->state);
   }
 
+  if (obj->mtx != NULL)
+    enif_mutex_destroy(obj->mtx);
+
   return;
 }
 
@@ -63,6 +67,11 @@ ERL_NIF_TERM enacl_crypto_sign_init(ErlNifEnv *env, int argc,
   }
   obj->alive = 1;
 
+  if ((obj->mtx = enif_mutex_create("enacl.sign")) == NULL) {
+    ret = enacl_error_tuple(env, "mutex_create");
+    goto free;
+  }
+
   if (0 != crypto_sign_init(obj->state)) {
     ret = enacl_error_tuple(env, "sign_init_error");
     goto free;
@@ -83,6 +92,7 @@ free:
       obj->state = NULL;
     }
 release:
+  // This also frees the mutex via the destructor
   enif_release_resource(obj);
 done:
   return ret;
@@ -108,6 +118,7 @@ ERL_NIF_TERM enacl_crypto_sign_update(ErlNifEnv *env, int argc,
   if (!enif_inspect_binary(env, argv[1], &data))
     goto bad_arg;
 
+  enif_mutex_lock(obj->mtx);
   if (!obj->alive) {
     ret = enacl_error_tuple(env, "finalized");
     goto done;
@@ -124,6 +135,7 @@ ERL_NIF_TERM enacl_crypto_sign_update(ErlNifEnv *env, int argc,
 bad_arg:
   return enif_make_badarg(env);
 done:
+  enif_mutex_unlock(obj->mtx);
   return ret;
 }
 
@@ -143,6 +155,7 @@ ERL_NIF_TERM enacl_crypto_sign_final_create(ErlNifEnv *env, int argc,
   if (sk.size != crypto_sign_SECRETKEYBYTES)
     goto bad_arg;
 
+  enif_mutex_lock(obj->mtx);
   if (!obj->alive) {
     ret = enacl_error_tuple(env, "finalized");
     goto done;
@@ -174,6 +187,7 @@ cleanup:
   enif_free(obj->state);
   obj->state = NULL;
 done:
+  enif_mutex_unlock(obj->mtx);
   return ret;
 }
 
@@ -194,6 +208,12 @@ ERL_NIF_TERM enacl_crypto_sign_final_verify(ErlNifEnv *env, int argc,
   if (pk.size != crypto_sign_PUBLICKEYBYTES)
     goto bad_arg;
 
+  enif_mutex_lock(obj->mtx);
+  if (!obj->alive) {
+    ret = enacl_error_tuple(env, "finalized");
+    goto done;
+  }
+
   if (0 == crypto_sign_final_verify(obj->state, sig.data, pk.data)) {
     ret = enif_make_atom(env, ATOM_OK);
   } else {
@@ -210,7 +230,8 @@ cleanup:
   sodium_memzero(obj->state, crypto_sign_statebytes());
   enif_free(obj->state);
   obj->state = NULL;
-
+done:
+  enif_mutex_unlock(obj->mtx);
   return ret;
 }