[tor-commits] [tor/release-0.3.4] When enabling periodic events, schedule but don't run them immediately.

[nickm at torproject.org]

commit 176999fd955dea5916a560374db7d4efd3a1dad1
Author: Nick Mathewson <nickm at 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
 };
-