From 8627a40fbab223386c9d13bb63e4e5d52a795286 Mon Sep 17 00:00:00 2001
From: Ola Bini <ola@olabini.se>
Date: Fri, 29 Jan 2016 11:38:54 -0500
Subject: [PATCH 1/5] Add a helper to search for strings in the log, and change
 option tests to use this helper instead of looking at specific indices in the
 log list

---
 src/test/log_test_helpers.c |  17 ++++
 src/test/log_test_helpers.h |   1 +
 src/test/test_options.c     | 159 ++++++++++++++++++------------------
 3 files changed, 97 insertions(+), 80 deletions(-)

diff --git a/src/test/log_test_helpers.c b/src/test/log_test_helpers.c
index 51b5f9b7b1..88d28e1cc0 100644
--- a/src/test/log_test_helpers.c
+++ b/src/test/log_test_helpers.c
@@ -83,6 +83,23 @@ mock_saved_logs(void)
   return saved_logs;
 }
 
+int
+mock_saved_log_has_message(const char *msg)
+{
+  int has_msg = 0;
+  if (saved_logs) {
+    SMARTLIST_FOREACH(saved_logs, mock_saved_log_entry_t *, m,
+                      {
+                        if (msg && m->generated_msg &&
+                            !strcmp(msg, m->generated_msg)) {
+                          has_msg = 1;
+                        }
+                      });
+  }
+
+  return has_msg;
+}
+
 void
 mock_saving_logv(int severity, log_domain_mask_t domain,
                  const char *funcname, const char *suffix,
diff --git a/src/test/log_test_helpers.h b/src/test/log_test_helpers.h
index af8e8a60e7..3a565c67c5 100644
--- a/src/test/log_test_helpers.h
+++ b/src/test/log_test_helpers.h
@@ -26,6 +26,7 @@ void teardown_capture_of_logs(int prev);
 const char *mock_saved_log_at(int ix);
 int mock_saved_severity_at(int ix);
 int mock_saved_log_number(void);
+int mock_saved_log_has_message(const char *msg);
 
 #endif
 
diff --git a/src/test/test_options.c b/src/test/test_options.c
index e00c8020fe..275fee2a35 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -367,6 +367,14 @@ free_options_test_data(options_test_data_t *td)
   tor_free(td);
 }
 
+#define expect_log_msg(str) \
+  tt_assert_msg(mock_saved_log_has_message(str), \
+                "expected log to contain " # str);
+
+#define expect_no_log_msg(str)                      \
+  tt_assert_msg(!mock_saved_log_has_message(str), \
+                "expected log to not contain " # str);
+
 static void
 test_options_validate__uname_for_server(void *ignored)
 {
@@ -379,7 +387,7 @@ test_options_validate__uname_for_server(void *ignored)
   MOCK(get_uname, fixed_get_uname);
   fixed_get_uname_result = "Windows 95";
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "Tor is running as a server, but you"
+  expect_log_msg("Tor is running as a server, but you"
            " are running Windows 95; this probably won't work. See https://www"
            ".torproject.org/docs/faq.html#BestOSForRelay for details.\n");
   tor_free(msg);
@@ -387,7 +395,7 @@ test_options_validate__uname_for_server(void *ignored)
   fixed_get_uname_result = "Windows 98";
   mock_clean_saved_logs();
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "Tor is running as a server, but you"
+  expect_log_msg("Tor is running as a server, but you"
            " are running Windows 98; this probably won't work. See https://www"
            ".torproject.org/docs/faq.html#BestOSForRelay for details.\n");
   tor_free(msg);
@@ -395,7 +403,7 @@ test_options_validate__uname_for_server(void *ignored)
   fixed_get_uname_result = "Windows Me";
   mock_clean_saved_logs();
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "Tor is running as a server, but you"
+  expect_log_msg("Tor is running as a server, but you"
            " are running Windows Me; this probably won't work. See https://www"
            ".torproject.org/docs/faq.html#BestOSForRelay for details.\n");
   tor_free(msg);
@@ -512,7 +520,7 @@ test_options_validate__contactinfo(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ,
+  expect_log_msg(
             "Your ContactInfo config option is not"
             " set. Please consider setting it, so we can contact you if your"
             " server is misconfigured or something else goes wrong.\n");
@@ -524,7 +532,7 @@ test_options_validate__contactinfo(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_NE,
+  expect_no_log_msg(
             "Your ContactInfo config option is not"
             " set. Please consider setting it, so we can contact you if your"
             " server is misconfigured or something else goes wrong.\n");
@@ -632,7 +640,7 @@ test_options_validate__authdir(void *ignored)
   tt_int_op(ret, OP_EQ, -1);
   tt_str_op(msg, OP_EQ, "Failed to resolve/guess local address. See logs for"
             " details.");
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "Could not resolve local Address "
+  expect_log_msg("Could not resolve local Address "
             "'this.should.not_exist.example.org'. Failing.\n");
   tor_free(msg);
 
@@ -763,7 +771,7 @@ test_options_validate__authdir(void *ignored)
                                 "SchedulerLowWaterMark__ 10\n");
   mock_clean_saved_logs();
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "Authoritative directory servers "
+  expect_log_msg("Authoritative directory servers "
             "can't set UseEntryGuards. Disabling.\n");
   tt_int_op(tdata->opt->UseEntryGuards, OP_EQ, 0);
   tor_free(msg);
@@ -777,7 +785,7 @@ test_options_validate__authdir(void *ignored)
                                 "SchedulerLowWaterMark__ 10\n");
   mock_clean_saved_logs();
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "Authoritative directories always try"
+  expect_log_msg("Authoritative directories always try"
             " to download extra-info documents. Setting DownloadExtraInfo.\n");
   tt_int_op(tdata->opt->DownloadExtraInfo, OP_EQ, 1);
   tor_free(msg);
@@ -792,7 +800,7 @@ test_options_validate__authdir(void *ignored)
                                 "SchedulerLowWaterMark__ 10\n");
   mock_clean_saved_logs();
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(0), OP_NE, "Authoritative directories always try"
+  expect_no_log_msg("Authoritative directories always try"
             " to download extra-info documents. Setting DownloadExtraInfo.\n");
   tt_int_op(tdata->opt->DownloadExtraInfo, OP_EQ, 1);
   tor_free(msg);
@@ -935,7 +943,7 @@ test_options_validate__relay_with_hidden_services(void *ignored)
                                                      );
 
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(1), OP_EQ,
+  expect_log_msg(
             "Tor is currently configured as a relay and a hidden service. "
             "That's not very secure: you should probably run your hidden servi"
             "ce in a separate Tor process, at least -- see "
@@ -1145,7 +1153,7 @@ test_options_validate__exclude_nodes(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ,
+  expect_log_msg(
             "You have asked to exclude certain relays from all positions "
             "in your circuits. Expect hidden services and other Tor "
             "features to be broken in unpredictable ways.\n");
@@ -1158,7 +1166,7 @@ test_options_validate__exclude_nodes(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_NE,
+  expect_no_log_msg(
             "You have asked to exclude certain relays from all positions "
             "in your circuits. Expect hidden services and other Tor "
             "features to be broken in unpredictable ways.\n");
@@ -1183,8 +1191,7 @@ test_options_validate__scheduler(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ,
-            "Bad SchedulerLowWaterMark__ option\n");
+  expect_log_msg("Bad SchedulerLowWaterMark__ option\n");
   tor_free(msg);
 
   // TODO: this test cannot run on platforms where UINT32_MAX == UINT64_MAX.
@@ -1207,8 +1214,7 @@ test_options_validate__scheduler(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ,
-            "Bad SchedulerHighWaterMark option\n");
+  expect_log_msg("Bad SchedulerHighWaterMark option\n");
   tor_free(msg);
 
  done:
@@ -1283,8 +1289,7 @@ test_options_validate__tlsec(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ,
-            "Unrecognized TLSECGroup: Falling back to the default.\n");
+  expect_log_msg("Unrecognized TLSECGroup: Falling back to the default.\n");
   tt_assert(!tdata->opt->TLSECGroup);
   tor_free(msg);
 
@@ -1295,7 +1300,7 @@ test_options_validate__tlsec(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_NE,
+  expect_no_log_msg(
             "Unrecognized TLSECGroup: Falling back to the default.\n");
   tt_assert(tdata->opt->TLSECGroup);
   tor_free(msg);
@@ -1307,7 +1312,7 @@ test_options_validate__tlsec(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(0), OP_NE,
+  expect_no_log_msg(
             "Unrecognized TLSECGroup: Falling back to the default.\n");
   tt_assert(tdata->opt->TLSECGroup);
   tor_free(msg);
@@ -1360,8 +1365,7 @@ test_options_validate__recommended_packages(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_int_op(mock_saved_log_number(), OP_EQ, 1);
-  tt_str_op(mock_saved_log_at(0), OP_NE, "Invalid RecommendedPackage line "
+  expect_no_log_msg("Invalid RecommendedPackage line "
             "invalid-package-line will be ignored\n");
 
  done:
@@ -1455,9 +1459,7 @@ test_options_validate__paths_needed(void *ignored)
   tt_int_op(ret, OP_EQ, -1);
   tt_assert(tdata->opt->PathsNeededToBuildCircuits > 0.24 &&
             tdata->opt->PathsNeededToBuildCircuits < 0.26);
-  tt_int_op(mock_saved_log_number(), OP_EQ, 1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ,
-            "PathsNeededToBuildCircuits is too low. Increasing to 0.25\n");
+  expect_log_msg("PathsNeededToBuildCircuits is too low. Increasing to 0.25\n");
   tor_free(msg);
 
   free_options_test_data(tdata);
@@ -1471,8 +1473,7 @@ test_options_validate__paths_needed(void *ignored)
   tt_int_op(ret, OP_EQ, -1);
   tt_assert(tdata->opt->PathsNeededToBuildCircuits > 0.94 &&
             tdata->opt->PathsNeededToBuildCircuits < 0.96);
-  tt_int_op(mock_saved_log_number(), OP_EQ, 1);
-  tt_str_op(mock_saved_log_at(0), OP_EQ, "PathsNeededToBuildCircuits is "
+  expect_log_msg("PathsNeededToBuildCircuits is "
             "too high. Decreasing to 0.95\n");
   tor_free(msg);
 
@@ -1638,11 +1639,10 @@ test_options_validate__reachable_addresses(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_int_op(mock_saved_log_number(), OP_EQ, 6);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "Converting FascistFirewall config "
+  expect_log_msg("Converting FascistFirewall config "
             "option to new format: \"ReachableDirAddresses *:80\"\n");
   tt_str_op(tdata->opt->ReachableDirAddresses->value, OP_EQ, "*:80");
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "Converting FascistFirewall config "
+  expect_log_msg("Converting FascistFirewall config "
             "option to new format: \"ReachableORAddresses *:443\"\n");
   tt_str_op(tdata->opt->ReachableORAddresses->value, OP_EQ, "*:443");
   tor_free(msg);
@@ -1675,8 +1675,7 @@ test_options_validate__reachable_addresses(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_int_op(mock_saved_log_number(), OP_EQ, 5);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "Converting FascistFirewall and "
+  expect_log_msg("Converting FascistFirewall and "
             "FirewallPorts config options to new format: "
             "\"ReachableAddresses *:123\"\n");
   tt_str_op(tdata->opt->ReachableAddresses->value, OP_EQ, "*:123");
@@ -2056,7 +2055,7 @@ test_options_validate__publish_server_descriptor(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "Can't set a DirPort on a bridge "
+  expect_log_msg("Can't set a DirPort on a bridge "
             "relay; disabling DirPort\n");
   tt_assert(!tdata->opt->DirPort_lines);
   tt_assert(!tdata->opt->DirPort_set);
@@ -2158,7 +2157,7 @@ test_options_validate__hidserv(void *ignored)
   tdata->opt->MinUptimeHidServDirectoryV2 = -1;
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "MinUptimeHidServDirectoryV2 "
+  expect_log_msg("MinUptimeHidServDirectoryV2 "
             "option must be at least 0 seconds. Changing to 0.\n");
   tt_int_op(tdata->opt->MinUptimeHidServDirectoryV2, OP_EQ, 0);
   tor_free(msg);
@@ -2169,7 +2168,7 @@ test_options_validate__hidserv(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "RendPostPeriod option is too short;"
+  expect_log_msg("RendPostPeriod option is too short;"
             " raising to 600 seconds.\n");
   tt_int_op(tdata->opt->RendPostPeriod, OP_EQ, 600);
   tor_free(msg);
@@ -2180,7 +2179,7 @@ test_options_validate__hidserv(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "RendPostPeriod is too large; "
+  expect_log_msg("RendPostPeriod is too large; "
             "clipping to 302400s.\n");
   tt_int_op(tdata->opt->RendPostPeriod, OP_EQ, 302400);
   tor_free(msg);
@@ -2206,7 +2205,7 @@ test_options_validate__predicted_ports(void *ignored)
                                      TEST_OPTIONS_DEFAULT_VALUES);
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "PredictedPortsRelevanceTime is too "
+  expect_log_msg("PredictedPortsRelevanceTime is too "
             "large; clipping to 3600s.\n");
   tt_int_op(tdata->opt->PredictedPortsRelevanceTime, OP_EQ, 3600);
 
@@ -2433,7 +2432,7 @@ test_options_validate__circuits(void *ignored)
   tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
                                 "MaxCircuitDirtiness 2592001\n");
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "MaxCircuitDirtiness option is too "
+  expect_log_msg("MaxCircuitDirtiness option is too "
             "high; setting to 30 days.\n");
   tt_int_op(tdata->opt->MaxCircuitDirtiness, OP_EQ, 2592000);
   tor_free(msg);
@@ -2443,7 +2442,7 @@ test_options_validate__circuits(void *ignored)
   tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
                                 "CircuitStreamTimeout 1\n");
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "CircuitStreamTimeout option is too"
+  expect_log_msg("CircuitStreamTimeout option is too"
             " short; raising to 10 seconds.\n");
   tt_int_op(tdata->opt->CircuitStreamTimeout, OP_EQ, 10);
   tor_free(msg);
@@ -2453,7 +2452,7 @@ test_options_validate__circuits(void *ignored)
   tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
                                 "CircuitStreamTimeout 111\n");
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(2), OP_NE, "CircuitStreamTimeout option is too"
+  expect_no_log_msg("CircuitStreamTimeout option is too"
             " short; raising to 10 seconds.\n");
   tt_int_op(tdata->opt->CircuitStreamTimeout, OP_EQ, 111);
   tor_free(msg);
@@ -2463,7 +2462,7 @@ test_options_validate__circuits(void *ignored)
   tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
                                 "HeartbeatPeriod 1\n");
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "HeartbeatPeriod option is too short;"
+  expect_log_msg("HeartbeatPeriod option is too short;"
             " raising to 1800 seconds.\n");
   tt_int_op(tdata->opt->HeartbeatPeriod, OP_EQ, 1800);
   tor_free(msg);
@@ -2473,7 +2472,7 @@ test_options_validate__circuits(void *ignored)
   tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
                                 "HeartbeatPeriod 1982\n");
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(2), OP_NE, "HeartbeatPeriod option is too short;"
+  expect_no_log_msg("HeartbeatPeriod option is too short;"
             " raising to 1800 seconds.\n");
   tt_int_op(tdata->opt->HeartbeatPeriod, OP_EQ, 1982);
   tor_free(msg);
@@ -2484,7 +2483,7 @@ test_options_validate__circuits(void *ignored)
                                 "CircuitBuildTimeout 1\n"
                                 );
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "CircuitBuildTimeout is shorter (1"
+  expect_log_msg("CircuitBuildTimeout is shorter (1"
             " seconds) than the recommended minimum (10 seconds), and "
             "LearnCircuitBuildTimeout is disabled.  If tor isn't working, "
             "raise this value or enable LearnCircuitBuildTimeout.\n");
@@ -2496,7 +2495,7 @@ test_options_validate__circuits(void *ignored)
                                 "CircuitBuildTimeout 11\n"
                                 );
   options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
-  tt_str_op(mock_saved_log_at(1), OP_NE, "CircuitBuildTimeout is shorter (1 "
+  expect_no_log_msg("CircuitBuildTimeout is shorter (1 "
             "seconds) than the recommended minimum (10 seconds), and "
             "LearnCircuitBuildTimeout is disabled.  If tor isn't working, "
             "raise this value or enable LearnCircuitBuildTimeout.\n");
@@ -2590,7 +2589,7 @@ test_options_validate__rend(void *ignored)
                                 );
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(1), OP_EQ, "UseEntryGuards is disabled, but you"
+  expect_log_msg("UseEntryGuards is disabled, but you"
             " have configured one or more hidden services on this Tor "
             "instance.  Your hidden services will be very easy to locate using"
             " a well-known attack -- see http://freehaven.net/anonbib/#hs-"
@@ -2607,7 +2606,7 @@ test_options_validate__rend(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(1), OP_NE, "UseEntryGuards is disabled, but you"
+  expect_no_log_msg("UseEntryGuards is disabled, but you"
             " have configured one or more hidden services on this Tor "
             "instance.  Your hidden services will be very easy to locate using"
             " a well-known attack -- see http://freehaven.net/anonbib/#hs-"
@@ -2710,7 +2709,7 @@ test_options_validate__accounting(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(4), OP_EQ, "Using accounting with a hidden "
+  expect_log_msg("Using accounting with a hidden "
             "service and an ORPort is risky: your hidden service(s) and "
             "your public address will all turn off at the same time, "
             "which may alert observers that they are being run by the "
@@ -2727,7 +2726,7 @@ test_options_validate__accounting(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(4), OP_NE, "Using accounting with a hidden "
+  expect_no_log_msg("Using accounting with a hidden "
             "service and an ORPort is risky: your hidden service(s) and "
             "your public address will all turn off at the same time, "
             "which may alert observers that they are being run by the "
@@ -2746,7 +2745,7 @@ test_options_validate__accounting(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(3), OP_EQ, "Using accounting with multiple "
+  expect_log_msg("Using accounting with multiple "
             "hidden services is risky: they will all turn off at the same"
             " time, which may alert observers that they are being run by "
             "the same party.\n");
@@ -2957,7 +2956,7 @@ test_options_validate__proxy(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "HTTPProxy configured, but no SOCKS "
+  expect_log_msg("HTTPProxy configured, but no SOCKS "
             "proxy or HTTPS proxy configured. Watch out: this configuration "
             "will proxy unencrypted directory connections only.\n");
   tor_free(msg);
@@ -2970,7 +2969,7 @@ test_options_validate__proxy(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE, "HTTPProxy configured, but no SOCKS "
+  expect_no_log_msg("HTTPProxy configured, but no SOCKS "
             "proxy or HTTPS proxy configured. Watch out: this configuration "
             "will proxy unencrypted directory connections only.\n");
   tor_free(msg);
@@ -2983,7 +2982,7 @@ test_options_validate__proxy(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE, "HTTPProxy configured, but no SOCKS "
+  expect_no_log_msg("HTTPProxy configured, but no SOCKS "
             "proxy or HTTPS proxy configured. Watch out: this configuration "
             "will proxy unencrypted directory connections only.\n");
   tor_free(msg);
@@ -2996,7 +2995,7 @@ test_options_validate__proxy(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "HTTPProxy configured, but no SOCKS proxy or HTTPS proxy "
             "configured. Watch out: this configuration will proxy "
             "unencrypted directory connections only.\n");
@@ -3166,7 +3165,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ,
+  expect_log_msg(
             "ControlPort is open, but no authentication method has been "
             "configured.  This means that any program on your computer can "
             "reconfigure your Tor.  That's bad!  You should upgrade your Tor"
@@ -3182,7 +3181,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "ControlPort is open, but no authentication method has been "
             "configured.  This means that any program on your computer can "
             "reconfigure your Tor.  That's bad!  You should upgrade your Tor "
@@ -3199,7 +3198,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "ControlPort is open, but no authentication method has been "
             "configured.  This means that any program on your computer can "
             "reconfigure your Tor.  That's bad!  You should upgrade your Tor "
@@ -3214,7 +3213,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "ControlPort is open, but no authentication method has been "
             "configured.  This means that any program on your computer can "
             "reconfigure your Tor.  That's bad!  You should upgrade your Tor "
@@ -3229,7 +3228,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ,
+  expect_log_msg(
             "ControlSocket is world writable, but no authentication method has"
             " been configured.  This means that any program on your computer "
             "can reconfigure your Tor.  That's bad!  You should upgrade your "
@@ -3245,7 +3244,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "ControlSocket is world writable, but no authentication method has"
             " been configured.  This means that any program on your computer "
             "can reconfigure your Tor.  That's bad!  You should upgrade your "
@@ -3262,7 +3261,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "ControlSocket is world writable, but no authentication method has"
             " been configured.  This means that any program on your computer "
             "can reconfigure your Tor.  That's bad!  You should upgrade your "
@@ -3277,7 +3276,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "ControlSocket is world writable, but no authentication method has"
             " been configured.  This means that any program on your computer "
             "can reconfigure your Tor.  That's bad!  You should upgrade your "
@@ -3292,7 +3291,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ,
+  expect_log_msg(
             "CookieAuthFileGroupReadable is set, but will have no effect: you "
             "must specify an explicit CookieAuthFile to have it "
             "group-readable.\n");
@@ -3306,7 +3305,7 @@ test_options_validate__control(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "CookieAuthFileGroupReadable is set, but will have no effect: you "
             "must specify an explicit CookieAuthFile to have it "
             "group-readable.\n");
@@ -3343,7 +3342,7 @@ test_options_validate__families(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ,
+  expect_log_msg(
             "Listing a family for a bridge relay is not supported: it can "
             "reveal bridge fingerprints to censors. You should also make sure "
             "you aren't listing this bridge's fingerprint in any other "
@@ -3357,7 +3356,7 @@ test_options_validate__families(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "Listing a family for a bridge relay is not supported: it can "
             "reveal bridge fingerprints to censors. You should also make sure "
             "you aren't listing this bridge's fingerprint in any other "
@@ -3438,7 +3437,7 @@ test_options_validate__dir_auth(void *ignored)
   tt_str_op(msg, OP_EQ,
             "Directory authority/fallback line did not parse. See logs for "
             "details.");
-  tt_str_op(mock_saved_log_at(2), OP_EQ,
+  expect_log_msg(
             "You cannot set both DirAuthority and Alternate*Authority.\n");
   tor_free(msg);
 
@@ -3524,7 +3523,7 @@ test_options_validate__transport(void *ignored)
   tt_int_op(ret, OP_EQ, -1);
   tt_str_op(msg, OP_EQ,
             "Invalid client transport line. See logs for details.");
-  tt_str_op(mock_saved_log_at(3), OP_EQ,
+  expect_log_msg(
             "Too few arguments on ClientTransportPlugin line.\n");
   tor_free(msg);
 
@@ -3546,7 +3545,7 @@ test_options_validate__transport(void *ignored)
   tt_int_op(ret, OP_EQ, -1);
   tt_str_op(msg, OP_EQ,
             "Invalid server transport line. See logs for details.");
-  tt_str_op(mock_saved_log_at(3), OP_EQ,
+  expect_log_msg(
             "Too few arguments on ServerTransportPlugin line.\n");
   tor_free(msg);
 
@@ -3557,7 +3556,7 @@ test_options_validate__transport(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(3), OP_EQ,
+  expect_log_msg(
             "Tor is not configured as a relay but you specified a "
             "ServerTransportPlugin line (\"foo exec bar\"). The "
             "ServerTransportPlugin line will be ignored.\n");
@@ -3575,7 +3574,7 @@ test_options_validate__transport(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(3), OP_NE,
+  expect_no_log_msg(
             "Tor is not configured as a relay but you specified a "
             "ServerTransportPlugin line (\"foo exec bar\"). The "
             "ServerTransportPlugin line will be ignored.\n");
@@ -3599,7 +3598,7 @@ test_options_validate__transport(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(3), OP_EQ,
+  expect_log_msg(
             "You need at least a single managed-proxy to specify a transport "
             "listen address. The ServerTransportListenAddr line will be "
             "ignored.\n");
@@ -3618,7 +3617,7 @@ test_options_validate__transport(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(3), OP_NE,
+  expect_no_log_msg(
             "You need at least a single managed-proxy to specify a transport "
             "listen address. The ServerTransportListenAddr line will be "
             "ignored.\n");
@@ -3687,7 +3686,7 @@ test_options_validate__constrained_sockets(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "You have requested constrained "
+  expect_log_msg("You have requested constrained "
             "socket buffers while also serving directory entries via DirPort."
             "  It is strongly suggested that you disable serving directory"
             " requests when system TCP buffer resources are scarce.\n");
@@ -3701,7 +3700,7 @@ test_options_validate__constrained_sockets(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE,
+  expect_no_log_msg(
             "You have requested constrained socket buffers while also serving"
             " directory entries via DirPort.  It is strongly suggested that "
             "you disable serving directory requests when system TCP buffer "
@@ -3824,7 +3823,7 @@ test_options_validate__v3_auth(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "V3AuthVotingInterval does not divide"
+  expect_log_msg("V3AuthVotingInterval does not divide"
             " evenly into 24 hours.\n");
   tor_free(msg);
 
@@ -3837,7 +3836,7 @@ test_options_validate__v3_auth(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE, "V3AuthVotingInterval does not divide"
+  expect_no_log_msg("V3AuthVotingInterval does not divide"
             " evenly into 24 hours.\n");
   tor_free(msg);
 
@@ -3852,7 +3851,7 @@ test_options_validate__v3_auth(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(3), OP_EQ, "V3AuthVotingInterval is very low. "
+  expect_log_msg("V3AuthVotingInterval is very low. "
             "This may lead to failure to synchronise for a consensus.\n");
   tor_free(msg);
 
@@ -3985,7 +3984,7 @@ test_options_validate__exits(void *ignored)
                                 );
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_EQ, "You have set AllowSingleHopExits; "
+  expect_log_msg("You have set AllowSingleHopExits; "
             "now your relay will allow others to make one-hop exits. However,"
             " since by default most clients avoid relays that set this option,"
             " most clients will ignore you.\n");
@@ -3999,7 +3998,7 @@ test_options_validate__exits(void *ignored)
   mock_clean_saved_logs();
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, 0);
-  tt_str_op(mock_saved_log_at(2), OP_NE, "You have set AllowSingleHopExits; "
+  expect_no_log_msg("You have set AllowSingleHopExits; "
             "now your relay will allow others to make one-hop exits. However,"
             " since by default most clients avoid relays that set this option,"
             " most clients will ignore you.\n");
@@ -4044,7 +4043,7 @@ test_options_validate__testing_options(void *ignored)
   mock_clean_saved_logs();                                              \
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);\
   tt_int_op(ret, OP_EQ, 0);                                             \
-  tt_str_op(mock_saved_log_at(3), OP_EQ, #name " is insanely high.\n"); \
+  expect_log_msg( #name " is insanely high.\n"); \
   tor_free(msg); \
   STMT_END
 

From 13db39b8563b42cdf47b1feb546d33217c30c824 Mon Sep 17 00:00:00 2001
From: "teor (Tim Wilson-Brown)" <teor2345@gmail.com>
Date: Fri, 29 Jan 2016 08:13:11 +1100
Subject: [PATCH 2/5] Fix existing options_validate unit tests for
 ClientUseIPv4

---
 src/test/test_options.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/test/test_options.c b/src/test/test_options.c
index 275fee2a35..35f9f676eb 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -327,6 +327,7 @@ fixed_get_uname(void)
   "V3AuthVoteDelay 20\n"                                                \
   "V3AuthDistDelay 20\n"                                                \
   "V3AuthNIntervalsValid 3\n"                                           \
+  "ClientUseIPv4 1\n"                                                     \
   "VirtualAddrNetworkIPv4 127.192.0.0/10\n"                             \
   "VirtualAddrNetworkIPv6 [FE80::]/10\n"                                \
   "SchedulerHighWaterMark__ 42\n"                                       \
@@ -1698,6 +1699,10 @@ test_options_validate__reachable_addresses(void *ignored)
   tt_str_op(tdata->opt->ReachableAddresses->value, OP_EQ, "*:82");
   tor_free(msg);
 
+#define SERVERS_REACHABLE_MSG "Servers must be able to freely connect to" \
+  " the rest of the Internet, so they must not set Reachable*Addresses or" \
+  " FascistFirewall or FirewallPorts or ClientUseIPv4 0."
+
   free_options_test_data(tdata);
   tdata = get_options_test_data("ReachableAddresses *:82\n"
                                 "ORListenAddress 127.0.0.1:5555\n"
@@ -1709,9 +1714,7 @@ test_options_validate__reachable_addresses(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(msg, OP_EQ, "Servers must be able to freely connect to the rest of"
-            " the Internet, so they must not set Reachable*Addresses or"
-            " FascistFirewall.");
+  tt_str_op(msg, OP_EQ, SERVERS_REACHABLE_MSG);
   tor_free(msg);
 
   free_options_test_data(tdata);
@@ -1725,9 +1728,7 @@ test_options_validate__reachable_addresses(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(msg, OP_EQ, "Servers must be able to freely connect to the rest of"
-            " the Internet, so they must not set Reachable*Addresses or"
-            " FascistFirewall.");
+  tt_str_op(msg, OP_EQ, SERVERS_REACHABLE_MSG);
   tor_free(msg);
 
   free_options_test_data(tdata);
@@ -1741,9 +1742,7 @@ test_options_validate__reachable_addresses(void *ignored)
 
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(msg, OP_EQ, "Servers must be able to freely connect to the rest of"
-            " the Internet, so they must not set Reachable*Addresses or"
-            " FascistFirewall.");
+  tt_str_op(msg, OP_EQ, SERVERS_REACHABLE_MSG);
   tor_free(msg);
 
  done:
@@ -1760,6 +1759,7 @@ test_options_validate__use_bridges(void *ignored)
   char *msg;
   options_test_data_t *tdata = get_options_test_data(
                                    "UseBridges 1\n"
+                                   "ClientUseIPv4 1\n"
                                    "ORListenAddress 127.0.0.1:5555\n"
                                    "ORPort 955\n"
                                    "MaxClientCircuitsPending 1\n"

From 26f68a771c2d3df12d0dce20f37ee6549e16c920 Mon Sep 17 00:00:00 2001
From: "teor (Tim Wilson-Brown)" <teor2345@gmail.com>
Date: Fri, 29 Jan 2016 08:15:14 +1100
Subject: [PATCH 3/5] Report malformed options in options_validate unit tests

---
 src/test/test_options.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/test/test_options.c b/src/test/test_options.c
index 35f9f676eb..eb2d3326fa 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -339,22 +339,47 @@ typedef struct {
   or_options_t *def_opt;
 } options_test_data_t;
 
+static void free_options_test_data(options_test_data_t *td);
+
 static options_test_data_t *
 get_options_test_data(const char *conf)
 {
+  int rv = -1;
+  char *msg = NULL;
   config_line_t *cl=NULL;
   options_test_data_t *result = tor_malloc(sizeof(options_test_data_t));
   result->opt = options_new();
   result->old_opt = options_new();
   result->def_opt = options_new();
-  config_get_lines(conf, &cl, 1);
-  config_assign(&options_format, result->opt, cl, 0, 0, NULL);
+  rv = config_get_lines(conf, &cl, 1);
+  tt_assert(rv == 0);
+  rv = config_assign(&options_format, result->opt, cl, 0, 0, &msg);
+  if (msg) {
+    /* Display the parse error message by comparing it with an empty string */
+    tt_str_op(msg, OP_EQ, "");
+  }
+  tt_assert(rv == 0);
   config_free_lines(cl);
   result->opt->LogTimeGranularity = 1;
   result->opt->TokenBucketRefillInterval = 1;
-  config_get_lines(TEST_OPTIONS_OLD_VALUES, &cl, 1);
-  config_assign(&options_format, result->def_opt, cl, 0, 0, NULL);
+  rv = config_get_lines(TEST_OPTIONS_OLD_VALUES, &cl, 1);
+  tt_assert(rv == 0);
+  rv = config_assign(&options_format, result->def_opt, cl, 0, 0, &msg);
+  if (msg) {
+    /* Display the parse error message by comparing it with an empty string */
+    tt_str_op(msg, OP_EQ, "");
+  }
+  tt_assert(rv == 0);
+
+done:
   config_free_lines(cl);
+  if (rv != 0) {
+    free_options_test_data(result);
+    result = NULL;
+    /* Callers expect a non-NULL result, so just die if we can't provide one.
+     */
+    tor_assert(0);
+  }
   return result;
 }
 

From 1dae4dac12de391a7aea7b375628a7898168cc12 Mon Sep 17 00:00:00 2001
From: "teor (Tim Wilson-Brown)" <teor2345@gmail.com>
Date: Fri, 29 Jan 2016 09:12:07 +1100
Subject: [PATCH 4/5] Add unit tests for ClientUseIPv[4,6] and
 ClientPreferIPv6[OR,Dir]Port

---
 src/test/test_options.c | 98 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/src/test/test_options.c b/src/test/test_options.c
index eb2d3326fa..e15c88112d 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -1770,6 +1770,104 @@ test_options_validate__reachable_addresses(void *ignored)
   tt_str_op(msg, OP_EQ, SERVERS_REACHABLE_MSG);
   tor_free(msg);
 
+  free_options_test_data(tdata);
+  tdata = get_options_test_data("ClientUseIPv4 0\n"
+                                "ORListenAddress 127.0.0.1:5555\n"
+                                "ORPort 955\n"
+                                "MaxClientCircuitsPending 1\n"
+                                "ConnLimit 1\n"
+                                "SchedulerHighWaterMark__ 42\n"
+                                "SchedulerLowWaterMark__ 10\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, -1);
+  tt_str_op(msg, OP_EQ, SERVERS_REACHABLE_MSG);
+  tor_free(msg);
+
+  /* Test IPv4-only clients setting IPv6 preferences */
+
+#define WARN_PLEASE_USE_IPV6_OR_LOG_MSG \
+        "ClientPreferIPv6ORPort 1 is ignored unless tor is using IPv6. " \
+        "Please set ClientUseIPv6 1, ClientUseIPv4 0, or configure bridges.\n"
+
+#define WARN_PLEASE_USE_IPV6_DIR_LOG_MSG \
+        "ClientPreferIPv6DirPort 1 is ignored unless tor is using IPv6. " \
+        "Please set ClientUseIPv6 1, ClientUseIPv4 0, or configure bridges.\n"
+
+  free_options_test_data(tdata);
+  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
+                                "ClientUseIPv4 1\n"
+                                "ClientUseIPv6 0\n"
+                                "UseBridges 0\n"
+                                "ClientPreferIPv6ORPort 1\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, 0);
+  expect_log_msg(WARN_PLEASE_USE_IPV6_OR_LOG_MSG);
+  tor_free(msg);
+
+  free_options_test_data(tdata);
+  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
+                                "ClientUseIPv4 1\n"
+                                "ClientUseIPv6 0\n"
+                                "UseBridges 0\n"
+                                "ClientPreferIPv6DirPort 1\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, 0);
+  expect_log_msg(WARN_PLEASE_USE_IPV6_DIR_LOG_MSG);
+  tor_free(msg);
+
+  /* Now test an IPv4/IPv6 client setting IPv6 preferences */
+
+  free_options_test_data(tdata);
+  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
+                                "ClientUseIPv4 1\n"
+                                "ClientUseIPv6 1\n"
+                                "ClientPreferIPv6ORPort 1\n"
+                                "ClientPreferIPv6DirPort 1\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+
+  /* Now test an IPv6 client setting IPv6 preferences */
+
+  free_options_test_data(tdata);
+  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
+                                "ClientUseIPv6 1\n"
+                                "ClientPreferIPv6ORPort 1\n"
+                                "ClientPreferIPv6DirPort 1\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+
+  /* And an implicit (IPv4 disabled) IPv6 client setting IPv6 preferences */
+
+  free_options_test_data(tdata);
+  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
+                                "ClientUseIPv4 0\n"
+                                "ClientPreferIPv6ORPort 1\n"
+                                "ClientPreferIPv6DirPort 1\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+
+  /* And an implicit (bridge) client setting IPv6 preferences */
+
+  free_options_test_data(tdata);
+  tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES
+                                "UseBridges 1\n"
+                                "Bridge 127.0.0.1:12345\n"
+                                "ClientPreferIPv6ORPort 1\n"
+                                "ClientPreferIPv6DirPort 1\n");
+
+  ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
+  tt_int_op(ret, OP_EQ, 0);
+  tt_ptr_op(msg, OP_EQ, NULL);
+
  done:
   teardown_capture_of_logs(previous_log);
   free_options_test_data(tdata);

From b316c87bc90969b2bf724bc2dd695e3f362955b8 Mon Sep 17 00:00:00 2001
From: "teor (Tim Wilson-Brown)" <teor2345@gmail.com>
Date: Wed, 3 Feb 2016 23:52:39 +1100
Subject: [PATCH 5/5] Make bridge clients prefer the configured bridge address

When ClientPreferIPv6ORPort is auto, bridges prefer the configured
bridge ORPort address. Otherwise, they use the value of the option.
Other clients prefer IPv4 ORPorts if ClientPreferIPv6ORPort is auto.

When ClientPreferIPv6DirPort is auto, all clients prefer IPv4 DirPorts.
---
 doc/tor.1.txt              | 12 ++++++------
 src/or/entrynodes.c        | 22 ++++++++++++++++------
 src/or/nodelist.c          | 24 +++++++++++-------------
 src/or/policies.c          | 18 +++++++-----------
 src/test/test_entrynodes.c | 10 ++++++----
 src/test/test_policy.c     | 25 +++++++++++++++----------
 6 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index 87d976b2fa..4c9c53d8cb 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -1504,17 +1504,17 @@ The following options are useful only for clients (that is, if
     If this option is set to 1, Tor prefers a directory port with an IPv6
     address over one with IPv4, for direct connections, if a given directory
     server has both. (Tor also prefers an IPv6 DirPort if IPv4Client is set to
-    0.) If this option is set to auto, Tor bridge clients prefer IPv6, and
-    other clients prefer IPv4. Other things may influence the choice. This
-    option breaks a tie to the favor of IPv6. (Default: auto)
+    0.) If this option is set to auto, clients prefer IPv4. Other things may
+    influence the choice. This option breaks a tie to the favor of IPv6.
+    (Default: auto)
 
 [[ClientPreferIPv6ORPort]] **ClientPreferIPv6ORPort** **0**|**1**|**auto**::
     If this option is set to 1, Tor prefers an OR port with an IPv6
     address over one with IPv4 if a given entry node has both. (Tor also
     prefers an IPv6 ORPort if IPv4Client is set to 0.) If this option is set
-    to auto, Tor bridge clients prefer IPv6, and other clients prefer IPv4.
-    Other things may influence the choice. This option breaks a tie to the
-    favor of IPv6. (Default: auto)
+    to auto, Tor bridge clients prefer the configured bridge address, and
+    other clients prefer IPv4. Other things may influence the choice. This
+    option breaks a tie to the favor of IPv6. (Default: auto)
 
 [[PathsNeededToBuildCircuits]] **PathsNeededToBuildCircuits** __NUM__::
     Tor clients don't build circuits for user traffic until they know
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index d6bef658ff..a4b935065d 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -2240,6 +2240,7 @@ rewrite_node_address_for_bridge(const bridge_info_t *bridge, node_t *node)
    *   does so through an address from any source other than node_get_addr().
    */
   tor_addr_t addr;
+  const or_options_t *options = get_options();
 
   if (node->ri) {
     routerinfo_t *ri = node->ri;
@@ -2272,9 +2273,15 @@ rewrite_node_address_for_bridge(const bridge_info_t *bridge, node_t *node)
       }
     }
 
-    /* Mark which address to use based on which bridge_t we got. */
-    node->ipv6_preferred = (tor_addr_family(&bridge->addr) == AF_INET6 &&
-                            !tor_addr_is_null(&node->ri->ipv6_addr));
+    if (options->ClientPreferIPv6ORPort == -1) {
+      /* Mark which address to use based on which bridge_t we got. */
+      node->ipv6_preferred = (tor_addr_family(&bridge->addr) == AF_INET6 &&
+                              !tor_addr_is_null(&node->ri->ipv6_addr));
+    } else {
+      /* Mark which address to use based on user preference */
+      node->ipv6_preferred = (fascist_firewall_prefer_ipv6_orport(options) &&
+                              !tor_addr_is_null(&node->ri->ipv6_addr));
+    }
 
     /* XXXipv6 we lack support for falling back to another address for
        the same relay, warn the user */
@@ -2283,10 +2290,13 @@ rewrite_node_address_for_bridge(const bridge_info_t *bridge, node_t *node)
       node_get_pref_orport(node, &ap);
       log_notice(LD_CONFIG,
                  "Bridge '%s' has both an IPv4 and an IPv6 address.  "
-                 "Will prefer using its %s address (%s).",
+                 "Will prefer using its %s address (%s) based on %s.",
                  ri->nickname,
-                 tor_addr_family(&ap.addr) == AF_INET6 ? "IPv6" : "IPv4",
-                 fmt_addrport(&ap.addr, ap.port));
+                 node->ipv6_preferred ? "IPv6" : "IPv4",
+                 fmt_addrport(&ap.addr, ap.port),
+                 options->ClientPreferIPv6ORPort == -1 ?
+                 "the configured Bridge address" :
+                 "ClientPreferIPv6ORPort");
     }
   }
   if (node->rs) {
diff --git a/src/or/nodelist.c b/src/or/nodelist.c
index d7cada94d3..23e9b0e176 100644
--- a/src/or/nodelist.c
+++ b/src/or/nodelist.c
@@ -981,10 +981,6 @@ node_has_ipv6_dirport(const node_t *node)
  *  i) the node_t says that it prefers IPv6
  *  or
  *  ii) the router has no IPv4 OR address.
- *  or
- *  iii) our preference is for IPv6 addresses.
- *  (This extra step is needed in case our preferences have changed since
- *  node->ipv6_preferred was set at the time the consensus was loaded.)
  */
 int
 node_ipv6_or_preferred(const node_t *node)
@@ -993,10 +989,12 @@ node_ipv6_or_preferred(const node_t *node)
   tor_addr_port_t ipv4_addr;
   node_assert_ok(node);
 
+  /* XX/teor - node->ipv6_preferred is set from
+   * fascist_firewall_prefer_ipv6_orport() each time the consensus is loaded.
+   */
   if (!fascist_firewall_use_ipv6(options)) {
     return 0;
-  } else if (node->ipv6_preferred || node_get_prim_orport(node, &ipv4_addr)
-      || fascist_firewall_prefer_ipv6_orport(get_options())) {
+  } else if (node->ipv6_preferred || node_get_prim_orport(node, &ipv4_addr)) {
     return node_has_ipv6_orport(node);
   }
   return 0;
@@ -1077,13 +1075,9 @@ node_get_pref_ipv6_orport(const node_t *node, tor_addr_port_t *ap_out)
  *
  *  We prefer the IPv6 address if the router has an IPv6 address,
  *  and we can use IPv6 addresses, and:
- *  i) the node_t says that it prefers IPv6
+ *  i) the router has no IPv4 Dir address.
  *  or
- *  ii) the router has no IPv4 Dir address.
- *  or
- *  iii) our preference is for IPv6 addresses.
- *  (This extra step is needed in case our preferences have changed since
- *  node->ipv6_preferred was set at the time the consensus was loaded.)
+ *  ii) our preference is for IPv6 Dir addresses.
  */
 int
 node_ipv6_dir_preferred(const node_t *node)
@@ -1092,9 +1086,13 @@ node_ipv6_dir_preferred(const node_t *node)
   tor_addr_port_t ipv4_addr;
   node_assert_ok(node);
 
+  /* node->ipv6_preferred is set from fascist_firewall_prefer_ipv6_orport(),
+   * so we can't use it to determine DirPort IPv6 preference.
+   * This means that bridge clients will use IPv4 DirPorts by default.
+   */
   if (!fascist_firewall_use_ipv6(options)) {
     return 0;
-  } else if (node->ipv6_preferred || node_get_prim_dirport(node, &ipv4_addr)
+  } else if (node_get_prim_dirport(node, &ipv4_addr)
       || fascist_firewall_prefer_ipv6_dirport(get_options())) {
     return node_has_ipv6_dirport(node);
   }
diff --git a/src/or/policies.c b/src/or/policies.c
index 734558d836..e2cece56e4 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -458,6 +458,13 @@ fascist_firewall_prefer_ipv6_impl(const or_options_t *options)
 int
 fascist_firewall_prefer_ipv6_orport(const or_options_t *options)
 {
+  /* node->ipv6_preferred is set from fascist_firewall_prefer_ipv6_orport()
+   * each time the consensus is loaded.
+   * If our preferences change, we will only reset ipv6_preferred on the node
+   * when the next consensus is loaded. But the consensus is realoded when the
+   * configuration changes after a HUP. So as long as the result of this
+   * function only depends on Tor's options, everything should work ok.
+   */
   int pref_ipv6 = fascist_firewall_prefer_ipv6_impl(options);
 
   if (pref_ipv6 >= 0) {
@@ -469,11 +476,6 @@ fascist_firewall_prefer_ipv6_orport(const or_options_t *options)
     return 1;
   }
 
-  /* For bridge clients, ClientPreferIPv6ORPort auto means "prefer IPv6". */
-  if (options->UseBridges && options->ClientPreferIPv6ORPort != 0) {
-    return 1;
-  }
-
   return 0;
 }
 
@@ -493,12 +495,6 @@ fascist_firewall_prefer_ipv6_dirport(const or_options_t *options)
     return 1;
   }
 
-  /* For bridge clients, ClientPreferIPv6ORPort auto means "prefer IPv6".
-   * XX/teor - do bridge clients ever use a DirPort? */
-  if (options->UseBridges && options->ClientPreferIPv6DirPort != 0) {
-    return 1;
-  }
-
   return 0;
 }
 
diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c
index 14baa8c9bf..fd19db095d 100644
--- a/src/test/test_entrynodes.c
+++ b/src/test/test_entrynodes.c
@@ -9,14 +9,16 @@
 
 #include "or.h"
 #include "test.h"
+
+#include "config.h"
 #include "entrynodes.h"
-#include "routerparse.h"
 #include "nodelist.h"
-#include "util.h"
+#include "policies.h"
 #include "routerlist.h"
+#include "routerparse.h"
 #include "routerset.h"
 #include "statefile.h"
-#include "config.h"
+#include "util.h"
 
 #include "test_helpers.h"
 
@@ -826,7 +828,7 @@ test_node_preferred_orport(void *arg)
    * ClientUseIPv4 is 0 */
   mocked_options.ClientUseIPv4 = 0;
   mocked_options.ClientUseIPv6 = 1;
-  node.ipv6_preferred = 0;
+  node.ipv6_preferred = fascist_firewall_prefer_ipv6_orport(&mocked_options);
   node_get_pref_orport(&node, &ap);
   tt_assert(tor_addr_eq(&ap.addr, &ipv6_addr));
   tt_assert(ap.port == ipv6_port);
diff --git a/src/test/test_policy.c b/src/test/test_policy.c
index b4cbfb2579..c044d9f210 100644
--- a/src/test/test_policy.c
+++ b/src/test/test_policy.c
@@ -1510,21 +1510,25 @@ test_policies_fascist_firewall_choose_address(void *arg)
                                             FIREWALL_DIR_CONNECTION, 1)
             == &ipv4_dir_ap);
 
-  /* Auto (Preferring IPv6 for bridge clients) */
+  /* Auto:
+   * - bridge clients prefer the configured bridge OR address,
+   * - other clients prefer IPv4 OR by default,
+   * - all clients prefer IPv4 Dir by default.
+   */
   mock_options.ClientPreferIPv6ORPort = -1;
   mock_options.ClientPreferIPv6DirPort = -1;
   tt_assert(fascist_firewall_choose_address(&ipv4_or_ap, &ipv6_or_ap, 0,
                                             FIREWALL_OR_CONNECTION, 0)
-            == &ipv6_or_ap);
+            == &ipv4_or_ap);
   tt_assert(fascist_firewall_choose_address(&ipv4_or_ap, &ipv6_or_ap, 0,
                                             FIREWALL_OR_CONNECTION, 1)
-            == &ipv6_or_ap);
+            == &ipv4_or_ap);
   tt_assert(fascist_firewall_choose_address(&ipv4_dir_ap, &ipv6_dir_ap, 0,
                                             FIREWALL_DIR_CONNECTION, 0)
-            == &ipv6_dir_ap);
+            == &ipv4_dir_ap);
   tt_assert(fascist_firewall_choose_address(&ipv4_dir_ap, &ipv6_dir_ap, 0,
                                             FIREWALL_DIR_CONNECTION, 1)
-            == &ipv6_dir_ap);
+            == &ipv4_dir_ap);
 
   /* Preferring IPv6 */
   mock_options.ClientPreferIPv6ORPort = 1;
@@ -1544,22 +1548,23 @@ test_policies_fascist_firewall_choose_address(void *arg)
 
 
   /* In the default configuration (Auto / IPv6 off), bridge clients should
-   * still use and prefer IPv6 regardless of ClientUseIPv6. */
+   * still use IPv6, and only prefer it for bridges configured with an IPv6
+   * address, regardless of ClientUseIPv6. */
   mock_options.ClientUseIPv6 = 0;
   mock_options.ClientPreferIPv6ORPort = -1;
   mock_options.ClientPreferIPv6DirPort = -1;
   tt_assert(fascist_firewall_choose_address(&ipv4_or_ap, &ipv6_or_ap, 0,
                                             FIREWALL_OR_CONNECTION, 0)
-            == &ipv6_or_ap);
+            == &ipv4_or_ap);
   tt_assert(fascist_firewall_choose_address(&ipv4_or_ap, &ipv6_or_ap, 0,
                                             FIREWALL_OR_CONNECTION, 1)
-            == &ipv6_or_ap);
+            == &ipv4_or_ap);
   tt_assert(fascist_firewall_choose_address(&ipv4_dir_ap, &ipv6_dir_ap, 0,
                                             FIREWALL_DIR_CONNECTION, 0)
-            == &ipv6_dir_ap);
+            == &ipv4_dir_ap);
   tt_assert(fascist_firewall_choose_address(&ipv4_dir_ap, &ipv6_dir_ap, 0,
                                             FIREWALL_DIR_CONNECTION, 1)
-            == &ipv6_dir_ap);
+            == &ipv4_dir_ap);
 
   /* Choose an address with IPv4 on */
   memset(&mock_options, 0, sizeof(or_options_t));