commit fbc455cbd224aaf28c613ed92bbaee656291efec Author: David Goulet <dgoulet@torproject.org> Date: Wed Jan 31 14:15:02 2018 -0500 ns: Add a before and after consensus has changed notification In 0.3.2.1-alpha, we've added notify_networkstatus_changed() in order to have a way to notify other subsystems that the consensus just changed. The old and new consensus are passed to it. Before this patch, this was done _before_ the new consensus was set globally (thus NOT accessible by getting the latest consensus). The scheduler notification was assuming that it was set and select_scheduler() is looking at the latest consensus to get the parameters it might needs. This was very wrong because at that point it is still the old consensus set globally. This commit changes the notify_networkstatus_changed() to be the "before" function and adds an "after" notification from which the scheduler subsystem is notified. Fixes #24975 --- changes/bug24975 | 6 ++++++ src/or/networkstatus.c | 24 +++++++++++++++++++----- src/or/scheduler.c | 5 ++--- src/or/scheduler.h | 8 +++----- src/or/scheduler_kist.c | 20 ++++++++------------ src/test/test_scheduler.c | 16 ++++++++-------- 6 files changed, 46 insertions(+), 33 deletions(-) diff --git a/changes/bug24975 b/changes/bug24975 new file mode 100644 index 000000000..32a5dfc92 --- /dev/null +++ b/changes/bug24975 @@ -0,0 +1,6 @@ + o Major bugfixes (scheduler, consensus): + - A logic in the code was preventing the scheduler subystem to properly + make a decision based on the latest consensus when it arrives. This lead + to the scheduler failing to notice any consensus parameters that might + have changed between consensuses. Fixes bug 24975; bugfix on + 0.3.2.1-alpha. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index e0a3e4cdc..9d9a2b8ad 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1564,13 +1564,20 @@ notify_control_networkstatus_changed(const networkstatus_t *old_c, smartlist_free(changed); } -/* Called when the consensus has changed from old_c to new_c. */ +/* Called before the consensus changes from old_c to new_c. */ static void -notify_networkstatus_changed(const networkstatus_t *old_c, - const networkstatus_t *new_c) +notify_before_networkstatus_changes(const networkstatus_t *old_c, + const networkstatus_t *new_c) { notify_control_networkstatus_changed(old_c, new_c); - scheduler_notify_networkstatus_changed(old_c, new_c); +} + +/* Called after a new consensus has been put in the global state. It is safe + * to use the consensus getters in this function. */ +static void +notify_after_networkstatus_changes(void) +{ + scheduler_notify_networkstatus_changed(); } /** Copy all the ancillary information (like router download status and so on) @@ -1897,8 +1904,11 @@ networkstatus_set_current_consensus(const char *consensus, const int is_usable_flavor = flav == usable_consensus_flavor(); + /* Before we switch to the new consensus, notify that we are about to change + * it using the old consensus and the new one. */ if (is_usable_flavor) { - notify_networkstatus_changed(networkstatus_get_latest_consensus(), c); + notify_before_networkstatus_changes(networkstatus_get_latest_consensus(), + c); } if (flav == FLAV_NS) { if (current_ns_consensus) { @@ -1941,6 +1951,10 @@ networkstatus_set_current_consensus(const char *consensus, } if (is_usable_flavor) { + /* Notify that we just changed the consensus so the current global value + * can be looked at. */ + notify_after_networkstatus_changes(); + /* The "current" consensus has just been set and it is a usable flavor so * the first thing we need to do is recalculate the voting schedule static * object so we can use the timings in there needed by some subsystems diff --git a/src/or/scheduler.c b/src/or/scheduler.c index cbf51447b..b68e48da0 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -435,15 +435,14 @@ scheduler_conf_changed(void) * Whenever we get a new consensus, this function is called. */ void -scheduler_notify_networkstatus_changed(const networkstatus_t *old_c, - const networkstatus_t *new_c) +scheduler_notify_networkstatus_changed(void) { /* Maybe the consensus param made us change the scheduler. */ set_scheduler(); /* Then tell the (possibly new) scheduler that we have a new consensus */ if (the_scheduler->on_new_consensus) { - the_scheduler->on_new_consensus(old_c, new_c); + the_scheduler->on_new_consensus(); } } diff --git a/src/or/scheduler.h b/src/or/scheduler.h index 47c98f096..559f1c8af 100644 --- a/src/or/scheduler.h +++ b/src/or/scheduler.h @@ -80,8 +80,7 @@ typedef struct scheduler_s { * (which might be new) will call this so it has the chance to react to the * new consensus too. If there's a consensus parameter that your scheduler * wants to keep an eye on, this is where you should check for it. */ - void (*on_new_consensus)(const networkstatus_t *old_c, - const networkstatus_t *new_c); + void (*on_new_consensus)(void); /* (Optional) To be called when a channel is being freed. Sometimes channels * go away (for example: the relay on the other end is shutting down). If @@ -119,8 +118,7 @@ typedef struct scheduler_s { void scheduler_init(void); void scheduler_free_all(void); void scheduler_conf_changed(void); -void scheduler_notify_networkstatus_changed(const networkstatus_t *old_c, - const networkstatus_t *new_c); +void scheduler_notify_networkstatus_changed(void); MOCK_DECL(void, scheduler_release_channel, (channel_t *chan)); /* @@ -197,7 +195,7 @@ int scheduler_can_use_kist(void); void scheduler_kist_set_full_mode(void); void scheduler_kist_set_lite_mode(void); scheduler_t *get_kist_scheduler(void); -int kist_scheduler_run_interval(const networkstatus_t *ns); +int kist_scheduler_run_interval(void); #ifdef TOR_UNIT_TESTS extern int32_t sched_run_interval; diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c index f50f3aa9e..877e561e0 100644 --- a/src/or/scheduler_kist.c +++ b/src/or/scheduler_kist.c @@ -362,10 +362,10 @@ outbuf_table_remove(outbuf_table_t *table, channel_t *chan) /* Set the scheduler running interval. */ static void -set_scheduler_run_interval(const networkstatus_t *ns) +set_scheduler_run_interval(void) { int old_sched_run_interval = sched_run_interval; - sched_run_interval = kist_scheduler_run_interval(ns); + sched_run_interval = kist_scheduler_run_interval(); if (old_sched_run_interval != sched_run_interval) { log_info(LD_SCHED, "Scheduler KIST changing its running interval " "from %" PRId32 " to %" PRId32, @@ -481,13 +481,9 @@ kist_on_channel_free(const channel_t *chan) /* Function of the scheduler interface: on_new_consensus() */ static void -kist_scheduler_on_new_consensus(const networkstatus_t *old_c, - const networkstatus_t *new_c) +kist_scheduler_on_new_consensus(void) { - (void) old_c; - (void) new_c; - - set_scheduler_run_interval(new_c); + set_scheduler_run_interval(); } /* Function of the scheduler interface: on_new_options() */ @@ -497,7 +493,7 @@ kist_scheduler_on_new_options(void) sock_buf_size_factor = get_options()->KISTSockBufSizeFactor; /* Calls kist_scheduler_run_interval which calls get_options(). */ - set_scheduler_run_interval(NULL); + set_scheduler_run_interval(); } /* Function of the scheduler interface: init() */ @@ -770,7 +766,7 @@ get_kist_scheduler(void) * - If consensus doesn't say anything, return 10 milliseconds, default. */ int -kist_scheduler_run_interval(const networkstatus_t *ns) +kist_scheduler_run_interval(void) { int run_interval = get_options()->KISTSchedRunInterval; @@ -784,7 +780,7 @@ kist_scheduler_run_interval(const networkstatus_t *ns) /* Will either be the consensus value or the default. Note that 0 can be * returned which means the consensus wants us to NOT use KIST. */ - return networkstatus_get_param(ns, "KISTSchedRunInterval", + return networkstatus_get_param(NULL, "KISTSchedRunInterval", KIST_SCHED_RUN_INTERVAL_DEFAULT, KIST_SCHED_RUN_INTERVAL_MIN, KIST_SCHED_RUN_INTERVAL_MAX); @@ -823,7 +819,7 @@ scheduler_can_use_kist(void) /* We do have the support, time to check if we can get the interval that the * consensus can be disabling. */ - int run_interval = kist_scheduler_run_interval(NULL); + int run_interval = kist_scheduler_run_interval(); log_debug(LD_SCHED, "Determined KIST sched_run_interval should be " "%" PRId32 ". Can%s use KIST.", run_interval, (run_interval > 0 ? "" : " not")); diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index 18f989514..1f014c4f6 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -959,7 +959,7 @@ test_scheduler_can_use_kist(void *arg) clear_options(); mocked_options.KISTSchedRunInterval = 1234; res_should = scheduler_can_use_kist(); - res_freq = kist_scheduler_run_interval(NULL); + res_freq = kist_scheduler_run_interval(); #ifdef HAVE_KIST_SUPPORT tt_int_op(res_should, ==, 1); #else /* HAVE_KIST_SUPPORT */ @@ -971,7 +971,7 @@ test_scheduler_can_use_kist(void *arg) clear_options(); mocked_options.KISTSchedRunInterval = 0; res_should = scheduler_can_use_kist(); - res_freq = kist_scheduler_run_interval(NULL); + res_freq = kist_scheduler_run_interval(); #ifdef HAVE_KIST_SUPPORT tt_int_op(res_should, ==, 1); #else /* HAVE_KIST_SUPPORT */ @@ -984,7 +984,7 @@ test_scheduler_can_use_kist(void *arg) clear_options(); mocked_options.KISTSchedRunInterval = 0; res_should = scheduler_can_use_kist(); - res_freq = kist_scheduler_run_interval(NULL); + res_freq = kist_scheduler_run_interval(); #ifdef HAVE_KIST_SUPPORT tt_int_op(res_should, ==, 1); #else /* HAVE_KIST_SUPPORT */ @@ -998,7 +998,7 @@ test_scheduler_can_use_kist(void *arg) clear_options(); mocked_options.KISTSchedRunInterval = 0; res_should = scheduler_can_use_kist(); - res_freq = kist_scheduler_run_interval(NULL); + res_freq = kist_scheduler_run_interval(); tt_int_op(res_should, ==, 0); tt_int_op(res_freq, ==, 0); UNMOCK(networkstatus_get_param); @@ -1032,7 +1032,7 @@ test_scheduler_ns_changed(void *arg) /* Change from vanilla to kist via consensus */ the_scheduler = get_vanilla_scheduler(); MOCK(networkstatus_get_param, mock_kist_networkstatus_get_param); - scheduler_notify_networkstatus_changed(NULL, NULL); + scheduler_notify_networkstatus_changed(); UNMOCK(networkstatus_get_param); #ifdef HAVE_KIST_SUPPORT tt_ptr_op(the_scheduler, ==, get_kist_scheduler()); @@ -1043,14 +1043,14 @@ test_scheduler_ns_changed(void *arg) /* Change from kist to vanilla via consensus */ the_scheduler = get_kist_scheduler(); MOCK(networkstatus_get_param, mock_vanilla_networkstatus_get_param); - scheduler_notify_networkstatus_changed(NULL, NULL); + scheduler_notify_networkstatus_changed(); UNMOCK(networkstatus_get_param); tt_ptr_op(the_scheduler, ==, get_vanilla_scheduler()); /* Doesn't change when using KIST */ the_scheduler = get_kist_scheduler(); MOCK(networkstatus_get_param, mock_kist_networkstatus_get_param); - scheduler_notify_networkstatus_changed(NULL, NULL); + scheduler_notify_networkstatus_changed(); UNMOCK(networkstatus_get_param); #ifdef HAVE_KIST_SUPPORT tt_ptr_op(the_scheduler, ==, get_kist_scheduler()); @@ -1061,7 +1061,7 @@ test_scheduler_ns_changed(void *arg) /* Doesn't change when using vanilla */ the_scheduler = get_vanilla_scheduler(); MOCK(networkstatus_get_param, mock_vanilla_networkstatus_get_param); - scheduler_notify_networkstatus_changed(NULL, NULL); + scheduler_notify_networkstatus_changed(); UNMOCK(networkstatus_get_param); tt_ptr_op(the_scheduler, ==, get_vanilla_scheduler());