Refactor test_utils_general() to fix Coverity warnings

changes/bug29823

@@ -0,0 +1,5 @@
+  o Minor bugfixes (unit tests):
+    - Split test_utils_general() to several smaller test functions in
+      test_utils_general(). This makes it easier to perform resource
+      deallocation on assert failure and fixes Coverity warnings CID 1444117
+      and CID 1444118. Fixes bug 29823; bugfix on 0.2.9.1-alpha.

src/test/test_shared_random.c

@@ -1081,70 +1081,85 @@ test_sr_get_majority_srv_from_votes(void *arg)
   smartlist_free(votes);
 }
 
-/* Test utils that don't depend on authority state */
+/* Testing sr_srv_dup(). */
 static void
-test_utils_general(void *arg)
+test_sr_svr_dup(void *arg)
 {
-  (void) arg;
+  (void)arg;
 
-  /* Testing sr_srv_dup(). */
-  {
-    sr_srv_t *srv = NULL, *dup_srv = NULL;
-    const char *srv_value =
-      "1BDB7C3E973936E4D13A49F37C859B3DC69C429334CF9412E3FEF6399C52D47A";
-    srv = tor_malloc_zero(sizeof(*srv));
-    srv->num_reveals = 42;
-    memcpy(srv->value, srv_value, sizeof(srv->value));
-    dup_srv = sr_srv_dup(srv);
-    tt_assert(dup_srv);
-    tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals);
-    tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value));
-    tor_free(srv);
-    tor_free(dup_srv);
-  }
+  sr_srv_t *srv = NULL, *dup_srv = NULL;
+  const char *srv_value =
+    "1BDB7C3E973936E4D13A49F37C859B3DC69C429334CF9412E3FEF6399C52D47A";
+  srv = tor_malloc_zero(sizeof(*srv));
+  srv->num_reveals = 42;
+  memcpy(srv->value, srv_value, sizeof(srv->value));
+  dup_srv = sr_srv_dup(srv);
+  tt_assert(dup_srv);
+  tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals);
+  tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value));
 
-  /* Testing commitments_are_the_same(). Currently, the check is to test the
-   * value of the encoded commit so let's make sure that actually works. */
-  {
-    /* Payload of 57 bytes that is the length of sr_commit_t->encoded_commit.
-     * 56 bytes of payload and a NUL terminated byte at the end ('\x00')
-     * which comes down to SR_COMMIT_BASE64_LEN + 1. */
-    const char *payload =
-      "\x5d\xb9\x60\xb6\xcc\x51\x68\x52\x31\xd9\x88\x88\x71\x71\xe0\x30"
-      "\x59\x55\x7f\xcd\x61\xc0\x4b\x05\xb8\xcd\xc1\x48\xe9\xcd\x16\x1f"
-      "\x70\x15\x0c\xfc\xd3\x1a\x75\xd0\x93\x6c\xc4\xe0\x5c\xbe\xe2\x18"
-      "\xc7\xaf\x72\xb6\x7c\x9b\x52\x00";
-    sr_commit_t commit1, commit2;
-    memcpy(commit1.encoded_commit, payload, sizeof(commit1.encoded_commit));
-    memcpy(commit2.encoded_commit, payload, sizeof(commit2.encoded_commit));
-    tt_int_op(commitments_are_the_same(&commit1, &commit2), OP_EQ, 1);
-    /* Let's corrupt one of them. */
-    memset(commit1.encoded_commit, 'A', sizeof(commit1.encoded_commit));
-    tt_int_op(commitments_are_the_same(&commit1, &commit2), OP_EQ, 0);
-  }
+ done:
+  tor_free(srv);
+  tor_free(dup_srv);
+}
 
-  /* Testing commit_is_authoritative(). */
-  {
-    crypto_pk_t *k = crypto_pk_new();
-    char digest[DIGEST_LEN];
-    sr_commit_t commit;
+/* Testing commitments_are_the_same(). Currently, the check is to test the
+ * value of the encoded commit so let's make sure that actually works. */
+static void
+test_commitments_are_the_same(void *arg)
+{
+  (void)arg;
 
-    tt_assert(!crypto_pk_generate_key(k));
+  /* Payload of 57 bytes that is the length of sr_commit_t->encoded_commit.
+  * 56 bytes of payload and a NUL terminated byte at the end ('\x00')
+  * which comes down to SR_COMMIT_BASE64_LEN + 1. */
+  const char *payload =
+  "\x5d\xb9\x60\xb6\xcc\x51\x68\x52\x31\xd9\x88\x88\x71\x71\xe0\x30"
+  "\x59\x55\x7f\xcd\x61\xc0\x4b\x05\xb8\xcd\xc1\x48\xe9\xcd\x16\x1f"
+  "\x70\x15\x0c\xfc\xd3\x1a\x75\xd0\x93\x6c\xc4\xe0\x5c\xbe\xe2\x18"
+  "\xc7\xaf\x72\xb6\x7c\x9b\x52\x00";
+  sr_commit_t commit1, commit2;
+  memcpy(commit1.encoded_commit, payload, sizeof(commit1.encoded_commit));
+  memcpy(commit2.encoded_commit, payload, sizeof(commit2.encoded_commit));
+  tt_int_op(commitments_are_the_same(&commit1, &commit2), OP_EQ, 1);
+  /* Let's corrupt one of them. */
+  memset(commit1.encoded_commit, 'A', sizeof(commit1.encoded_commit));
+  tt_int_op(commitments_are_the_same(&commit1, &commit2), OP_EQ, 0);
 
-    tt_int_op(0, OP_EQ, crypto_pk_get_digest(k, digest));
-    memcpy(commit.rsa_identity, digest, sizeof(commit.rsa_identity));
-    tt_int_op(commit_is_authoritative(&commit, digest), OP_EQ, 1);
-    /* Change the pubkey. */
-    memset(commit.rsa_identity, 0, sizeof(commit.rsa_identity));
-    tt_int_op(commit_is_authoritative(&commit, digest), OP_EQ, 0);
-    crypto_pk_free(k);
-  }
+ done:
+  return;
+}
 
-  /* Testing get_phase_str(). */
-  {
-    tt_str_op(get_phase_str(SR_PHASE_REVEAL), OP_EQ, "reveal");
-    tt_str_op(get_phase_str(SR_PHASE_COMMIT), OP_EQ, "commit");
-  }
+/* Testing commit_is_authoritative(). */
+static void
+test_commit_is_authoritative(void *arg)
+{
+  (void)arg;
+
+  crypto_pk_t *k = crypto_pk_new();
+  char digest[DIGEST_LEN];
+  sr_commit_t commit;
+
+  tt_assert(!crypto_pk_generate_key(k));
+
+  tt_int_op(0, OP_EQ, crypto_pk_get_digest(k, digest));
+  memcpy(commit.rsa_identity, digest, sizeof(commit.rsa_identity));
+  tt_int_op(commit_is_authoritative(&commit, digest), OP_EQ, 1);
+  /* Change the pubkey. */
+  memset(commit.rsa_identity, 0, sizeof(commit.rsa_identity));
+  tt_int_op(commit_is_authoritative(&commit, digest), OP_EQ, 0);
+
+ done:
+  crypto_pk_free(k);
+}
+
+static void
+test_get_phase_str(void *arg)
+{
+  (void)arg;
+
+  tt_str_op(get_phase_str(SR_PHASE_REVEAL), OP_EQ, "reveal");
+  tt_str_op(get_phase_str(SR_PHASE_COMMIT), OP_EQ, "commit");
 
  done:
   return;
@@ -1649,7 +1664,12 @@ struct testcase_t sr_tests[] = {
   { "sr_compute_srv", test_sr_compute_srv, TT_FORK, NULL, NULL },
   { "sr_get_majority_srv_from_votes", test_sr_get_majority_srv_from_votes,
     TT_FORK, NULL, NULL },
-  { "utils_general", test_utils_general, TT_FORK, NULL, NULL },
+  { "sr_svr_dup", test_sr_svr_dup, TT_FORK, NULL, NULL },
+  { "commitments_are_the_same", test_commitments_are_the_same, TT_FORK, NULL,
+    NULL },
+  { "commit_is_authoritative", test_commit_is_authoritative, TT_FORK, NULL,
+    NULL },
+  { "get_phase_str", test_get_phase_str, TT_FORK, NULL, NULL },
   { "utils_auth", test_utils_auth, TT_FORK, NULL, NULL },
   { "state_transition", test_state_transition, TT_FORK, NULL, NULL },
   { "state_update", test_state_update, TT_FORK,