commit 26e6f56023e749800d95bbc5669c60197d481314 Author: teor <teor@torproject.org> Date: Sat Mar 9 10:50:55 2019 +1000 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. --- changes/bug29706_refactor | 4 ++++ src/or/shared_random.c | 2 +- src/or/shared_random.h | 3 ++- src/or/shared_random_state.c | 20 ++++++++++++++------ src/test/test_shared_random.c | 25 +++++++++++++++---------- 5 files changed, 36 insertions(+), 18 deletions(-) diff --git a/changes/bug29706_refactor b/changes/bug29706_refactor new file mode 100644 index 000000000..ba1d0c7ed --- /dev/null +++ b/changes/bug29706_refactor @@ -0,0 +1,4 @@ + 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. diff --git a/src/or/shared_random.c b/src/or/shared_random.c index 5f6b03f1b..c3e640977 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 9885934cc..68a101979 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 f27eccafc..e99817106 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 0a3c2e119..5d3e574fa 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(); }