touchups and refactorings on bug 18616 branch

no behavior changes

9 files changed, 59 insertions, 52 deletions

diff --git a/changes/bug18616 b/changes/bug18616
index d760f15adb..ec59e846ed 100644
--- a/changes/bug18616
+++ b/changes/bug18616
@@ -1,8 +1,14 @@
   o Major bugfixes (directory mirrors):
-    - Fix broken DirPort self-checks. Decide to advertise begindir
-      support the same way we decide to advertise DirPorts.
-      Add additional config options that might change the content of
-      a relay's descriptor.
-      Avoid checking ORPort reachability when the network is disabled.
-      Resolves #18616, bugfix on 0c8e042c30946faa in #12538 in
-      0.2.8.1-alpha. Patch by "teor".
+    - Decide whether to advertise begindir support the same way we decide
+      whether to advertise our DirPort. These decisions being out of sync
+      led to surprising behavior like advertising begindir support when
+      our hibernation config options made us not advertise a DirPort.
+      Resolves bug 18616; bugfix on 0.2.8.1-alpha. Patch by teor.
+
+  o Minor bugfixes:
+    - Consider more config options when relays decide whether to regenerate
+      their descriptor. Fixes more of bug 12538; bugfix on 0.2.8.1-alpha.
+    - Resolve some edge cases where we might launch an ORPort reachability
+      check even when DisableNetwork is set. Noticed while fixing bug
+      18616; bugfix on 0.2.3.9-alpha.
+
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index a5a933e6b0..820724adea 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -984,7 +984,7 @@ circuit_send_next_onion_skin(origin_circuit_t *circ)
         }
         control_event_client_status(LOG_NOTICE, "CIRCUIT_ESTABLISHED");
         clear_broken_connection_map(1);
-        if (server_mode(options) && !check_whether_orport_reachable()) {
+        if (server_mode(options) && !check_whether_orport_reachable(options)) {
           inform_testing_reachability();
           consider_testing_reachability(1, 1);
         }
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index 31003ea095..f96a3b18f1 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -1426,7 +1426,7 @@ static void
 circuit_testing_opened(origin_circuit_t *circ)
 {
   if (have_performed_bandwidth_test ||
-      !check_whether_orport_reachable()) {
+      !check_whether_orport_reachable(get_options())) {
     /* either we've already done everything we want with testing circuits,
      * or this testing circuit became open due to a fluke, e.g. we picked
      * a last hop where we already had the connection open due to an
@@ -1443,7 +1443,8 @@ circuit_testing_opened(origin_circuit_t *circ)
 static void
 circuit_testing_failed(origin_circuit_t *circ, int at_last_hop)
 {
-  if (server_mode(get_options()) && check_whether_orport_reachable())
+  const or_options_t *options = get_options();
+  if (server_mode(options) && check_whether_orport_reachable(options))
     return;
 
   log_info(LD_GENERAL,
diff --git a/src/or/control.c b/src/or/control.c
index 655b4dd335..a2ae2e9f1d 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -2143,6 +2143,7 @@ getinfo_helper_events(control_connection_t *control_conn,
                       const char *question, char **answer,
                       const char **errmsg)
 {
+  const or_options_t *options = get_options();
   (void) control_conn;
   if (!strcmp(question, "circuit-status")) {
     smartlist_t *status = smartlist_new();
@@ -2279,17 +2280,19 @@ getinfo_helper_events(control_connection_t *control_conn,
       *answer = tor_strdup(directories_have_accepted_server_descriptor()
                            ? "1" : "0");
     } else if (!strcmp(question, "status/reachability-succeeded/or")) {
-      *answer = tor_strdup(check_whether_orport_reachable() ? "1" : "0");
+      *answer = tor_strdup(check_whether_orport_reachable(options) ?
+                            "1" : "0");
     } else if (!strcmp(question, "status/reachability-succeeded/dir")) {
-      *answer = tor_strdup(check_whether_dirport_reachable() ? "1" : "0");
+      *answer = tor_strdup(check_whether_dirport_reachable(options) ?
+                            "1" : "0");
     } else if (!strcmp(question, "status/reachability-succeeded")) {
       tor_asprintf(answer, "OR=%d DIR=%d",
-                   check_whether_orport_reachable() ? 1 : 0,
-                   check_whether_dirport_reachable() ? 1 : 0);
+                   check_whether_orport_reachable(options) ? 1 : 0,
+                   check_whether_dirport_reachable(options) ? 1 : 0);
     } else if (!strcmp(question, "status/bootstrap-phase")) {
       *answer = tor_strdup(last_sent_bootstrap_message);
     } else if (!strcmpstart(question, "status/version/")) {
-      int is_server = server_mode(get_options());
+      int is_server = server_mode(options);
       networkstatus_t *c = networkstatus_get_latest_consensus();
       version_status_t status;
       const char *recommended;
@@ -2331,7 +2334,7 @@ getinfo_helper_events(control_connection_t *control_conn,
       }
       *answer = bridge_stats;
     } else if (!strcmp(question, "status/fresh-relay-descs")) {
-      if (!server_mode(get_options())) {
+      if (!server_mode(options)) {
         *errmsg = "Only relays have descriptors";
         return -1;
       }
diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index 5f848bd575..846951c8ed 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -1133,9 +1133,9 @@ directory_caches_unknown_auth_certs(const or_options_t *options)
 
 /** Return 1 if we want to keep descriptors, networkstatuses, etc around.
  * Else return 0.
- * Check get_options()->DirPort_set and directory_permits_begindir_requests()
+ * Check options->DirPort_set and directory_permits_begindir_requests()
  * to see if we are willing to serve these directory documents to others via
- * the DirPort and begindir over ORPort, respectively.
+ * the DirPort and begindir-over-ORPort, respectively.
  */
 int
 directory_caches_dir_info(const or_options_t *options)
diff --git a/src/or/main.c b/src/or/main.c
index a2cf5b1101..86fb2bd46d 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -2094,7 +2094,7 @@ second_elapsed_callback(periodic_timer_t *timer, void *arg)
         TIMEOUT_UNTIL_UNREACHABILITY_COMPLAINT) {
     /* every 20 minutes, check and complain if necessary */
     const routerinfo_t *me = router_get_my_routerinfo();
-    if (me && !check_whether_orport_reachable()) {
+    if (me && !check_whether_orport_reachable(options)) {
       char *address = tor_dup_ip(me->addr);
       log_warn(LD_CONFIG,"Your server (%s:%d) has not managed to confirm that "
                "its ORPort is reachable. Relays do not publish descriptors "
@@ -2107,7 +2107,7 @@ second_elapsed_callback(periodic_timer_t *timer, void *arg)
       tor_free(address);
     }
 
-    if (me && !check_whether_dirport_reachable()) {
+    if (me && !check_whether_dirport_reachable(options)) {
       char *address = tor_dup_ip(me->addr);
       log_warn(LD_CONFIG,
                "Your server (%s:%d) has not managed to confirm that its "
diff --git a/src/or/rephist.c b/src/or/rephist.c
index fe0ca91c25..04ed7aef0f 100644
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -1867,14 +1867,17 @@ any_predicted_circuits(time_t now)
 int
 rep_hist_circbuilding_dormant(time_t now)
 {
+  const or_options_t *options = get_options();
+
   if (any_predicted_circuits(now))
     return 0;
 
   /* see if we'll still need to build testing circuits */
-  if (server_mode(get_options()) &&
-      (!check_whether_orport_reachable() || !circuit_enough_testing_circs()))
+  if (server_mode(options) &&
+      (!check_whether_orport_reachable(options) ||
+       !circuit_enough_testing_circs()))
     return 0;
-  if (!check_whether_dirport_reachable())
+  if (!check_whether_dirport_reachable(options))
     return 0;
 
   return 1;
diff --git a/src/or/router.c b/src/or/router.c
index 8f369f8727..57c0618bfb 100644
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -1079,7 +1079,7 @@ router_reset_reachability(void)
   can_reach_or_port = can_reach_dir_port = 0;
 }
 
-/**  Return 1 if we won't do reachability checks, because:
+/** Return 1 if we won't do reachability checks, because:
  *   - AssumeReachable is set, or
  *   - the network is disabled.
  * Otherwise, return 0.
@@ -1100,9 +1100,8 @@ router_reachability_checks_disabled(const or_options_t *options)
  *   - the network is disabled.
  */
 int
-check_whether_orport_reachable(void)
+check_whether_orport_reachable(const or_options_t *options)
 {
-  const or_options_t *options = get_options();
   int reach_checks_disabled = router_reachability_checks_disabled(options);
   return reach_checks_disabled ||
          can_reach_or_port;
@@ -1118,9 +1117,8 @@ check_whether_orport_reachable(void)
  *   - the network is disabled.
  */
 int
-check_whether_dirport_reachable(void)
+check_whether_dirport_reachable(const or_options_t *options)
 {
-  const or_options_t *options = get_options();
   int reach_checks_disabled = router_reachability_checks_disabled(options) ||
                               !options->DirPort_set;
   return reach_checks_disabled ||
@@ -1258,18 +1256,14 @@ decide_to_advertise_dir_impl(const or_options_t *options,
       !router_get_advertised_or_port(options))
     return 0;
 
-  /* Part two: reasons to publish or not publish that the user
-   * might find surprising. router_should_be_directory_server()
-   * considers config options that make us choose not to publish. */
+  /* Part two: consider config options that could make us choose to
+   * publish or not publish that the user might find surprising. */
   return router_should_be_directory_server(options, dir_port);
 }
 
-/** Look at a variety of factors, and return 0 if we don't want to
+/** Front-end to decide_to_advertise_dir_impl(): return 0 if we don't want to
  * advertise the fact that we have a DirPort open, else return the
  * DirPort we want to advertise.
- *
- * Log a helpful message if we change our mind about whether to publish
- * a DirPort.
  */
 static int
 decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
@@ -1278,11 +1272,8 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
   return decide_to_advertise_dir_impl(options, dir_port, 0) ? dir_port : 0;
 }
 
-/** Look at a variety of factors, and return 0 if we don't want to
+/** Front-end to decide_to_advertise_dir_impl(): return 0 if we don't want to
  * advertise the fact that we support begindir requests, else return 1.
- *
- * Log a helpful message if we change our mind about whether to advertise
- * support for begindir.
  */
 static int
 decide_to_advertise_begindir(const or_options_t *options,
@@ -1324,9 +1315,9 @@ void
 consider_testing_reachability(int test_or, int test_dir)
 {
   const routerinfo_t *me = router_get_my_routerinfo();
-  int orport_reachable = check_whether_orport_reachable();
-  tor_addr_t addr;
   const or_options_t *options = get_options();
+  int orport_reachable = check_whether_orport_reachable(options);
+  tor_addr_t addr;
   if (!me)
     return;
 
@@ -1359,7 +1350,7 @@ consider_testing_reachability(int test_or, int test_dir)
 
   /* XXX IPv6 self testing */
   tor_addr_from_ipv4h(&addr, me->addr);
-  if (test_dir && !check_whether_dirport_reachable() &&
+  if (test_dir && !check_whether_dirport_reachable(options) &&
       !connection_get_by_type_addr_port_purpose(
                 CONN_TYPE_DIR, &addr, me->dir_port,
                 DIR_PURPOSE_FETCH_SERVERDESC)) {
@@ -1378,18 +1369,19 @@ void
 router_orport_found_reachable(void)
 {
   const routerinfo_t *me = router_get_my_routerinfo();
+  const or_options_t *options = get_options();
   if (!can_reach_or_port && me) {
     char *address = tor_dup_ip(me->addr);
     log_notice(LD_OR,"Self-testing indicates your ORPort is reachable from "
                "the outside. Excellent.%s",
-               get_options()->PublishServerDescriptor_ != NO_DIRINFO
-               && check_whether_dirport_reachable() ?
+               options->PublishServerDescriptor_ != NO_DIRINFO
+               && check_whether_dirport_reachable(options) ?
                  " Publishing server descriptor." : "");
     can_reach_or_port = 1;
     mark_my_descriptor_dirty("ORPort found reachable");
     /* This is a significant enough change to upload immediately,
      * at least in a test network */
-    if (get_options()->TestingTorNetwork == 1) {
+    if (options->TestingTorNetwork == 1) {
       reschedule_descriptor_update_check();
     }
     control_event_server_status(LOG_NOTICE,
@@ -1404,19 +1396,20 @@ void
 router_dirport_found_reachable(void)
 {
   const routerinfo_t *me = router_get_my_routerinfo();
+  const or_options_t *options = get_options();
   if (!can_reach_dir_port && me) {
     char *address = tor_dup_ip(me->addr);
     log_notice(LD_DIRSERV,"Self-testing indicates your DirPort is reachable "
                "from the outside. Excellent.%s",
-               get_options()->PublishServerDescriptor_ != NO_DIRINFO
-               && check_whether_orport_reachable() ?
+               options->PublishServerDescriptor_ != NO_DIRINFO
+               && check_whether_orport_reachable(options) ?
                " Publishing server descriptor." : "");
     can_reach_dir_port = 1;
-    if (decide_to_advertise_dirport(get_options(), me->dir_port)) {
+    if (decide_to_advertise_dirport(options, me->dir_port)) {
       mark_my_descriptor_dirty("DirPort found reachable");
       /* This is a significant enough change to upload immediately,
        * at least in a test network */
-      if (get_options()->TestingTorNetwork == 1) {
+      if (options->TestingTorNetwork == 1) {
         reschedule_descriptor_update_check();
       }
     }
@@ -1633,7 +1626,8 @@ decide_if_publishable_server(void)
   if (!router_get_advertised_or_port(options))
     return 0;
 
-  return check_whether_orport_reachable() && check_whether_dirport_reachable();
+  return check_whether_orport_reachable(options) &&
+         check_whether_dirport_reachable(options);
 }
 
 /** Initiate server descriptor upload as reasonable (if server is publishable,
diff --git a/src/or/router.h b/src/or/router.h
index 5165462a13..73bfea1faa 100644
--- a/src/or/router.h
+++ b/src/or/router.h
@@ -39,8 +39,8 @@ int router_initialize_tls_context(void);
 int init_keys(void);
 int init_keys_client(void);
 
-int check_whether_orport_reachable(void);
-int check_whether_dirport_reachable(void);
+int check_whether_orport_reachable(const or_options_t *options);
+int check_whether_dirport_reachable(const or_options_t *options);
 int dir_server_mode(const or_options_t *options);
 void consider_testing_reachability(int test_or, int test_dir);
 void router_orport_found_reachable(void);