[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-commits] [tor/master] sr: Free SRVs before replacing them in state_query_put_()
commit 26e6f56023e749800d95bbc5669c60197d481314
Author: teor <teor@xxxxxxxxxxxxxx>
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();
}
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits