Fix some onion helpers

Fix add_onion_helper_clientauth() and add_onion_helper_keyarg() to
explicitly call the appropriate control reply abstractions instead of
allocating a string to pass to their callers.

Part of ticket 30889.

1 files changed, 65 insertions, 41 deletions

diff --git a/src/test/test_controller.c b/src/test/test_controller.c
index ee48d656bd..b9cbe0a14d 100644
--- a/src/test/test_controller.c
+++ b/src/test/test_controller.c
@@ -9,6 +9,7 @@
 #include "feature/control/control.h"
 #include "feature/control/control_cmd.h"
 #include "feature/control/control_getinfo.h"
+#include "feature/control/control_proto.h"
 #include "feature/client/entrynodes.h"
 #include "feature/hs/hs_common.h"
 #include "feature/nodelist/networkstatus.h"
@@ -201,42 +202,58 @@ static const control_cmd_syntax_t one_arg_kwargs_syntax = {
 static const parse_test_params_t parse_one_arg_kwargs_params =
   TESTPARAMS( one_arg_kwargs_syntax, one_arg_kwargs_tests );
 
+static char *reply_str = NULL;
+/* Mock for control_write_reply that copies the string for inspection
+ * by tests */
+static void
+mock_control_write_reply(control_connection_t *conn, int code, int c,
+                                const char *s)
+{
+  (void)conn;
+  (void)code;
+  (void)c;
+  tor_free(reply_str);
+  reply_str = tor_strdup(s);
+}
+
 static void
 test_add_onion_helper_keyarg_v3(void *arg)
 {
   int ret, hs_version;
   add_onion_secret_key_t pk;
   char *key_new_blob = NULL;
-  char *err_msg = NULL;
   const char *key_new_alg = NULL;
 
   (void) arg;
+  MOCK(control_write_reply, mock_control_write_reply);
 
   memset(&pk, 0, sizeof(pk));
 
   /* Test explicit ED25519-V3 key generation. */
+  tor_free(reply_str);
   ret = add_onion_helper_keyarg("NEW:ED25519-V3", 0, &key_new_alg,
                                 &key_new_blob, &pk, &hs_version,
-                                &err_msg);
+                                NULL);
   tt_int_op(ret, OP_EQ, 0);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE);
   tt_assert(pk.v3);
   tt_str_op(key_new_alg, OP_EQ, "ED25519-V3");
   tt_assert(key_new_blob);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
   tor_free(pk.v3); pk.v3 = NULL;
   tor_free(key_new_blob);
 
   /* Test discarding the private key. */
+  tor_free(reply_str);
   ret = add_onion_helper_keyarg("NEW:ED25519-V3", 1, &key_new_alg,
                                 &key_new_blob, &pk, &hs_version,
-                                &err_msg);
+                                NULL);
   tt_int_op(ret, OP_EQ, 0);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE);
   tt_assert(pk.v3);
   tt_ptr_op(key_new_alg, OP_EQ, NULL);
   tt_ptr_op(key_new_blob, OP_EQ, NULL);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
   tor_free(pk.v3); pk.v3 = NULL;
   tor_free(key_new_blob);
 
@@ -256,9 +273,10 @@ test_add_onion_helper_keyarg_v3(void *arg)
 
     tor_asprintf(&key_blob, "ED25519-V3:%s", base64_sk);
     tt_assert(key_blob);
+    tor_free(reply_str);
     ret = add_onion_helper_keyarg(key_blob, 1, &key_new_alg,
                                   &key_new_blob, &pk, &hs_version,
-                                  &err_msg);
+                                  NULL);
     tor_free(key_blob);
     tt_int_op(ret, OP_EQ, 0);
     tt_int_op(hs_version, OP_EQ, HS_VERSION_THREE);
@@ -266,7 +284,7 @@ test_add_onion_helper_keyarg_v3(void *arg)
     tt_mem_op(pk.v3, OP_EQ, hex_sk, 64);
     tt_ptr_op(key_new_alg, OP_EQ, NULL);
     tt_ptr_op(key_new_blob, OP_EQ, NULL);
-    tt_ptr_op(err_msg, OP_EQ, NULL);
+    tt_ptr_op(reply_str, OP_EQ, NULL);
     tor_free(pk.v3); pk.v3 = NULL;
     tor_free(key_new_blob);
   }
@@ -274,7 +292,8 @@ test_add_onion_helper_keyarg_v3(void *arg)
  done:
   tor_free(pk.v3);
   tor_free(key_new_blob);
-  tor_free(err_msg);
+  tor_free(reply_str);
+  UNMOCK(control_write_reply);
 }
 
 static void
@@ -285,72 +304,73 @@ test_add_onion_helper_keyarg_v2(void *arg)
   crypto_pk_t *pk1 = NULL;
   const char *key_new_alg = NULL;
   char *key_new_blob = NULL;
-  char *err_msg = NULL;
   char *encoded = NULL;
   char *arg_str = NULL;
 
   (void) arg;
+  MOCK(control_write_reply, mock_control_write_reply);
 
   memset(&pk, 0, sizeof(pk));
 
   /* Test explicit RSA1024 key generation. */
+  tor_free(reply_str);
   ret = add_onion_helper_keyarg("NEW:RSA1024", 0, &key_new_alg, &key_new_blob,
-                                &pk, &hs_version, &err_msg);
+                                &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, 0);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(pk.v2);
   tt_str_op(key_new_alg, OP_EQ, "RSA1024");
   tt_assert(key_new_blob);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
 
   /* Test "BEST" key generation (Assumes BEST = RSA1024). */
   crypto_pk_free(pk.v2); pk.v2 = NULL;
   tor_free(key_new_blob);
   ret = add_onion_helper_keyarg("NEW:BEST", 0, &key_new_alg, &key_new_blob,
-                                &pk, &hs_version, &err_msg);
+                                &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, 0);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(pk.v2);
   tt_str_op(key_new_alg, OP_EQ, "RSA1024");
   tt_assert(key_new_blob);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
 
   /* Test discarding the private key. */
   crypto_pk_free(pk.v2); pk.v2 = NULL;
   tor_free(key_new_blob);
   ret = add_onion_helper_keyarg("NEW:BEST", 1, &key_new_alg, &key_new_blob,
-                               &pk, &hs_version, &err_msg);
+                               &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, 0);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(pk.v2);
   tt_ptr_op(key_new_alg, OP_EQ, NULL);
   tt_ptr_op(key_new_blob, OP_EQ, NULL);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
 
   /* Test generating a invalid key type. */
   crypto_pk_free(pk.v2); pk.v2 = NULL;
   ret = add_onion_helper_keyarg("NEW:RSA512", 0, &key_new_alg, &key_new_blob,
-                               &pk, &hs_version, &err_msg);
+                               &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, -1);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(!pk.v2);
   tt_ptr_op(key_new_alg, OP_EQ, NULL);
   tt_ptr_op(key_new_blob, OP_EQ, NULL);
-  tt_assert(err_msg);
+  tt_assert(reply_str);
 
   /* Test loading a RSA1024 key. */
-  tor_free(err_msg);
+  tor_free(reply_str);
   pk1 = pk_generate(0);
   tt_int_op(0, OP_EQ, crypto_pk_base64_encode_private(pk1, &encoded));
   tor_asprintf(&arg_str, "RSA1024:%s", encoded);
   ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob,
-                                &pk, &hs_version, &err_msg);
+                                &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, 0);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(pk.v2);
   tt_ptr_op(key_new_alg, OP_EQ, NULL);
   tt_ptr_op(key_new_blob, OP_EQ, NULL);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
   tt_int_op(crypto_pk_cmp_keys(pk1, pk.v2), OP_EQ, 0);
 
   /* Test loading a invalid key type. */
@@ -359,36 +379,37 @@ test_add_onion_helper_keyarg_v2(void *arg)
   crypto_pk_free(pk.v2); pk.v2 = NULL;
   tor_asprintf(&arg_str, "RSA512:%s", encoded);
   ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob,
-                                &pk, &hs_version, &err_msg);
+                                &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, -1);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(!pk.v2);
   tt_ptr_op(key_new_alg, OP_EQ, NULL);
   tt_ptr_op(key_new_blob, OP_EQ, NULL);
-  tt_assert(err_msg);
+  tt_assert(reply_str);
 
   /* Test loading a invalid key. */
   tor_free(arg_str);
   crypto_pk_free(pk.v2); pk.v2 = NULL;
-  tor_free(err_msg);
+  tor_free(reply_str);
   encoded[strlen(encoded)/2] = '\0';
   tor_asprintf(&arg_str, "RSA1024:%s", encoded);
   ret = add_onion_helper_keyarg(arg_str, 0, &key_new_alg, &key_new_blob,
-                               &pk, &hs_version, &err_msg);
+                               &pk, &hs_version, NULL);
   tt_int_op(ret, OP_EQ, -1);
   tt_int_op(hs_version, OP_EQ, HS_VERSION_TWO);
   tt_assert(!pk.v2);
   tt_ptr_op(key_new_alg, OP_EQ, NULL);
   tt_ptr_op(key_new_blob, OP_EQ, NULL);
-  tt_assert(err_msg);
+  tt_assert(reply_str);
 
  done:
   crypto_pk_free(pk1);
   crypto_pk_free(pk.v2);
   tor_free(key_new_blob);
-  tor_free(err_msg);
+  tor_free(reply_str);
   tor_free(encoded);
   tor_free(arg_str);
+  UNMOCK(control_write_reply);
 }
 
 static void
@@ -542,49 +563,52 @@ static void
 test_add_onion_helper_clientauth(void *arg)
 {
   rend_authorized_client_t *client = NULL;
-  char *err_msg = NULL;
   int created = 0;
 
   (void)arg;
 
+  MOCK(control_write_reply, mock_control_write_reply);
   /* Test "ClientName" only. */
-  client = add_onion_helper_clientauth("alice", &created, &err_msg);
+  tor_free(reply_str);
+  client = add_onion_helper_clientauth("alice", &created, NULL);
   tt_assert(client);
   tt_assert(created);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
   rend_authorized_client_free(client);
 
   /* Test "ClientName:Blob" */
+  tor_free(reply_str);
   client = add_onion_helper_clientauth("alice:475hGBHPlq7Mc0cRZitK/B",
-                                       &created, &err_msg);
+                                       &created, NULL);
   tt_assert(client);
   tt_assert(!created);
-  tt_ptr_op(err_msg, OP_EQ, NULL);
+  tt_ptr_op(reply_str, OP_EQ, NULL);
   rend_authorized_client_free(client);
 
   /* Test invalid client names */
+  tor_free(reply_str);
   client = add_onion_helper_clientauth("no*asterisks*allowed", &created,
-                                       &err_msg);
+                                       NULL);
   tt_ptr_op(client, OP_EQ, NULL);
-  tt_assert(err_msg);
-  tor_free(err_msg);
+  tt_assert(reply_str);
 
   /* Test invalid auth cookie */
-  client = add_onion_helper_clientauth("alice:12345", &created, &err_msg);
+  tor_free(reply_str);
+  client = add_onion_helper_clientauth("alice:12345", &created, NULL);
   tt_ptr_op(client, OP_EQ, NULL);
-  tt_assert(err_msg);
-  tor_free(err_msg);
+  tt_assert(reply_str);
 
   /* Test invalid syntax */
+  tor_free(reply_str);
   client = add_onion_helper_clientauth(":475hGBHPlq7Mc0cRZitK/B", &created,
-                                       &err_msg);
+                                       NULL);
   tt_ptr_op(client, OP_EQ, NULL);
-  tt_assert(err_msg);
-  tor_free(err_msg);
+  tt_assert(reply_str);
 
  done:
   rend_authorized_client_free(client);
-  tor_free(err_msg);
+  tor_free(reply_str);
+  UNMOCK(control_write_reply);
 }
 
 /* Mocks and data/variables used for GETINFO download status tests */