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.

3 files changed, 13 insertions, 10 deletions

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
 };
-