Merge branch 'maint-0.2.8'

changes/bug18616

@@ -0,0 +1,14 @@
+  o Major bugfixes (directory mirrors):
+    - 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.
+

src/or/circuitbuild.c

@@ -980,7 +980,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);
         }

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,

src/or/config.c

@@ -4344,8 +4344,10 @@ options_transition_affects_descriptor(const or_options_t *old_options,
       !opt_streq(old_options->MyFamily, new_options->MyFamily) ||
       !opt_streq(old_options->AccountingStart, new_options->AccountingStart) ||
       old_options->AccountingMax != new_options->AccountingMax ||
+      old_options->AccountingRule != new_options->AccountingRule ||
       public_server_mode(old_options) != public_server_mode(new_options) ||
-      old_options->DirCache != new_options->DirCache)
+      old_options->DirCache != new_options->DirCache ||
+      old_options->AssumeReachable != new_options->AssumeReachable)
     return 1;
 
   return 0;
@@ -7006,9 +7008,8 @@ get_first_listener_addrport_string(int listener_type)
 int
 get_first_advertised_port_by_type_af(int listener_type, int address_family)
 {
-  if (!configured_ports)
-    return 0;
-  SMARTLIST_FOREACH_BEGIN(configured_ports, const port_cfg_t *, cfg) {
+  const smartlist_t *conf_ports = get_configured_ports();
+  SMARTLIST_FOREACH_BEGIN(conf_ports, const port_cfg_t *, cfg) {
     if (cfg->type == listener_type &&
         !cfg->server_cfg.no_advertise &&
         (tor_addr_family(&cfg->addr) == address_family ||

src/or/control.c

@@ -2148,6 +2148,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();
@@ -2284,17 +2285,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;
@@ -2336,7 +2339,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;
       }

src/or/dirserv.c

@@ -1131,8 +1131,11 @@ directory_caches_unknown_auth_certs(const or_options_t *options)
   return dir_server_mode(options) || options->BridgeRelay;
 }
 
-/** Return 1 if we want to keep descriptors, networkstatuses, etc around
- * and we're willing to serve them to others. Else return 0.
+/** Return 1 if we want to keep descriptors, networkstatuses, etc around.
+ * Else return 0.
+ * 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.
  */
 int
 directory_caches_dir_info(const or_options_t *options)

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 "

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;

src/or/router.c

@@ -1079,23 +1079,49 @@ router_reset_reachability(void)
   can_reach_or_port = can_reach_dir_port = 0;
 }
 
-/** Return 1 if ORPort is known reachable; else return 0. */
-int
-check_whether_orport_reachable(void)
+/** Return 1 if we won't do reachability checks, because:
+ *   - AssumeReachable is set, or
+ *   - the network is disabled.
+ * Otherwise, return 0.
+ */
+static int
+router_reachability_checks_disabled(const or_options_t *options)
 {
-  const or_options_t *options = get_options();
   return options->AssumeReachable ||
+         net_is_disabled();
+}
+
+/** Return 0 if we need to do an ORPort reachability check, because:
+ *   - no reachability check has been done yet, or
+ *   - we've initiated reachability checks, but none have succeeded.
+ *  Return 1 if we don't need to do an ORPort reachability check, because:
+ *   - we've seen a successful reachability check, or
+ *   - AssumeReachable is set, or
+ *   - the network is disabled.
+ */
+int
+check_whether_orport_reachable(const or_options_t *options)
+{
+  int reach_checks_disabled = router_reachability_checks_disabled(options);
+  return reach_checks_disabled ||
          can_reach_or_port;
 }
 
-/** Return 1 if we don't have a dirport configured, or if it's reachable. */
+/** Return 0 if we need to do a DirPort reachability check, because:
+ *   - no reachability check has been done yet, or
+ *   - we've initiated reachability checks, but none have succeeded.
+ *  Return 1 if we don't need to do a DirPort reachability check, because:
+ *   - we've seen a successful reachability check, or
+ *   - there is no DirPort set, or
+ *   - AssumeReachable is set, or
+ *   - 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();
-  return !options->DirPort_set ||
-         options->AssumeReachable ||
-         net_is_disabled() ||
+  int reach_checks_disabled = router_reachability_checks_disabled(options) ||
+                              !options->DirPort_set;
+  return reach_checks_disabled ||
          can_reach_dir_port;
 }
 
@@ -1148,10 +1174,11 @@ router_should_be_directory_server(const or_options_t *options, int dir_port)
                        "seconds long. Raising to 1.");
       interval_length = 1;
     }
-    log_info(LD_GENERAL, "Calculating whether to disable dirport: effective "
+    log_info(LD_GENERAL, "Calculating whether to advertise %s: effective "
                          "bwrate: %u, AccountingMax: "U64_FORMAT", "
-                         "accounting interval length %d", effective_bw,
-                         U64_PRINTF_ARG(options->AccountingMax),
+                         "accounting interval length %d",
+                         dir_port ? "dirport" : "begindir",
+                         effective_bw, U64_PRINTF_ARG(options->AccountingMax),
                          interval_length);
 
     acc_bytes = options->AccountingMax;
@@ -1199,34 +1226,62 @@ dir_server_mode(const or_options_t *options)
 }
 
 /** Look at a variety of factors, and 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.
+ * advertise the fact that we have a DirPort open or begindir support, else
+ * return 1.
  *
- * Log a helpful message if we change our mind about whether to publish
- * a DirPort.
+ * Where dir_port or supports_tunnelled_dir_requests are not relevant, they
+ * must be 0.
+ *
+ * Log a helpful message if we change our mind about whether to publish.
  */
 static int
-decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
+decide_to_advertise_dir_impl(const or_options_t *options,
+                             uint16_t dir_port,
+                             int supports_tunnelled_dir_requests)
 {
   /* Part one: reasons to publish or not publish that aren't
    * worth mentioning to the user, either because they're obvious
    * or because they're normal behavior. */
 
-  if (!dir_port) /* short circuit the rest of the function */
+  /* short circuit the rest of the function */
+  if (!dir_port && !supports_tunnelled_dir_requests)
     return 0;
   if (authdir_mode(options)) /* always publish */
-    return dir_port;
+    return 1;
   if (net_is_disabled())
     return 0;
-  if (!check_whether_dirport_reachable())
+  if (dir_port && !router_get_advertised_dir_port(options, dir_port))
     return 0;
-  if (!router_get_advertised_dir_port(options, dir_port))
+  if (supports_tunnelled_dir_requests &&
+      !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. */
-  return router_should_be_directory_server(options, dir_port) ? dir_port : 0;
+  /* 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);
+}
+
+/** 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.
+ */
+static int
+decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
+{
+  /* supports_tunnelled_dir_requests is not relevant, pass 0 */
+  return decide_to_advertise_dir_impl(options, dir_port, 0) ? dir_port : 0;
+}
+
+/** 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.
+ */
+static int
+decide_to_advertise_begindir(const or_options_t *options,
+                             int supports_tunnelled_dir_requests)
+{
+  /* dir_port is not relevant, pass 0 */
+  return decide_to_advertise_dir_impl(options, 0,
+                                      supports_tunnelled_dir_requests);
 }
 
 /** Allocate and return a new extend_info_t that can be used to build
@@ -1260,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;
 
@@ -1295,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)) {
@@ -1314,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,
@@ -1340,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();
       }
     }
@@ -1570,14 +1627,14 @@ decide_if_publishable_server(void)
     return 1;
   if (!router_get_advertised_or_port(options))
     return 0;
-  if (!check_whether_orport_reachable())
+  if (!check_whether_orport_reachable(options))
     return 0;
   if (router_have_consensus_path() == CONSENSUS_PATH_INTERNAL) {
     /* All set: there are no exits in the consensus (maybe this is a tiny
      * test network), so we can't check our DirPort reachability. */
     return 1;
   } else {
-    return check_whether_dirport_reachable();
+    return check_whether_dirport_reachable(options);
   }
 }
 
@@ -1933,8 +1990,8 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
   ri->addr = addr;
   ri->or_port = router_get_advertised_or_port(options);
   ri->dir_port = router_get_advertised_dir_port(options, 0);
-  ri->supports_tunnelled_dir_requests = dir_server_mode(options) &&
-    router_should_be_directory_server(options, ri->dir_port);
+  ri->supports_tunnelled_dir_requests =
+    directory_permits_begindir_requests(options);
   ri->cache_info.published_on = time(NULL);
   ri->onion_pkey = crypto_pk_dup_key(get_onion_key()); /* must invoke from
                                                         * main thread */
@@ -2715,7 +2772,8 @@ router_dump_router_to_string(routerinfo_t *router,
     tor_free(p6);
   }
 
-  if (router->supports_tunnelled_dir_requests) {
+  if (decide_to_advertise_begindir(options,
+                                   router->supports_tunnelled_dir_requests)) {
     smartlist_add(chunks, tor_strdup("tunnelled-dir-server\n"));
   }
 

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

src/test/test_dir.c

@@ -85,6 +85,15 @@ test_dir_nicknames(void *arg)
   ;
 }
 
+static smartlist_t *mocked_configured_ports = NULL;
+
+/** Returns mocked_configured_ports */
+static const smartlist_t *
+mock_get_configured_ports(void)
+{
+  return mocked_configured_ports;
+}
+
 /** Run unit tests for router descriptor generation logic. */
 static void
 test_dir_formats(void *arg)
@@ -104,6 +113,7 @@ test_dir_formats(void *arg)
   or_options_t *options = get_options_mutable();
   const addr_policy_t *p;
   time_t now = time(NULL);
+  port_cfg_t orport, dirport;
 
   (void)arg;
   pk1 = pk_generate(0);
@@ -185,9 +195,31 @@ test_dir_formats(void *arg)
   /* XXXX025 router_dump_to_string should really take this from ri.*/
   options->ContactInfo = tor_strdup("Magri White "
                                     "<magri@elsewhere.example.com>");
+  /* Skip reachability checks for DirPort and tunnelled-dir-server */
+  options->AssumeReachable = 1;
+
+  /* Fake just enough of an ORPort and DirPort to get by */
+  MOCK(get_configured_ports, mock_get_configured_ports);
+  mocked_configured_ports = smartlist_new();
+
+  memset(&orport, 0, sizeof(orport));
+  orport.type = CONN_TYPE_OR_LISTENER;
+  orport.addr.family = AF_INET;
+  orport.port = 9000;
+  smartlist_add(mocked_configured_ports, &orport);
+
+  memset(&dirport, 0, sizeof(dirport));
+  dirport.type = CONN_TYPE_DIR_LISTENER;
+  dirport.addr.family = AF_INET;
+  dirport.port = 9003;
+  smartlist_add(mocked_configured_ports, &dirport);
 
   buf = router_dump_router_to_string(r1, pk2, NULL, NULL, NULL);
 
+  UNMOCK(get_configured_ports);
+  smartlist_free(mocked_configured_ports);
+  mocked_configured_ports = NULL;
+
   tor_free(options->ContactInfo);
   tt_assert(buf);
 
@@ -308,6 +340,16 @@ test_dir_formats(void *arg)
   strlcat(buf2, "tunnelled-dir-server\n", sizeof(buf2));
   strlcat(buf2, "router-sig-ed25519 ", sizeof(buf2));
 
+  /* Fake just enough of an ORPort to get by */
+  MOCK(get_configured_ports, mock_get_configured_ports);
+  mocked_configured_ports = smartlist_new();
+
+  memset(&orport, 0, sizeof(orport));
+  orport.type = CONN_TYPE_OR_LISTENER;
+  orport.addr.family = AF_INET;
+  orport.port = 9005;
+  smartlist_add(mocked_configured_ports, &orport);
+
   buf = router_dump_router_to_string(r2, pk1, pk2, &r2_onion_keypair, &kp2);
   tt_assert(buf);
   buf[strlen(buf2)] = '\0'; /* Don't compare the sig; it's never the same
@@ -318,6 +360,10 @@ test_dir_formats(void *arg)
 
   buf = router_dump_router_to_string(r2, pk1, NULL, NULL, NULL);
 
+  UNMOCK(get_configured_ports);
+  smartlist_free(mocked_configured_ports);
+  mocked_configured_ports = NULL;
+
   /* Reset for later */
   cp = buf;
   rp2 = router_parse_entry_from_string((const char*)cp,NULL,1,0,NULL,NULL);

src/test/test_policy.c

@@ -716,10 +716,9 @@ test_policies_reject_exit_address(void *arg)
 }
 
 static smartlist_t *test_configured_ports = NULL;
-const smartlist_t *mock_get_configured_ports(void);
 
 /** Returns test_configured_ports */
-const smartlist_t *
+static const smartlist_t *
 mock_get_configured_ports(void)
 {
   return test_configured_ports;