Merge remote-tracking branch 'teor/fix-multi-dir'

src/or/networkstatus.c

@@ -1286,25 +1286,29 @@ networkstatus_consensus_is_boostrapping(time_t now)
 }
 
 /** Check if we can use multiple directories for a consensus download.
- * Only clients (including bridges, but excluding bridge clients) benefit
+ * Only clients (including bridge relays, which act like clients) benefit
  * from multiple simultaneous consensus downloads. */
 int
 networkstatus_consensus_can_use_multiple_directories(
                                                   const or_options_t *options)
 {
   /* If we are a client, bridge, bridge client, or hidden service */
-  return (!directory_fetches_from_authorities(options));
+  return !public_server_mode(options);
 }
 
 /** Check if we can use fallback directory mirrors for a consensus download.
- * Only clients that have a list of additional fallbacks can use fallbacks. */
+ * If we have fallbacks and don't want to fetch from the authorities,
+ * we can use them. */
 int
 networkstatus_consensus_can_use_extra_fallbacks(const or_options_t *options)
 {
-  /* If we are a client, and we have additional mirrors, we can use them.
-   * The list length comparisons are a quick way to check if we have any
+  /* The list length comparisons are a quick way to check if we have any
    * non-authority fallback directories. If we ever have any authorities that
    * aren't fallback directories, we will need to change this code. */
+  tor_assert(smartlist_len(router_get_fallback_dir_servers())
+             >= smartlist_len(router_get_trusted_dir_servers()));
+  /* If we don't fetch from the authorities, and we have additional mirrors,
+   * we can use them. */
   return (!directory_fetches_from_authorities(options)
           && (smartlist_len(router_get_fallback_dir_servers())
               > smartlist_len(router_get_trusted_dir_servers())));

src/or/router.c

@@ -1457,8 +1457,8 @@ static int server_is_advertised=0;
 
 /** Return true iff we have published our descriptor lately.
  */
-int
-advertised_server_mode(void)
+MOCK_IMPL(int,
+advertised_server_mode,(void))
 {
   return server_is_advertised;
 }
@@ -1820,8 +1820,8 @@ static int router_guess_address_from_dir_headers(uint32_t *guess);
  * it's configured in torrc, or because we've learned it from
  * dirserver headers. Place the answer in *<b>addr</b> and return
  * 0 on success, else return -1 if we have no guess. */
-int
-router_pick_published_address(const or_options_t *options, uint32_t *addr)
+MOCK_IMPL(int,
+router_pick_published_address,(const or_options_t *options, uint32_t *addr))
 {
   *addr = get_last_resolved_addr();
   if (!*addr &&

src/or/router.h

@@ -67,7 +67,7 @@ uint16_t router_get_advertised_dir_port(const or_options_t *options,
 
 MOCK_DECL(int, server_mode, (const or_options_t *options));
 MOCK_DECL(int, public_server_mode, (const or_options_t *options));
-int advertised_server_mode(void);
+MOCK_DECL(int, advertised_server_mode, (void));
 int proxy_mode(const or_options_t *options);
 void consider_publishable_server(int force);
 int should_refuse_unknown_exits(const or_options_t *options);
@@ -89,7 +89,8 @@ int router_digest_is_me(const char *digest);
 const uint8_t *router_get_my_id_digest(void);
 int router_extrainfo_digest_is_me(const char *digest);
 int router_is_me(const routerinfo_t *router);
-int router_pick_published_address(const or_options_t *options, uint32_t *addr);
+MOCK_DECL(int,router_pick_published_address,(const or_options_t *options,
+                                             uint32_t *addr));
 int router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e);
 int router_rebuild_descriptor(int force);
 char *router_dump_router_to_string(routerinfo_t *router,

src/test/test_config.c

@@ -19,6 +19,8 @@
 #include "transports.h"
 #include "routerlist.h"
 #include "networkstatus.h"
+#include "router.h"
+#include "dirserv.h"
 
 static void
 test_config_addressmap(void *arg)
@@ -3450,22 +3452,74 @@ test_config_default_dir_servers(void *arg)
   or_options_free(opts);
 }
 
+static int mock_router_pick_published_address_result = 0;
+
+static int
+mock_router_pick_published_address(const or_options_t *options, uint32_t *addr)
+{
+  (void)options;
+  (void)addr;
+  return mock_router_pick_published_address_result;
+}
+
+static int mock_router_my_exit_policy_is_reject_star_result = 0;
+
+static int
+mock_router_my_exit_policy_is_reject_star(void)
+{
+  return mock_router_my_exit_policy_is_reject_star_result;
+}
+
+static int mock_advertised_server_mode_result = 0;
+
+static int
+mock_advertised_server_mode(void)
+{
+  return mock_advertised_server_mode_result;
+}
+
+static routerinfo_t *mock_router_get_my_routerinfo_result = NULL;
+
+static const routerinfo_t *
+mock_router_get_my_routerinfo(void)
+{
+  return mock_router_get_my_routerinfo_result;
+}
+
 static void
-test_config_use_multiple_directories(void *arg)
+test_config_directory_fetch(void *arg)
 {
   (void)arg;
 
+  /* Test Setup */
   or_options_t *options = tor_malloc_zero(sizeof(or_options_t));
+  routerinfo_t routerinfo;
+  memset(&routerinfo, 0, sizeof(routerinfo));
+  mock_router_pick_published_address_result = -1;
+  mock_router_my_exit_policy_is_reject_star_result = 1;
+  mock_advertised_server_mode_result = 0;
+  mock_router_get_my_routerinfo_result = NULL;
+  MOCK(router_pick_published_address, mock_router_pick_published_address);
+  MOCK(router_my_exit_policy_is_reject_star,
+       mock_router_my_exit_policy_is_reject_star);
+  MOCK(advertised_server_mode, mock_advertised_server_mode);
+  MOCK(router_get_my_routerinfo, mock_router_get_my_routerinfo);
 
   /* Clients can use multiple directory mirrors for bootstrap */
   memset(options, 0, sizeof(or_options_t));
   options->ClientOnly = 1;
+  tt_assert(server_mode(options) == 0);
+  tt_assert(public_server_mode(options) == 0);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
   tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
             == 1);
 
   /* Bridge Clients can use multiple directory mirrors for bootstrap */
   memset(options, 0, sizeof(or_options_t));
   options->UseBridges = 1;
+  tt_assert(server_mode(options) == 0);
+  tt_assert(public_server_mode(options) == 0);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
   tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
             == 1);
 
@@ -3473,23 +3527,130 @@ test_config_use_multiple_directories(void *arg)
    * directory mirrors for bootstrap */
   memset(options, 0, sizeof(or_options_t));
   options->BridgeRelay = 1;
+  options->ORPort_set = 1;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 0);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
   tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
             == 1);
 
-  /* Clients set to FetchDirInfoEarly must fetch it from the authorities */
+  /* Clients set to FetchDirInfoEarly must fetch it from the authorities,
+   * but can use multiple authorities for bootstrap */
   memset(options, 0, sizeof(or_options_t));
   options->FetchDirInfoEarly = 1;
+  tt_assert(server_mode(options) == 0);
+  tt_assert(public_server_mode(options) == 0);
+  tt_assert(directory_fetches_from_authorities(options) == 1);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 1);
+
+  /* OR servers only fetch the consensus from the authorities when they don't
+   * know their own address, but never use multiple directories for bootstrap
+   */
+  memset(options, 0, sizeof(or_options_t));
+  options->ORPort_set = 1;
+
+  mock_router_pick_published_address_result = -1;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 1);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  mock_router_pick_published_address_result = 0;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  /* Exit OR servers only fetch the consensus from the authorities when they
+   * refuse unknown exits, but never use multiple directories for bootstrap
+   */
+  memset(options, 0, sizeof(or_options_t));
+  options->ORPort_set = 1;
+  options->ExitRelay = 1;
+  mock_router_pick_published_address_result = 0;
+  mock_router_my_exit_policy_is_reject_star_result = 0;
+  mock_advertised_server_mode_result = 1;
+  mock_router_get_my_routerinfo_result = &routerinfo;
+
+  options->RefuseUnknownExits = 1;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 1);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  options->RefuseUnknownExits = 0;
+  mock_router_pick_published_address_result = 0;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
   tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
             == 0);
 
-  /* OR servers must fetch the consensus from the authorities */
+  /* Dir servers fetch the consensus from the authorities, unless they are not
+   * advertising themselves (hibernating) or have no routerinfo or are not
+   * advertising their dirport, and never use multiple directories for
+   * bootstrap. This only applies if they are also OR servers.
+   * (We don't care much about the behaviour of non-OR directory servers.) */
   memset(options, 0, sizeof(or_options_t));
+  options->DirPort_set = 1;
   options->ORPort_set = 1;
+  mock_router_pick_published_address_result = 0;
+  mock_router_my_exit_policy_is_reject_star_result = 1;
+
+  mock_advertised_server_mode_result = 1;
+  routerinfo.dir_port = 1;
+  mock_router_get_my_routerinfo_result = &routerinfo;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 1);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  mock_advertised_server_mode_result = 0;
+  routerinfo.dir_port = 1;
+  mock_router_get_my_routerinfo_result = &routerinfo;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  mock_advertised_server_mode_result = 1;
+  mock_router_get_my_routerinfo_result = NULL;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  mock_advertised_server_mode_result = 1;
+  routerinfo.dir_port = 0;
+  mock_router_get_my_routerinfo_result = &routerinfo;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 0);
+  tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
+            == 0);
+
+  mock_advertised_server_mode_result = 1;
+  routerinfo.dir_port = 1;
+  mock_router_get_my_routerinfo_result = &routerinfo;
+  tt_assert(server_mode(options) == 1);
+  tt_assert(public_server_mode(options) == 1);
+  tt_assert(directory_fetches_from_authorities(options) == 1);
   tt_assert(networkstatus_consensus_can_use_multiple_directories(options)
             == 0);
 
  done:
   tor_free(options);
+  UNMOCK(router_pick_published_address);
+  UNMOCK(router_get_my_routerinfo);
+  UNMOCK(advertised_server_mode);
+  UNMOCK(router_my_exit_policy_is_reject_star);
 }
 
 static void
@@ -3539,7 +3700,7 @@ struct testcase_t config_tests[] = {
   CONFIG_TEST(check_or_create_data_subdir, TT_FORK),
   CONFIG_TEST(write_to_data_subdir, TT_FORK),
   CONFIG_TEST(fix_my_family, 0),
-  CONFIG_TEST(use_multiple_directories, 0),
+  CONFIG_TEST(directory_fetch, 0),
   END_OF_TESTCASES
 };