Commit 26e6f560 authored by teor's avatar teor
Browse files

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.
parent 9400da9b

changes/bug29706_refactor

0 → 100644
+4 −0
Original line number Diff line number Diff line 
  o Minor bugfixes (memory management):

    - 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.

src/or/shared_random.c

+1 −1
Original line number Diff line number Diff line
@@ -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;

src/or/shared_random.h

+2 −1
Original line number Diff line number Diff line
@@ -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,

src/or/shared_random_state.c

+14 −6
Original line number Diff line number Diff line
@@ -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);

}

src/test/test_shared_random.c

+15 −10
Original line number Diff line number Diff line
@@ -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();

}