sr: Free SRVs before replacing them in state_query_put_()

Refactor the shared random state's memory management so that it actually
takes ownership of the shared random value pointers.

Fixes bug 29706; bugfix on 0.2.9.1-alpha.

4 files changed, 32 insertions, 18 deletions

diff --git a/src/or/shared_random.c b/src/or/shared_random.c
index 5f6b03f1ba..c3e6409771 100644
--- a/src/or/shared_random.c
+++ b/src/or/shared_random.c
@@ -112,7 +112,7 @@ static const char sr_flag_ns_str[] = "shared-rand-participate";
 static int32_t num_srv_agreements_from_vote;
 
 /* Return a heap allocated copy of the SRV <b>orig</b>. */
-STATIC sr_srv_t *
+sr_srv_t *
 srv_dup(const sr_srv_t *orig)
 {
   sr_srv_t *duplicate = NULL;
diff --git a/src/or/shared_random.h b/src/or/shared_random.h
index 9885934cc7..68a1019794 100644
--- a/src/or/shared_random.h
+++ b/src/or/shared_random.h
@@ -129,6 +129,8 @@ const char *sr_commit_get_rsa_fpr(const sr_commit_t *commit)
 void sr_compute_srv(void);
 sr_commit_t *sr_generate_our_commit(time_t timestamp,
                                     const authority_cert_t *my_rsa_cert);
+sr_srv_t *srv_dup(const sr_srv_t *orig);
+
 #ifdef SHARED_RANDOM_PRIVATE
 
 /* Encode */
@@ -146,7 +148,6 @@ STATIC sr_srv_t *get_majority_srv_from_votes(const smartlist_t *votes,
                                              int current);
 
 STATIC void save_commit_to_state(sr_commit_t *commit);
-STATIC sr_srv_t *srv_dup(const sr_srv_t *orig);
 STATIC int commitments_are_the_same(const sr_commit_t *commit_one,
                                     const sr_commit_t *commit_two);
 STATIC int commit_is_authoritative(const sr_commit_t *commit,
diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c
index f27eccafc7..e99817106c 100644
--- a/src/or/shared_random_state.c
+++ b/src/or/shared_random_state.c
@@ -92,6 +92,8 @@ static const config_format_t state_format = {
   &state_extra_var,
 };
 
+static void state_query_del_(sr_state_object_t obj_type, void *data);
+
 /* Return a string representation of a protocol phase. */
 STATIC const char *
 get_phase_str(sr_phase_t phase)
@@ -883,7 +885,8 @@ state_query_get_(sr_state_object_t obj_type, const void *data)
 }
 
 /* Helper function: This handles the PUT state action using an
- * <b>obj_type</b> and <b>data</b> needed for the action. */
+ * <b>obj_type</b> and <b>data</b> needed for the action.
+ * PUT frees the previous data before replacing it, if needed. */
 static void
 state_query_put_(sr_state_object_t obj_type, void *data)
 {
@@ -892,13 +895,18 @@ state_query_put_(sr_state_object_t obj_type, void *data)
   {
     sr_commit_t *commit = data;
     tor_assert(commit);
+    /* commit_add_to_state() frees the old commit, if there is one */
     commit_add_to_state(commit, sr_state);
     break;
   }
   case SR_STATE_OBJ_CURSRV:
+    /* Always free the old SRV */
+    state_query_del_(SR_STATE_OBJ_CURSRV, NULL);
     sr_state->current_srv = (sr_srv_t *) data;
     break;
   case SR_STATE_OBJ_PREVSRV:
+    /* Always free the old SRV */
+    state_query_del_(SR_STATE_OBJ_PREVSRV, NULL);
     sr_state->previous_srv = (sr_srv_t *) data;
     break;
   case SR_STATE_OBJ_VALID_AFTER:
@@ -1021,16 +1029,16 @@ state_del_previous_srv(void)
   state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_PREVSRV, NULL, NULL);
 }
 
-/* Rotate SRV value by freeing the previous value, assigning the current
- * value to the previous one and nullifying the current one. */
+/* Rotate SRV value by setting the previous SRV to the current SRV, and
+ * clearing the current SRV. */
 STATIC void
 state_rotate_srv(void)
 {
   /* First delete previous SRV from the state. Object will be freed. */
   state_del_previous_srv();
-  /* Set previous SRV with the current one. */
-  sr_state_set_previous_srv(sr_state_get_current_srv());
-  /* Nullify the current srv. */
+  /* Set previous SRV to a copy of the current one. */
+  sr_state_set_previous_srv(srv_dup(sr_state_get_current_srv()));
+  /* Free and NULL the current srv. */
   sr_state_set_current_srv(NULL);
 }
 
diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c
index 0a3c2e119b..5d3e574fac 100644
--- a/src/test/test_shared_random.c
+++ b/src/test/test_shared_random.c
@@ -1013,6 +1013,7 @@ test_state_transition(void *arg)
 {
   sr_state_t *state = NULL;
   time_t now = time(NULL);
+  sr_srv_t *cur = NULL;
 
   (void) arg;
 
@@ -1051,44 +1052,47 @@ test_state_transition(void *arg)
 
   /* Test SRV rotation in our state. */
   {
-    const sr_srv_t *cur, *prev;
     test_sr_setup_srv(1);
-    cur = sr_state_get_current_srv();
+    tt_assert(sr_state_get_current_srv());
+    /* Take a copy of the data, because the state owns the pointer */
+    cur = srv_dup(sr_state_get_current_srv());
     tt_assert(cur);
-    /* After, current srv should be the previous and then set to NULL. */
+    /* After, the previous SRV should be the same as the old current SRV, and
+     * the current SRV should be set to NULL */
     state_rotate_srv();
-    prev = sr_state_get_previous_srv();
-    tt_assert(prev == cur);
+    tt_mem_op(sr_state_get_previous_srv(), OP_EQ, cur, sizeof(sr_srv_t));
     tt_assert(!sr_state_get_current_srv());
     sr_state_clean_srvs();
+    tor_free(cur);
   }
 
   /* New protocol run. */
   {
-    const sr_srv_t *cur;
     /* Setup some new SRVs so we can confirm that a new protocol run
      * actually makes them rotate and compute new ones. */
     test_sr_setup_srv(1);
-    cur = sr_state_get_current_srv();
-    tt_assert(cur);
+    tt_assert(sr_state_get_current_srv());
+    /* Take a copy of the data, because the state owns the pointer */
+    cur = srv_dup(sr_state_get_current_srv());
     set_sr_phase(SR_PHASE_REVEAL);
     MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m);
     new_protocol_run(now);
     UNMOCK(get_my_v3_authority_cert);
     /* Rotation happened. */
-    tt_assert(sr_state_get_previous_srv() == cur);
+    tt_mem_op(sr_state_get_previous_srv(), OP_EQ, cur, sizeof(sr_srv_t));
     /* We are going into COMMIT phase so we had to rotate our SRVs. Usually
      * our current SRV would be NULL but a new protocol run should make us
      * compute a new SRV. */
     tt_assert(sr_state_get_current_srv());
     /* Also, make sure we did change the current. */
-    tt_assert(sr_state_get_current_srv() != cur);
+    tt_mem_op(sr_state_get_current_srv(), OP_NE, cur, sizeof(sr_srv_t));
     /* We should have our commitment alone. */
     tt_int_op(digestmap_size(state->commits), ==, 1);
     tt_int_op(state->n_reveal_rounds, ==, 0);
     tt_int_op(state->n_commit_rounds, ==, 0);
     /* 46 here since we were at 45 just before. */
     tt_u64_op(state->n_protocol_runs, ==, 46);
+    tor_free(cur);
   }
 
   /* Cleanup of SRVs. */
@@ -1099,6 +1103,7 @@ test_state_transition(void *arg)
   }
 
  done:
+  tor_free(cur);
   sr_state_free();
 }