From 269cd5dba705e0a8ce30f473128779f34caf4da6 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 16 Apr 2018 15:26:49 -0400 Subject: [PATCH 1/8] main: Sort alphabetically periodic event callbacks No behavior change, just to make it easier to find callbacks and for the sake of our human brain to parse the list properly. Signed-off-by: David Goulet --- src/or/main.c | 96 +++++++++++++++++++++++++-------------------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index a852d3273d..5d64eb69be 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1316,34 +1316,34 @@ static int periodic_events_initialized = 0; #undef CALLBACK #define CALLBACK(name) \ static int name ## _callback(time_t, const or_options_t *) -CALLBACK(rotate_onion_key); -CALLBACK(check_onion_keys_expiry_time); -CALLBACK(check_ed_keys); -CALLBACK(launch_descriptor_fetches); -CALLBACK(rotate_x509_certificate); CALLBACK(add_entropy); -CALLBACK(launch_reachability_tests); -CALLBACK(downrate_stability); -CALLBACK(save_stability); CALLBACK(check_authority_cert); -CALLBACK(check_expired_networkstatus); -CALLBACK(write_stats_file); -CALLBACK(record_bridge_stats); -CALLBACK(clean_caches); -CALLBACK(rend_cache_failure_clean); -CALLBACK(retry_dns); -CALLBACK(check_descriptor); -CALLBACK(check_for_reachability_bw); -CALLBACK(fetch_networkstatus); -CALLBACK(retry_listeners); -CALLBACK(expire_old_ciruits_serverside); -CALLBACK(check_dns_honesty); -CALLBACK(write_bridge_ns); -CALLBACK(heartbeat); -CALLBACK(clean_consdiffmgr); -CALLBACK(reset_padding_counts); CALLBACK(check_canonical_channels); +CALLBACK(check_descriptor); +CALLBACK(check_dns_honesty); +CALLBACK(check_ed_keys); +CALLBACK(check_expired_networkstatus); +CALLBACK(check_for_reachability_bw); +CALLBACK(check_onion_keys_expiry_time); +CALLBACK(clean_caches); +CALLBACK(clean_consdiffmgr); +CALLBACK(downrate_stability); +CALLBACK(expire_old_ciruits_serverside); +CALLBACK(fetch_networkstatus); +CALLBACK(heartbeat); CALLBACK(hs_service); +CALLBACK(launch_descriptor_fetches); +CALLBACK(launch_reachability_tests); +CALLBACK(record_bridge_stats); +CALLBACK(rend_cache_failure_clean); +CALLBACK(reset_padding_counts); +CALLBACK(retry_dns); +CALLBACK(retry_listeners); +CALLBACK(rotate_onion_key); +CALLBACK(rotate_x509_certificate); +CALLBACK(save_stability); +CALLBACK(write_bridge_ns); +CALLBACK(write_stats_file); #undef CALLBACK @@ -1351,34 +1351,34 @@ CALLBACK(hs_service); #define CALLBACK(name) PERIODIC_EVENT(name) static periodic_event_item_t periodic_events[] = { - CALLBACK(rotate_onion_key), - CALLBACK(check_onion_keys_expiry_time), - CALLBACK(check_ed_keys), - CALLBACK(launch_descriptor_fetches), - CALLBACK(rotate_x509_certificate), CALLBACK(add_entropy), - CALLBACK(launch_reachability_tests), - CALLBACK(downrate_stability), - CALLBACK(save_stability), CALLBACK(check_authority_cert), - CALLBACK(check_expired_networkstatus), - CALLBACK(write_stats_file), - CALLBACK(record_bridge_stats), - CALLBACK(clean_caches), - CALLBACK(rend_cache_failure_clean), - CALLBACK(retry_dns), - CALLBACK(check_descriptor), - CALLBACK(check_for_reachability_bw), - CALLBACK(fetch_networkstatus), - CALLBACK(retry_listeners), - CALLBACK(expire_old_ciruits_serverside), - CALLBACK(check_dns_honesty), - CALLBACK(write_bridge_ns), - CALLBACK(heartbeat), - CALLBACK(clean_consdiffmgr), - CALLBACK(reset_padding_counts), CALLBACK(check_canonical_channels), + CALLBACK(check_descriptor), + CALLBACK(check_dns_honesty), + CALLBACK(check_ed_keys), + CALLBACK(check_expired_networkstatus), + CALLBACK(check_for_reachability_bw), + CALLBACK(check_onion_keys_expiry_time), + CALLBACK(clean_caches), + CALLBACK(clean_consdiffmgr), + CALLBACK(downrate_stability), + CALLBACK(expire_old_ciruits_serverside), + CALLBACK(fetch_networkstatus), + CALLBACK(heartbeat), CALLBACK(hs_service), + CALLBACK(launch_descriptor_fetches), + CALLBACK(launch_reachability_tests), + CALLBACK(record_bridge_stats), + CALLBACK(rend_cache_failure_clean), + CALLBACK(reset_padding_counts), + CALLBACK(retry_dns), + CALLBACK(retry_listeners), + CALLBACK(rotate_onion_key), + CALLBACK(rotate_x509_certificate), + CALLBACK(save_stability), + CALLBACK(write_bridge_ns), + CALLBACK(write_stats_file), END_OF_PERIODIC_EVENTS }; #undef CALLBACK From ed89bb32535fbf354b406a36f3176380a4e226bf Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 16 Apr 2018 15:50:50 -0400 Subject: [PATCH 2/8] main: Specialize the periodic events on a per-role basis In tor, we have a series of possible "roles" that the tor daemon can be enabled for. They are: Client, Bridge, Relay, Authority (directory or bridge) and Onion service. They can be combined sometimes. For instance, a Directory Authority is also a Relay. This adds a "roles" field to a periodic event item object which is used to know for which roles the event is for. The next step is to enable the event only if the roles apply. No behavior change at this commit. Pars of #25762 Signed-off-by: David Goulet --- src/or/main.c | 75 +++++++++++++++++++++++++++++------------------ src/or/periodic.h | 30 +++++++++++++++++-- 2 files changed, 74 insertions(+), 31 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 5d64eb69be..5ca6277c15 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1348,37 +1348,54 @@ CALLBACK(write_stats_file); #undef CALLBACK /* Now we declare an array of periodic_event_item_t for each periodic event */ -#define CALLBACK(name) PERIODIC_EVENT(name) +#define CALLBACK(name, r) PERIODIC_EVENT(name, r) static periodic_event_item_t periodic_events[] = { - CALLBACK(add_entropy), - CALLBACK(check_authority_cert), - CALLBACK(check_canonical_channels), - CALLBACK(check_descriptor), - CALLBACK(check_dns_honesty), - CALLBACK(check_ed_keys), - CALLBACK(check_expired_networkstatus), - CALLBACK(check_for_reachability_bw), - CALLBACK(check_onion_keys_expiry_time), - CALLBACK(clean_caches), - CALLBACK(clean_consdiffmgr), - CALLBACK(downrate_stability), - CALLBACK(expire_old_ciruits_serverside), - CALLBACK(fetch_networkstatus), - CALLBACK(heartbeat), - CALLBACK(hs_service), - CALLBACK(launch_descriptor_fetches), - CALLBACK(launch_reachability_tests), - CALLBACK(record_bridge_stats), - CALLBACK(rend_cache_failure_clean), - CALLBACK(reset_padding_counts), - CALLBACK(retry_dns), - CALLBACK(retry_listeners), - CALLBACK(rotate_onion_key), - CALLBACK(rotate_x509_certificate), - CALLBACK(save_stability), - CALLBACK(write_bridge_ns), - CALLBACK(write_stats_file), + /* Everyone needs to run those. */ + CALLBACK(add_entropy, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(check_expired_networkstatus, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(clean_caches, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(fetch_networkstatus, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(heartbeat, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(launch_descriptor_fetches, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(reset_padding_counts, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(retry_listeners, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(rotate_x509_certificate, PERIODIC_EVENT_ROLE_ALL), + CALLBACK(write_stats_file, PERIODIC_EVENT_ROLE_ALL), + + /* Routers (bridge and relay) only. */ + CALLBACK(check_descriptor, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(check_ed_keys, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(check_for_reachability_bw, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(check_onion_keys_expiry_time, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(clean_consdiffmgr, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(expire_old_ciruits_serverside, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(retry_dns, PERIODIC_EVENT_ROLE_ROUTER), + CALLBACK(rotate_onion_key, PERIODIC_EVENT_ROLE_ROUTER), + + /* Authorities (bridge and directory) only. */ + CALLBACK(downrate_stability, PERIODIC_EVENT_ROLE_AUTHORITIES), + CALLBACK(launch_reachability_tests, PERIODIC_EVENT_ROLE_AUTHORITIES), + CALLBACK(save_stability, PERIODIC_EVENT_ROLE_AUTHORITIES), + + /* Directory authority only. */ + CALLBACK(check_authority_cert, PERIODIC_EVENT_ROLE_DIRAUTH), + + /* Relay only. */ + CALLBACK(check_canonical_channels, PERIODIC_EVENT_ROLE_RELAY), + CALLBACK(check_dns_honesty, PERIODIC_EVENT_ROLE_RELAY), + + /* Hidden Service service only. */ + CALLBACK(hs_service, PERIODIC_EVENT_ROLE_HS_SERVICE), + + /* Bridge only. */ + CALLBACK(record_bridge_stats, PERIODIC_EVENT_ROLE_BRIDGE), + + /* Client only. */ + CALLBACK(rend_cache_failure_clean, PERIODIC_EVENT_ROLE_CLIENT), + + /* Bridge Authority only. */ + CALLBACK(write_bridge_ns, PERIODIC_EVENT_ROLE_BRIDGEAUTH), END_OF_PERIODIC_EVENTS }; #undef CALLBACK diff --git a/src/or/periodic.h b/src/or/periodic.h index 285400b8bb..4544ae2763 100644 --- a/src/or/periodic.h +++ b/src/or/periodic.h @@ -6,6 +6,29 @@ #define PERIODIC_EVENT_NO_UPDATE (-1) +/* Tor roles for which a periodic event item is for. An event can be for + * multiple roles, they can be combined. */ +#define PERIODIC_EVENT_ROLE_CLIENT (1U << 0) +#define PERIODIC_EVENT_ROLE_RELAY (1U << 1) +#define PERIODIC_EVENT_ROLE_BRIDGE (1U << 2) +#define PERIODIC_EVENT_ROLE_DIRAUTH (1U << 3) +#define PERIODIC_EVENT_ROLE_BRIDGEAUTH (1U << 4) +#define PERIODIC_EVENT_ROLE_HS_SERVICE (1U << 5) + +/* Helper macro to make it a bit less annoying to defined groups of roles that + * are often used. */ + +/* Router that is a Bridge or Relay. */ +#define PERIODIC_EVENT_ROLE_ROUTER \ + (PERIODIC_EVENT_ROLE_BRIDGE | PERIODIC_EVENT_ROLE_RELAY) +/* Authorities that is both bridge and directory. */ +#define PERIODIC_EVENT_ROLE_AUTHORITIES \ + (PERIODIC_EVENT_ROLE_BRIDGEAUTH | PERIODIC_EVENT_ROLE_DIRAUTH) +/* All roles. */ +#define PERIODIC_EVENT_ROLE_ALL \ + (PERIODIC_EVENT_ROLE_AUTHORITIES | PERIODIC_EVENT_ROLE_CLIENT | \ + PERIODIC_EVENT_ROLE_HS_SERVICE | PERIODIC_EVENT_ROLE_ROUTER) + /** Callback function for a periodic event to take action. The return value * influences the next time the function will get called. Return * PERIODIC_EVENT_NO_UPDATE to not update last_action_time and be polled @@ -23,11 +46,14 @@ typedef struct periodic_event_item_t { struct mainloop_event_t *ev; /**< Libevent callback we're using to implement * this */ const char *name; /**< Name of the function -- for debug */ + + /* Bitmask of roles define above for which this event applies. */ + uint32_t roles; } periodic_event_item_t; /** events will get their interval from first execution */ -#define PERIODIC_EVENT(fn) { fn##_callback, 0, NULL, #fn } -#define END_OF_PERIODIC_EVENTS { NULL, 0, NULL, NULL } +#define PERIODIC_EVENT(fn, r) { fn##_callback, 0, NULL, #fn, r } +#define END_OF_PERIODIC_EVENTS { NULL, 0, NULL, NULL, 0 } void periodic_event_launch(periodic_event_item_t *event); void periodic_event_setup(periodic_event_item_t *event); From a4fcdc5decfe60bbd95aee2e5586e90c40b73225 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 17 Apr 2018 09:31:50 -0400 Subject: [PATCH 3/8] main: Launch periodic events by roles Signed-off-by: David Goulet --- src/or/main.c | 40 +++++++++++++++++++++++++++++++++++++--- src/or/networkstatus.c | 2 +- src/or/networkstatus.h | 2 ++ src/or/periodic.c | 12 +++++++++++- src/or/periodic.h | 14 ++++++++++++-- 5 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 5ca6277c15..52e83822ef 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1437,6 +1437,32 @@ find_periodic_event(const char *name) return NULL; } +/** Return a bitmask of the roles this tor instance is configured for using + * the given options. */ +static int +get_my_roles(const or_options_t *options) +{ + tor_assert(options); + + int roles = 0; + int is_bridge = options->BridgeRelay; + int is_client = any_client_port_set(options); + int is_relay = server_mode(options); + int is_dirauth = authdir_mode_v3(options); + int is_bridgeauth = authdir_mode_bridge(options); + int is_hidden_service = !!hs_service_get_num_services() || + !!rend_num_services(); + + if (is_bridge) roles |= PERIODIC_EVENT_ROLE_BRIDGE; + if (is_client) roles |= PERIODIC_EVENT_ROLE_CLIENT; + if (is_relay) roles |= PERIODIC_EVENT_ROLE_RELAY; + if (is_dirauth) roles |= PERIODIC_EVENT_ROLE_DIRAUTH; + if (is_bridgeauth) roles |= PERIODIC_EVENT_ROLE_BRIDGEAUTH; + if (is_hidden_service) roles |= PERIODIC_EVENT_ROLE_HS_SERVICE; + + return roles; +} + /** Event to run initialize_periodic_events_cb */ static struct event *initialize_periodic_events_event = NULL; @@ -1451,10 +1477,17 @@ initialize_periodic_events_cb(evutil_socket_t fd, short events, void *data) (void) fd; (void) events; (void) data; + + int roles = get_my_roles(get_options()); + tor_event_free(initialize_periodic_events_event); - int i; - for (i = 0; periodic_events[i].name; ++i) { - periodic_event_launch(&periodic_events[i]); + + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + if (item->roles & roles && !periodic_event_is_enabled(item)) { + periodic_event_launch(item); + log_debug(LD_GENERAL, "Launching periodic event %s", item->name); + } } } @@ -1466,6 +1499,7 @@ initialize_periodic_events(void) tor_assert(periodic_events_initialized == 0); periodic_events_initialized = 1; + /* Set up all periodic events. We'll launch them by roles. */ int i; for (i = 0; periodic_events[i].name; ++i) { periodic_event_setup(&periodic_events[i]); diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 5f19792c7d..b0db0cecbc 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1680,7 +1680,7 @@ networkstatus_set_current_consensus_from_ns(networkstatus_t *c, * XXXX If we need this elsewhere at any point, we should make it nonstatic * XXXX and move it into another file. */ -static int +int any_client_port_set(const or_options_t *options) { return (options->SocksPort_set || diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 1851a55e82..04cf277d2f 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -140,6 +140,8 @@ void vote_routerstatus_free_(vote_routerstatus_t *rs); #define vote_routerstatus_free(rs) \ FREE_AND_NULL(vote_routerstatus_t, vote_routerstatus_free_, (rs)) +int any_client_port_set(const or_options_t *options); + #ifdef NETWORKSTATUS_PRIVATE #ifdef TOR_UNIT_TESTS STATIC int networkstatus_set_current_consensus_from_ns(networkstatus_t *c, diff --git a/src/or/periodic.c b/src/or/periodic.c index fa40965de1..42bea3ae65 100644 --- a/src/or/periodic.c +++ b/src/or/periodic.c @@ -78,7 +78,10 @@ periodic_event_dispatch(mainloop_event_t *ev, void *data) void periodic_event_reschedule(periodic_event_item_t *event) { - periodic_event_set_interval(event, 1); + /* Don't reschedule a disabled event. */ + if (periodic_event_is_enabled(event)) { + periodic_event_set_interval(event, 1); + } } /** Initializes the libevent backend for a periodic event. */ @@ -104,9 +107,15 @@ periodic_event_launch(periodic_event_item_t *event) log_err(LD_BUG, "periodic_event_launch without periodic_event_setup"); tor_assert(0); } + /* Event already enabled? This is a bug */ + if (periodic_event_is_enabled(event)) { + log_err(LD_BUG, "periodic_event_launch on an already enabled event"); + tor_assert(0); + } // Initial dispatch periodic_event_dispatch(event->ev, event); + event->enabled = 1; } /** Release all storage associated with event */ @@ -117,5 +126,6 @@ periodic_event_destroy(periodic_event_item_t *event) return; mainloop_event_free(event->ev); event->last_action_time = 0; + event->enabled = 0; } diff --git a/src/or/periodic.h b/src/or/periodic.h index 4544ae2763..1b346a1cc8 100644 --- a/src/or/periodic.h +++ b/src/or/periodic.h @@ -49,11 +49,21 @@ typedef struct periodic_event_item_t { /* Bitmask of roles define above for which this event applies. */ uint32_t roles; + /* Indicate that this event has been enabled that is scheduled. */ + unsigned int enabled : 1; } periodic_event_item_t; /** events will get their interval from first execution */ -#define PERIODIC_EVENT(fn, r) { fn##_callback, 0, NULL, #fn, r } -#define END_OF_PERIODIC_EVENTS { NULL, 0, NULL, NULL, 0 } +#define PERIODIC_EVENT(fn, r) { fn##_callback, 0, NULL, #fn, r, 0 } +#define END_OF_PERIODIC_EVENTS { NULL, 0, NULL, NULL, 0, 0 } + +/* Return true iff the given event was setup before thus is enabled to be + * scheduled. */ +static inline int +periodic_event_is_enabled(const periodic_event_item_t *item) +{ + return item->enabled; +} void periodic_event_launch(periodic_event_item_t *event); void periodic_event_setup(periodic_event_item_t *event); From 1d864987cbdab4286eed8f3a86a488759dc322ae Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 17 Apr 2018 09:44:01 -0400 Subject: [PATCH 4/8] config: Set up periodic events when options changes In case we transitionned to a new role in Tor, we need to launch and/or destroy some periodic events. Signed-off-by: David Goulet --- src/or/config.c | 7 ++++++- src/or/main.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/or/main.h | 2 ++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/or/config.c b/src/or/config.c index 9c0b321b56..1923afe7d0 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -903,8 +903,13 @@ set_options(or_options_t *new_val, char **msg) smartlist_free(elements); } - if (old_options != global_options) + if (old_options != global_options) { or_options_free(old_options); + /* If we are here it means we've successfully applied the new options and + * that the global options have been changed to the new values. We'll + * check if we need to remove or add periodic events. */ + periodic_events_on_new_options(global_options); + } return 0; } diff --git a/src/or/main.c b/src/or/main.c index 52e83822ef..eb8835198a 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1530,6 +1530,47 @@ teardown_periodic_events(void) periodic_events_initialized = 0; } +/** Do a pass at all our periodic events, destroy those we don't need anymore + * and enabled those we need now using the given options. */ +static void +rescan_periodic_events(const or_options_t *options) +{ + tor_assert(options); + + int roles = get_my_roles(options); + + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + int is_enabled = periodic_event_is_enabled(item); + int need_item = (item->roles & roles); + + /* We need this event but it is *not* enabled. */ + if (need_item && !is_enabled) { + periodic_event_launch(item); + continue; + } + /* We do *not* need this event but it is enabled. */ + if (!need_item && is_enabled) { + periodic_event_destroy(item); + continue; + } + } +} + +/* We just got new options globally set, see if we need to destroy or setup + * periodic events. */ +void +periodic_events_on_new_options(const or_options_t *options) +{ + /* Only if we've already initialized once the events, teardown them all and + * reinitialize. It is just simpler that way instead of going through all + * currently enabled events and trying to destroy only the one that could be + * affected. */ + if (periodic_events_initialized) { + rescan_periodic_events(options); + } +} + /** * Update our schedule so that we'll check whether we need to update our * descriptor immediately, rather than after up to CHECK_DESCRIPTOR_INTERVAL diff --git a/src/or/main.h b/src/or/main.h index e50d14d4d9..5220b13216 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -86,6 +86,8 @@ uint64_t get_main_loop_success_count(void); uint64_t get_main_loop_error_count(void); uint64_t get_main_loop_idle_count(void); +void periodic_events_on_new_options(const or_options_t *options); + extern time_t time_of_process_start; extern int quiet_level; extern token_bucket_rw_t global_bucket; From 4e85f17eec06fc963c33cc3f637876e7f1627a57 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 17 Apr 2018 09:57:09 -0400 Subject: [PATCH 5/8] periodic: Add an enable and disable function Two helper functions to enable an event and disable an event which wraps the launch and destroy of an event but takes care of the enabled flag. They are also idempotent that is can be called multiple time on the same event without effect if the event was already enabled or disabled. Signed-off-by: David Goulet --- src/or/main.c | 22 +++++++++------------- src/or/periodic.c | 41 ++++++++++++++++++++++++++++++++++++++++- src/or/periodic.h | 2 ++ 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index eb8835198a..1c5ac198b0 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1484,8 +1484,9 @@ initialize_periodic_events_cb(evutil_socket_t fd, short events, void *data) for (int i = 0; periodic_events[i].name; ++i) { periodic_event_item_t *item = &periodic_events[i]; - if (item->roles & roles && !periodic_event_is_enabled(item)) { - periodic_event_launch(item); + if (item->roles & roles) { + /* This is safe to be called on an already enabled event. */ + periodic_event_enable(item); log_debug(LD_GENERAL, "Launching periodic event %s", item->name); } } @@ -1541,18 +1542,13 @@ rescan_periodic_events(const or_options_t *options) for (int i = 0; periodic_events[i].name; ++i) { periodic_event_item_t *item = &periodic_events[i]; - int is_enabled = periodic_event_is_enabled(item); - int need_item = (item->roles & roles); - /* We need this event but it is *not* enabled. */ - if (need_item && !is_enabled) { - periodic_event_launch(item); - continue; - } - /* We do *not* need this event but it is enabled. */ - if (!need_item && is_enabled) { - periodic_event_destroy(item); - continue; + /* Enable the event if needed. It is safe to enable an event that was + * already enabled. Same goes for disabling it. */ + if (item->roles & roles) { + periodic_event_enable(item); + } else { + periodic_event_disable(item); } } } diff --git a/src/or/periodic.c b/src/or/periodic.c index 42bea3ae65..76aa418b35 100644 --- a/src/or/periodic.c +++ b/src/or/periodic.c @@ -43,12 +43,22 @@ periodic_event_dispatch(mainloop_event_t *ev, void *data) periodic_event_item_t *event = data; tor_assert(ev == event->ev); + if (BUG(!periodic_event_is_enabled(event))) { + return; + } + time_t now = time(NULL); const or_options_t *options = get_options(); // log_debug(LD_GENERAL, "Dispatching %s", event->name); int r = event->fn(now, options); int next_interval = 0; + if (!periodic_event_is_enabled(event)) { + /* The event got disabled from inside its callback; no need to + * reschedule. */ + return; + } + /* update the last run time if action was taken */ if (r==0) { log_err(LD_BUG, "Invalid return value for periodic event from %s.", @@ -114,8 +124,8 @@ periodic_event_launch(periodic_event_item_t *event) } // Initial dispatch - periodic_event_dispatch(event->ev, event); event->enabled = 1; + periodic_event_dispatch(event->ev, event); } /** Release all storage associated with event */ @@ -126,6 +136,35 @@ periodic_event_destroy(periodic_event_item_t *event) return; mainloop_event_free(event->ev); event->last_action_time = 0; +} + +/** Enable the given event which means the event is launched and then the + * event's enabled flag is set. This can be called for an event that is + * already enabled. */ +void +periodic_event_enable(periodic_event_item_t *event) +{ + tor_assert(event); + /* Safely and silently ignore if this event is already enabled. */ + if (periodic_event_is_enabled(event)) { + return; + } + + periodic_event_launch(event); +} + +/** Disable the given event which means the event is destroyed and then the + * event's enabled flag is unset. This can be called for an event that is + * already disabled. */ +void +periodic_event_disable(periodic_event_item_t *event) +{ + tor_assert(event); + /* Safely and silently ignore if this event is already disabled. */ + if (!periodic_event_is_enabled(event)) { + return; + } + mainloop_event_cancel(event->ev); event->enabled = 0; } diff --git a/src/or/periodic.h b/src/or/periodic.h index 1b346a1cc8..dde27db5af 100644 --- a/src/or/periodic.h +++ b/src/or/periodic.h @@ -69,6 +69,8 @@ void periodic_event_launch(periodic_event_item_t *event); void periodic_event_setup(periodic_event_item_t *event); void periodic_event_destroy(periodic_event_item_t *event); void periodic_event_reschedule(periodic_event_item_t *event); +void periodic_event_enable(periodic_event_item_t *event); +void periodic_event_disable(periodic_event_item_t *event); #endif /* !defined(TOR_PERIODIC_H) */ From f70fa67da6d22d7351bd8f50db0804c239ffa5d6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Apr 2018 18:55:28 -0400 Subject: [PATCH 6/8] main: Use rescan_periodic_events in initialize_periodic_events_cb --- src/or/main.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 1c5ac198b0..565131cb32 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -150,6 +150,7 @@ static int run_main_loop_until_done(void); static void process_signal(int sig); static void shutdown_did_not_work_callback(evutil_socket_t fd, short event, void *arg) ATTR_NORETURN; +static void rescan_periodic_events(const or_options_t *options); /********* START VARIABLES **********/ @@ -1478,18 +1479,9 @@ initialize_periodic_events_cb(evutil_socket_t fd, short events, void *data) (void) events; (void) data; - int roles = get_my_roles(get_options()); - tor_event_free(initialize_periodic_events_event); - for (int i = 0; periodic_events[i].name; ++i) { - periodic_event_item_t *item = &periodic_events[i]; - if (item->roles & roles) { - /* This is safe to be called on an already enabled event. */ - periodic_event_enable(item); - log_debug(LD_GENERAL, "Launching periodic event %s", item->name); - } - } + rescan_periodic_events(get_options()); } /** Set up all the members of periodic_events[], and configure them all to be @@ -1546,8 +1538,10 @@ rescan_periodic_events(const or_options_t *options) /* Enable the event if needed. It is safe to enable an event that was * already enabled. Same goes for disabling it. */ if (item->roles & roles) { + log_debug(LD_GENERAL, "Launching periodic event %s", item->name); periodic_event_enable(item); } else { + log_debug(LD_GENERAL, "Disabling periodic event %s", item->name); periodic_event_disable(item); } } From 87cb9ce90088bbe3e677804eca7c1ffa4f63a1cb Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 18 Apr 2018 10:25:39 -0400 Subject: [PATCH 7/8] main: Update periodic events comment based on latest code Signed-off-by: David Goulet --- src/or/main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 565131cb32..0a796c0fae 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1523,8 +1523,8 @@ teardown_periodic_events(void) periodic_events_initialized = 0; } -/** Do a pass at all our periodic events, destroy those we don't need anymore - * and enabled those we need now using the given options. */ +/** Do a pass at all our periodic events, disable those we don't need anymore + * and enable those we need now using the given options. */ static void rescan_periodic_events(const or_options_t *options) { @@ -1547,15 +1547,15 @@ rescan_periodic_events(const or_options_t *options) } } -/* We just got new options globally set, see if we need to destroy or setup +/* We just got new options globally set, see if we need to enabled or disable * periodic events. */ void periodic_events_on_new_options(const or_options_t *options) { - /* Only if we've already initialized once the events, teardown them all and - * reinitialize. It is just simpler that way instead of going through all - * currently enabled events and trying to destroy only the one that could be - * affected. */ + /* Only if we've already initialized the events, rescan the list which will + * enable or disable events depending on our roles. This will be called at + * bootup and we don't want this function to initialize the events because + * they aren't set up at this stage. */ if (periodic_events_initialized) { rescan_periodic_events(options); } From 665e23c59a370157c5e3526e29590798b1933ed5 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 18 Apr 2018 14:50:07 -0400 Subject: [PATCH 8/8] test: Add periodic events unit tests Signed-off-by: David Goulet --- src/or/main.c | 4 +- src/or/main.h | 5 + src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_periodic_event.c | 256 +++++++++++++++++++++++++++++++++ 6 files changed, 266 insertions(+), 2 deletions(-) create mode 100644 src/test/test_periodic_event.c diff --git a/src/or/main.c b/src/or/main.c index 0a796c0fae..595246bfcd 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1351,7 +1351,7 @@ CALLBACK(write_stats_file); /* Now we declare an array of periodic_event_item_t for each periodic event */ #define CALLBACK(name, r) PERIODIC_EVENT(name, r) -static periodic_event_item_t periodic_events[] = { +STATIC periodic_event_item_t periodic_events[] = { /* Everyone needs to run those. */ CALLBACK(add_entropy, PERIODIC_EVENT_ROLE_ALL), CALLBACK(check_expired_networkstatus, PERIODIC_EVENT_ROLE_ALL), @@ -1440,7 +1440,7 @@ find_periodic_event(const char *name) /** Return a bitmask of the roles this tor instance is configured for using * the given options. */ -static int +STATIC int get_my_roles(const or_options_t *options) { tor_assert(options); diff --git a/src/or/main.h b/src/or/main.h index 5220b13216..57ac8573a6 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -98,8 +98,13 @@ STATIC void init_connection_lists(void); STATIC void close_closeable_connections(void); STATIC void initialize_periodic_events(void); STATIC void teardown_periodic_events(void); +STATIC int get_my_roles(const or_options_t *options); #ifdef TOR_UNIT_TESTS extern smartlist_t *connection_array; + +/* We need the periodic_event_item_t definition. */ +#include "periodic.h" +extern periodic_event_item_t periodic_events[]; #endif #endif /* defined(MAIN_PRIVATE) */ diff --git a/src/test/include.am b/src/test/include.am index 474da3f880..b59517a135 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -144,6 +144,7 @@ src_test_test_SOURCES = \ src/test/test_oom.c \ src/test/test_oos.c \ src/test/test_options.c \ + src/test/test_periodic_event.c \ src/test/test_policy.c \ src/test/test_procmon.c \ src/test/test_proto_http.c \ diff --git a/src/test/test.c b/src/test/test.c index 422e181b94..2963f169cf 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -863,6 +863,7 @@ struct testgroup_t testgroups[] = { { "oom/", oom_tests }, { "oos/", oos_tests }, { "options/", options_tests }, + { "periodic-event/" , periodic_event_tests }, { "policy/" , policy_tests }, { "procmon/", procmon_tests }, { "proto/http/", proto_http_tests }, diff --git a/src/test/test.h b/src/test/test.h index 1728831ed0..15965ccaf4 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -239,6 +239,7 @@ extern struct testcase_t nodelist_tests[]; extern struct testcase_t oom_tests[]; extern struct testcase_t oos_tests[]; extern struct testcase_t options_tests[]; +extern struct testcase_t periodic_event_tests[]; extern struct testcase_t policy_tests[]; extern struct testcase_t procmon_tests[]; extern struct testcase_t proto_http_tests[]; diff --git a/src/test/test_periodic_event.c b/src/test/test_periodic_event.c new file mode 100644 index 0000000000..1a9f4351ea --- /dev/null +++ b/src/test/test_periodic_event.c @@ -0,0 +1,256 @@ +/* Copyright (c) 2018, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file test_periodic_event.c + * \brief Test the periodic events that Tor uses for different roles. They are + * part of the libevent mainloop + */ + +#define CONFIG_PRIVATE +#define HS_SERVICE_PRIVATE +#define MAIN_PRIVATE + +#include "test.h" +#include "test_helpers.h" + +#include "or.h" +#include "config.h" +#include "hs_service.h" +#include "main.h" +#include "periodic.h" + +/** Helper function: This is replaced in some tests for the event callbacks so + * we don't actually go into the code path of those callbacks. */ +static int +dumb_event_fn(time_t now, const or_options_t *options) +{ + (void) now; + (void) options; + + /* Will get rescheduled in 300 seconds. It just can't be 0. */ + return 300; +} + +static void +register_dummy_hidden_service(hs_service_t *service) +{ + memset(service, 0, sizeof(hs_service_t)); + memset(&service->keys.identity_pk, 'A', sizeof(service->keys.identity_pk)); + (void) register_service(get_hs_service_map(), service); +} + +static void +test_pe_initialize(void *arg) +{ + (void) arg; + + /* Initialize the events but the callback won't get called since we would + * need to run the main loop and then wait for a second delaying the unit + * tests. Instead, we'll test the callback work indepedently elsewhere. */ + initialize_periodic_events(); + + /* Validate that all events have been set up. */ + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + tt_assert(item->ev); + tt_assert(item->fn); + tt_u64_op(item->last_action_time, OP_EQ, 0); + /* Every event must have role(s) assign to it. This is done statically. */ + tt_u64_op(item->roles, OP_NE, 0); + tt_uint_op(periodic_event_is_enabled(item), OP_EQ, 0); + } + + done: + teardown_periodic_events(); +} + +static void +test_pe_launch(void *arg) +{ + hs_service_t service; + or_options_t *options; + + (void) arg; + + hs_init(); + + /* Hack: We'll set a dumb fn() of each events so they don't get called when + * dispatching them. We just want to test the state of the callbacks, not + * the whole code path. */ + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + item->fn = dumb_event_fn; + } + + /* Lets make sure that before intialization, we can't scan the periodic + * events list and launch them. Lets try by being a Client. */ + options = get_options_mutable(); + options->SocksPort_set = 1; + periodic_events_on_new_options(options); + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 0); + } + + initialize_periodic_events(); + + /* Now that we've initialized, rescan the list to launch. */ + periodic_events_on_new_options(options); + + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + if (item->roles & PERIODIC_EVENT_ROLE_CLIENT) { + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 1); + tt_u64_op(item->last_action_time, OP_NE, 0); + } else { + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 0); + tt_u64_op(item->last_action_time, OP_EQ, 0); + } + } + + /* Remove Client but become a Relay. */ + options->SocksPort_set = 0; + options->ORPort_set = 1; + periodic_events_on_new_options(options); + + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + /* Only Client role should be disabled. */ + if (item->roles == PERIODIC_EVENT_ROLE_CLIENT) { + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 0); + /* Was previously enabled so they should never be to 0. */ + tt_u64_op(item->last_action_time, OP_NE, 0); + } + if (item->roles & PERIODIC_EVENT_ROLE_RELAY) { + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 1); + tt_u64_op(item->last_action_time, OP_NE, 0); + } + /* Non Relay role should be disabled! */ + if (!(item->roles & PERIODIC_EVENT_ROLE_RELAY)) { + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 0); + } + } + + /* Disable everything and we'll enable them ALL. */ + options->SocksPort_set = 0; + options->ORPort_set = 0; + periodic_events_on_new_options(options); + + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 0); + } + + /* Enable everything. */ + options->SocksPort_set = 1; options->ORPort_set = 1; + options->BridgeRelay = 1; options->AuthoritativeDir = 1; + options->V3AuthoritativeDir = 1; options->BridgeAuthoritativeDir = 1; + register_dummy_hidden_service(&service); + periodic_events_on_new_options(options); + /* Remove it now so the hs_free_all() doesn't try to free stack memory. */ + remove_service(get_hs_service_map(), &service); + + for (int i = 0; periodic_events[i].name; ++i) { + periodic_event_item_t *item = &periodic_events[i]; + tt_int_op(periodic_event_is_enabled(item), OP_EQ, 1); + } + + done: + hs_free_all(); +} + +static void +test_pe_get_roles(void *arg) +{ + int roles; + + (void) arg; + + /* Just so the HS global map exists. */ + hs_init(); + + or_options_t *options = get_options_mutable(); + tt_assert(options); + + /* Nothing configured, should be no roles. */ + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, 0); + + /* Indicate we have a SocksPort, roles should be come Client. */ + options->SocksPort_set = 1; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, PERIODIC_EVENT_ROLE_CLIENT); + + /* Now, we'll add a ORPort so should now be a Relay + Client. */ + options->ORPort_set = 1; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, + (PERIODIC_EVENT_ROLE_CLIENT | PERIODIC_EVENT_ROLE_RELAY)); + + /* Now add a Bridge. */ + options->BridgeRelay = 1; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, + (PERIODIC_EVENT_ROLE_CLIENT | PERIODIC_EVENT_ROLE_RELAY | + PERIODIC_EVENT_ROLE_BRIDGE)); + tt_assert(roles & PERIODIC_EVENT_ROLE_ROUTER); + /* Unset client so we can solely test Router role. */ + options->SocksPort_set = 0; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, PERIODIC_EVENT_ROLE_ROUTER); + + /* Reset options so we can test authorities. */ + options->SocksPort_set = 0; + options->ORPort_set = 0; + options->BridgeRelay = 0; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, 0); + + /* Now upgrade to Dirauth. */ + options->AuthoritativeDir = 1; + options->V3AuthoritativeDir = 1; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, PERIODIC_EVENT_ROLE_DIRAUTH); + tt_assert(roles & PERIODIC_EVENT_ROLE_AUTHORITIES); + + /* Now Bridge Authority. */ + options->V3AuthoritativeDir = 0; + options->BridgeAuthoritativeDir = 1; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, PERIODIC_EVENT_ROLE_BRIDGEAUTH); + tt_assert(roles & PERIODIC_EVENT_ROLE_AUTHORITIES); + + /* Move that bridge auth to become a relay. */ + options->ORPort_set = 1; + roles = get_my_roles(options); + tt_int_op(roles, OP_EQ, + (PERIODIC_EVENT_ROLE_BRIDGEAUTH | PERIODIC_EVENT_ROLE_RELAY)); + tt_assert(roles & PERIODIC_EVENT_ROLE_AUTHORITIES); + + /* And now an Hidden service. */ + hs_service_t service; + register_dummy_hidden_service(&service); + roles = get_my_roles(options); + /* Remove it now so the hs_free_all() doesn't try to free stack memory. */ + remove_service(get_hs_service_map(), &service); + tt_int_op(roles, OP_EQ, + (PERIODIC_EVENT_ROLE_BRIDGEAUTH | PERIODIC_EVENT_ROLE_RELAY | + PERIODIC_EVENT_ROLE_HS_SERVICE)); + tt_assert(roles & PERIODIC_EVENT_ROLE_AUTHORITIES); + + done: + hs_free_all(); +} + +#define PE_TEST(name) \ + { #name, test_pe_## name , TT_FORK, NULL, NULL } + +struct testcase_t periodic_event_tests[] = { + PE_TEST(initialize), + PE_TEST(launch), + PE_TEST(get_roles), + + END_OF_TESTCASES +}; +