From 176999fd955dea5916a560374db7d4efd3a1dad1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 2 Aug 2018 10:03:24 -0400 Subject: When enabling periodic events, schedule but don't run them immediately. When we fixed 25939 in f7633c1fcaefe72bf97c001bab9062132c919996, we introduced a call to rescan_periodic_events() from inside the onion service logic. But this meant that we could rescan the event list -- thereby running event callbacks! -- from inside the hidden service code. This could cause us to run some of our event callbacks from an inconsistent state, if we were in the middle of changing options. A related bug (#25761) prevented us from rescanning our periodic events as appropriate, but when we fixed THAT one, this bug reared its ugly head. The fix here is that "enabling" an event should cause us to run it from the event loop, but not immediately from the point where we enable it. Fixes bug 27003; bugfix on 0.3.4.1-alpha. --- changes/bug27003 | 6 ++++++ src/or/periodic.c | 9 +++++---- src/test/test_periodic_event.c | 8 ++------ 3 files changed, 13 insertions(+), 10 deletions(-) create mode 100644 changes/bug27003 diff --git a/changes/bug27003 b/changes/bug27003 new file mode 100644 index 0000000000..4f2045afc7 --- /dev/null +++ b/changes/bug27003 @@ -0,0 +1,6 @@ + o Major bugfixes (event scheduler): + - When we enable a periodic event, schedule it in the event loop + rather than running it immediately. Previously, we would re-run + periodic events immediately in the middle of (for example) + changing our options, with unpredictable effects. Fixes bug + 27003; bugfix on 0.3.4.1-alpha. diff --git a/src/or/periodic.c b/src/or/periodic.c index 92fa677f8f..9470376d06 100644 --- a/src/or/periodic.c +++ b/src/or/periodic.c @@ -140,8 +140,8 @@ periodic_event_destroy(periodic_event_item_t *event) 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 +/** Enable the given event by setting its "enabled" flag and scheduling it to + * run immediately in the event loop. This can be called for an event that is * already enabled. */ void periodic_event_enable(periodic_event_item_t *event) @@ -152,7 +152,9 @@ periodic_event_enable(periodic_event_item_t *event) return; } - periodic_event_launch(event); + tor_assert(event->ev); + event->enabled = 1; + mainloop_event_activate(event->ev); } /** Disable the given event which means the event is destroyed and then the @@ -169,4 +171,3 @@ periodic_event_disable(periodic_event_item_t *event) mainloop_event_cancel(event->ev); event->enabled = 0; } - diff --git a/src/test/test_periodic_event.c b/src/test/test_periodic_event.c index 34689b64f4..f159c4f83a 100644 --- a/src/test/test_periodic_event.c +++ b/src/test/test_periodic_event.c @@ -106,11 +106,11 @@ test_pe_launch(void *arg) 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); } + // enabled or not, the event has not yet been run. + tt_u64_op(item->last_action_time, OP_EQ, 0); } /* Remove Client but become a Relay. */ @@ -127,12 +127,9 @@ test_pe_launch(void *arg) /* 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, except for Dirserver. */ if (!(item->roles & roles)) { @@ -330,4 +327,3 @@ struct testcase_t periodic_event_tests[] = { END_OF_TESTCASES }; - -- cgit v1.2.3-54-g00ecf