commit 176999fd955dea5916a560374db7d4efd3a1dad1 Author: Nick Mathewson <nickm@torproject.org> Date: Thu Aug 2 10:03:24 2018 -0400 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(-) diff --git a/changes/bug27003 b/changes/bug27003 new file mode 100644 index 000000000..4f2045afc --- /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 92fa677f8..9470376d0 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 34689b64f..f159c4f83 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 }; -