From 9400da9b5e44bfce0684a3b36edb37465be514d6 Mon Sep 17 00:00:00 2001 From: teor Date: Sat, 9 Mar 2019 10:50:07 +1000 Subject: [PATCH 1/6] test/sr: Free SRVs before replacing them in test_sr_setup_srv() Stop leaking parts of the shared random state in the shared-random unit tests. The previous fix in 29599 was incomplete. Fixes bug 29706; bugfix on 0.2.9.1-alpha. --- changes/bug29706_minimal | 4 ++++ src/or/shared_random_state.c | 4 ++-- src/or/shared_random_state.h | 2 ++ src/test/test_shared_random.c | 4 ++++ 4 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 changes/bug29706_minimal diff --git a/changes/bug29706_minimal b/changes/bug29706_minimal new file mode 100644 index 0000000000..9d4a43326c --- /dev/null +++ b/changes/bug29706_minimal @@ -0,0 +1,4 @@ + o Minor bugfixes (memory management, testing): + - Stop leaking parts of the shared random state in the shared-random unit + tests. The previous fix in 29599 was incomplete. + Fixes bug 29706; bugfix on 0.2.9.1-alpha. diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index 8438d46404..f27eccafc7 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -1007,7 +1007,7 @@ state_query(sr_state_action_t action, sr_state_object_t obj_type, /* Delete the current SRV value from the state freeing it and the value is set * to NULL meaning empty. */ -static void +STATIC void state_del_current_srv(void) { state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_CURSRV, NULL, NULL); @@ -1015,7 +1015,7 @@ state_del_current_srv(void) /* Delete the previous SRV value from the state freeing it and the value is * set to NULL meaning empty. */ -static void +STATIC void state_del_previous_srv(void) { state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_PREVSRV, NULL, NULL); diff --git a/src/or/shared_random_state.h b/src/or/shared_random_state.h index 43a7f1d284..cf027f2d35 100644 --- a/src/or/shared_random_state.h +++ b/src/or/shared_random_state.h @@ -140,6 +140,8 @@ STATIC int is_phase_transition(sr_phase_t next_phase); STATIC void set_sr_phase(sr_phase_t phase); STATIC sr_state_t *get_sr_state(void); +STATIC void state_del_previous_srv(void); +STATIC void state_del_current_srv(void); #endif /* TOR_UNIT_TESTS */ diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index cebe772d94..0a3c2e119b 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -461,6 +461,8 @@ test_sr_setup_srv(int also_current) "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ", sizeof(srv->value)); + /* sr_state_set_previous_srv() does not free() the old previous srv. */ + state_del_previous_srv(); sr_state_set_previous_srv(srv); if (also_current) { @@ -470,6 +472,8 @@ test_sr_setup_srv(int also_current) "NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN", sizeof(srv->value)); + /* sr_state_set_previous_srv() does not free() the old current srv. */ + state_del_current_srv(); sr_state_set_current_srv(srv); } } From 26e6f56023e749800d95bbc5669c60197d481314 Mon Sep 17 00:00:00 2001 From: teor Date: Sat, 9 Mar 2019 10:50:55 +1000 Subject: [PATCH 2/6] 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(-) create mode 100644 changes/bug29706_refactor diff --git a/changes/bug29706_refactor b/changes/bug29706_refactor new file mode 100644 index 0000000000..ba1d0c7edd --- /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 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 orig. */ -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 - * obj_type and data needed for the action. */ + * obj_type and data 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(); } From 593fde930fa52e0f4ed7aaf70c4ea9e4615d2a0e Mon Sep 17 00:00:00 2001 From: teor Date: Sat, 9 Mar 2019 11:48:05 +1000 Subject: [PATCH 3/6] sr: rename srv_dup() to sr_srv_dup() --- src/or/shared_random.c | 6 +++--- src/or/shared_random.h | 2 +- src/or/shared_random_state.c | 2 +- src/test/test_shared_random.c | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/or/shared_random.c b/src/or/shared_random.c index c3e6409771..ddb6de1a59 100644 --- a/src/or/shared_random.c +++ b/src/or/shared_random.c @@ -113,7 +113,7 @@ static int32_t num_srv_agreements_from_vote; /* Return a heap allocated copy of the SRV orig. */ sr_srv_t * -srv_dup(const sr_srv_t *orig) +sr_srv_dup(const sr_srv_t *orig) { sr_srv_t *duplicate = NULL; @@ -1318,8 +1318,8 @@ sr_act_post_consensus(const networkstatus_t *consensus) * decided by the majority. */ sr_state_unset_fresh_srv(); /* Set the SR values from the given consensus. */ - sr_state_set_previous_srv(srv_dup(consensus->sr_info.previous_srv)); - sr_state_set_current_srv(srv_dup(consensus->sr_info.current_srv)); + sr_state_set_previous_srv(sr_srv_dup(consensus->sr_info.previous_srv)); + sr_state_set_current_srv(sr_srv_dup(consensus->sr_info.current_srv)); } /* Prepare our state so that it's ready for the next voting period. */ diff --git a/src/or/shared_random.h b/src/or/shared_random.h index 68a1019794..c640470c16 100644 --- a/src/or/shared_random.h +++ b/src/or/shared_random.h @@ -129,7 +129,7 @@ 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); +sr_srv_t *sr_srv_dup(const sr_srv_t *orig); #ifdef SHARED_RANDOM_PRIVATE diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index e99817106c..50c6d3dd7a 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -1037,7 +1037,7 @@ state_rotate_srv(void) /* First delete previous SRV from the state. Object will be freed. */ state_del_previous_srv(); /* Set previous SRV to a copy of the current one. */ - sr_state_set_previous_srv(srv_dup(sr_state_get_current_srv())); + sr_state_set_previous_srv(sr_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 5d3e574fac..93125e5135 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -932,7 +932,7 @@ test_utils(void *arg) { (void) arg; - /* Testing srv_dup(). */ + /* Testing sr_srv_dup(). */ { sr_srv_t *srv = NULL, *dup_srv = NULL; const char *srv_value = @@ -940,7 +940,7 @@ test_utils(void *arg) srv = tor_malloc_zero(sizeof(*srv)); srv->num_reveals = 42; memcpy(srv->value, srv_value, sizeof(srv->value)); - dup_srv = srv_dup(srv); + dup_srv = sr_srv_dup(srv); tt_assert(dup_srv); tt_u64_op(dup_srv->num_reveals, ==, srv->num_reveals); tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value)); @@ -1055,7 +1055,7 @@ test_state_transition(void *arg) test_sr_setup_srv(1); 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()); + cur = sr_srv_dup(sr_state_get_current_srv()); tt_assert(cur); /* After, the previous SRV should be the same as the old current SRV, and * the current SRV should be set to NULL */ @@ -1073,7 +1073,7 @@ test_state_transition(void *arg) test_sr_setup_srv(1); 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()); + cur = sr_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); From 0cca55411074a78096dab1b84a4781e790e9aece Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 12 Mar 2019 10:24:34 +1000 Subject: [PATCH 4/6] sr: Check for replacing a SRV pointer with the same pointer Check if the new pointer is the same as the old one: if it is, it's probably a bug: * the caller may have confused current and previous, or * they may have forgotten to sr_srv_dup(). Putting NULL multiple times is allowed. Part of 29706. --- src/or/shared_random_state.c | 32 +++-- src/test/test_shared_random.c | 212 +++++++++++++++++++++++++++++++++- 2 files changed, 230 insertions(+), 14 deletions(-) diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index 50c6d3dd7a..22a503b89b 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -900,14 +900,26 @@ state_query_put_(sr_state_object_t obj_type, void *data) 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; + /* Check if the new pointer is the same as the old one: if it is, it's + * probably a bug. The caller may have confused current and previous, + * or they may have forgotten to sr_srv_dup(). + * Putting NULL multiple times is allowed. */ + if (!BUG(data && sr_state->current_srv == (sr_srv_t *) data)) { + /* We own the old SRV, so we need to free it. */ + 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; + /* Check if the new pointer is the same as the old one: if it is, it's + * probably a bug. The caller may have confused current and previous, + * or they may have forgotten to sr_srv_dup(). + * Putting NULL multiple times is allowed. */ + if (!BUG(data && sr_state->previous_srv == (sr_srv_t *) data)) { + /* We own the old SRV, so we need to free it. */ + state_query_del_(SR_STATE_OBJ_PREVSRV, NULL); + sr_state->previous_srv = (sr_srv_t *) data; + } break; case SR_STATE_OBJ_VALID_AFTER: sr_state->valid_after = *((time_t *) data); @@ -1059,7 +1071,9 @@ sr_state_get_phase(void) return *(sr_phase_t *) ptr; } -/* Return the previous SRV value from our state. Value CAN be NULL. */ +/* Return the previous SRV value from our state. Value CAN be NULL. + * The state object owns the SRV, so the calling code should not free the SRV. + * Use sr_srv_dup() if you want to keep a copy of the SRV. */ const sr_srv_t * sr_state_get_previous_srv(void) { @@ -1078,7 +1092,9 @@ sr_state_set_previous_srv(const sr_srv_t *srv) NULL); } -/* Return the current SRV value from our state. Value CAN be NULL. */ +/* Return the current SRV value from our state. Value CAN be NULL. + * The state object owns the SRV, so the calling code should not free the SRV. + * Use sr_srv_dup() if you want to keep a copy of the SRV. */ const sr_srv_t * sr_state_get_current_srv(void) { diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index 93125e5135..c81b6ec2d6 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -38,7 +38,8 @@ trusteddirserver_get_by_v3_auth_digest_m(const char *digest) } /* Setup a minimal dirauth environment by initializing the SR state and - * making sure the options are set to be an authority directory. */ + * making sure the options are set to be an authority directory. + * You must only call this function once per process. */ static void init_authority_state(void) { @@ -451,7 +452,9 @@ test_encoding(void *arg) /** Setup some SRVs in our SR state. If also_current is set, then set * both current and previous SRVs. - * Helper of test_vote() and test_sr_compute_srv(). */ + * Helper of test_vote() and test_sr_compute_srv(). + * You must call sr_state_free() to free the state at the end of each test + * function (on pass or fail). */ static void test_sr_setup_srv(int also_current) { @@ -927,8 +930,9 @@ test_sr_get_majority_srv_from_votes(void *arg) smartlist_free(votes); } +/* Test utils that don't depend on authority state */ static void -test_utils(void *arg) +test_utils_general(void *arg) { (void) arg; @@ -991,9 +995,19 @@ test_utils(void *arg) tt_str_op(get_phase_str(SR_PHASE_COMMIT), ==, "commit"); } + done: + return; +} + +/* Test utils that depend on authority state */ +static void +test_utils_auth(void *arg) +{ + (void)arg; + init_authority_state(); + /* Testing phase transition */ { - init_authority_state(); set_sr_phase(SR_PHASE_COMMIT); tt_int_op(is_phase_transition(SR_PHASE_REVEAL), ==, 1); tt_int_op(is_phase_transition(SR_PHASE_COMMIT), ==, 0); @@ -1004,8 +1018,193 @@ test_utils(void *arg) tt_int_op(is_phase_transition(42), ==, 1); } + /* Testing get, set, delete, clean SRVs */ + + { + /* Just set the previous SRV */ + test_sr_setup_srv(0); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + } + + { + /* Delete the SRVs one at a time */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And in the opposite order */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And both at once */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And do the gets and sets multiple times */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + } + + { + /* Now set the SRVs to NULL instead */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_current_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + sr_state_set_previous_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And in the opposite order */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_previous_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_current_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And both at once */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And do the gets and sets multiple times */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_previous_srv(NULL); + sr_state_set_previous_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_current_srv(NULL); + sr_state_set_previous_srv(NULL); + sr_state_set_current_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + } + + { + /* Now copy the values across */ + test_sr_setup_srv(1); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Check that the content is different */ + tt_mem_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Set the current to the previous: the protocol goes the other way */ + sr_state_set_current_srv(sr_srv_dup(sr_state_get_previous_srv())); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Check that the content is the same */ + tt_mem_op(sr_state_get_previous_srv(), OP_EQ, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + } + + { + /* Now copy a value onto itself */ + test_sr_setup_srv(1); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Take a copy of the old value */ + sr_srv_t old_current_srv; + memcpy(&old_current_srv, sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Check that the content is different */ + tt_mem_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Set the current to the current: the protocol never replaces an SRV with + * the same value */ + sr_state_set_current_srv(sr_srv_dup(sr_state_get_current_srv())); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Check that the content is different between current and previous */ + tt_mem_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Check that the content is the same as the old content */ + tt_mem_op(&old_current_srv, OP_EQ, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + } + + /* I don't think we can say "expect a BUG()" in our tests. */ +#if 0 + { + /* Now copy a value onto itself without sr_srv_dup(). + * This should fail with a BUG() warning. */ + test_sr_setup_srv(1); + sr_state_set_current_srv(sr_state_get_current_srv()); + sr_state_set_previous_srv(sr_state_get_previous_srv()); + } +#endif + done: - return; + sr_state_free(); } static void @@ -1293,7 +1492,8 @@ 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", test_utils, TT_FORK, NULL, NULL }, + { "utils_general", test_utils_general, 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, NULL, NULL }, From 9eeff921ae7b786d960ea4286d5bba56edaef47f Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 12 Mar 2019 11:09:48 +1000 Subject: [PATCH 5/6] sr: BUG() on NULL sr_state before doing a state_query_*() Part of #29706. --- src/or/shared_random_state.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index 22a503b89b..5011b5c7f3 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -857,6 +857,9 @@ state_query_get_commit(const char *rsa_fpr) static void * state_query_get_(sr_state_object_t obj_type, const void *data) { + if (BUG(!sr_state)) + return NULL; + void *obj = NULL; switch (obj_type) { @@ -890,6 +893,9 @@ state_query_get_(sr_state_object_t obj_type, const void *data) static void state_query_put_(sr_state_object_t obj_type, void *data) { + if (BUG(!sr_state)) + return; + switch (obj_type) { case SR_STATE_OBJ_COMMIT: { @@ -939,6 +945,9 @@ state_query_put_(sr_state_object_t obj_type, void *data) static void state_query_del_all_(sr_state_object_t obj_type) { + if (BUG(!sr_state)) + return; + switch (obj_type) { case SR_STATE_OBJ_COMMIT: { @@ -967,6 +976,9 @@ state_query_del_(sr_state_object_t obj_type, void *data) { (void) data; + if (BUG(!sr_state)) + return; + switch (obj_type) { case SR_STATE_OBJ_PREVSRV: tor_free(sr_state->previous_srv); From dfc3e552a3b7617add0bd576a5c34dca59f42649 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 12 Mar 2019 11:34:52 +1000 Subject: [PATCH 6/6] test/sr: update sr_state_free() to sr_state_free_all() The function name changed between 0.2.9 and 0.3.4. --- src/test/test_shared_random.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index 45f0e1db7f..991d73128e 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -548,7 +548,7 @@ test_encoding(void *arg) /** Setup some SRVs in our SR state. If also_current is set, then set * both current and previous SRVs. * Helper of test_vote() and test_sr_compute_srv(). - * You must call sr_state_free() to free the state at the end of each test + * You must call sr_state_free_all() to free the state at the end of each test * function (on pass or fail). */ static void test_sr_setup_srv(int also_current) @@ -1299,7 +1299,7 @@ test_utils_auth(void *arg) #endif done: - sr_state_free(); + sr_state_free_all(); } static void