Merge branch 'ticket20218_rebased_squashed' into ticket20218_merged

* ticket 32695 removed networkstatus_consensus_has_ipv6(),
  keep that change in master.
* ticket 20218 modifies the function name and comment for
  routerstatus_has_visibly_changed(), keep that change
  in ticket20218_rebased_squashed.

6 files changed, 198 insertions, 8 deletions

diff --git a/changes/ticket20218 b/changes/ticket20218
new file mode 100644
index 0000000000..d5fb2b2cfd
--- /dev/null
+++ b/changes/ticket20218
@@ -0,0 +1,3 @@
+  o Minor bugfixes (controller):
+    - In routerstatus_has_changed(), check all the fields that are output over the control port.
+      Fixes bug 20218; bugfix on 0.1.1.11-alpha
diff --git a/src/feature/nodelist/fmt_routerstatus.c b/src/feature/nodelist/fmt_routerstatus.c
index b3f1c6962d..0cf4a6eeab 100644
--- a/src/feature/nodelist/fmt_routerstatus.c
+++ b/src/feature/nodelist/fmt_routerstatus.c
@@ -113,6 +113,8 @@ routerstatus_format_entry(const routerstatus_t *rs, const char *version,
     if (format != NS_CONTROL_PORT) {
       /* Blow up more or less nicely if we didn't get anything or not the
        * thing we expected.
+       * This should be kept in sync with the function
+       * routerstatus_has_visibly_changed and the struct routerstatus_t
        */
       if (!desc) {
         char id[HEX_DIGEST_LEN+1];
diff --git a/src/feature/nodelist/networkstatus.c b/src/feature/nodelist/networkstatus.c
index 58fad49a82..0d2ff96a6e 100644
--- a/src/feature/nodelist/networkstatus.c
+++ b/src/feature/nodelist/networkstatus.c
@@ -102,6 +102,7 @@
 #include "feature/nodelist/routerlist_st.h"
 #include "feature/dirauth/vote_microdesc_hash_st.h"
 #include "feature/nodelist/vote_routerstatus_st.h"
+#include "routerstatus_st.h"
 
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
@@ -1578,10 +1579,16 @@ networkstatus_consensus_is_already_downloading(const char *resource)
   return answer;
 }
 
-/** Given two router status entries for the same router identity, return 1 if
- * if the contents have changed between them. Otherwise, return 0. */
-static int
-routerstatus_has_changed(const routerstatus_t *a, const routerstatus_t *b)
+/** Given two router status entries for the same router identity, return 1
+ * if the contents have changed between them. Otherwise, return 0.
+ * It only checks for fields that are output by control port.
+ * This should be kept in sync with the struct routerstatus_t
+ * and the printing function routerstatus_format_entry in
+ * NS_CONTROL_PORT mode.
+ **/
+STATIC int
+routerstatus_has_visibly_changed(const routerstatus_t *a,
+                                 const routerstatus_t *b)
 {
   tor_assert(tor_memeq(a->identity_digest, b->identity_digest, DIGEST_LEN));
 
@@ -1600,9 +1607,14 @@ routerstatus_has_changed(const routerstatus_t *a, const routerstatus_t *b)
          a->is_valid != b->is_valid ||
          a->is_possible_guard != b->is_possible_guard ||
          a->is_bad_exit != b->is_bad_exit ||
-         a->is_hs_dir != b->is_hs_dir;
-  // XXXX this function needs a huge refactoring; it has gotten out
-  // XXXX of sync with routerstatus_t, and it will do so again.
+         a->is_hs_dir != b->is_hs_dir ||
+         a->is_staledesc != b->is_staledesc ||
+         a->has_bandwidth != b->has_bandwidth ||
+         a->published_on != b->published_on ||
+         a->ipv6_orport != b->ipv6_orport ||
+         a->is_v2_dir != b->is_v2_dir ||
+         a->bandwidth_kb != b->bandwidth_kb ||
+         tor_addr_compare(&a->ipv6_addr, &b->ipv6_addr, CMP_EXACT);
 }
 
 /** Notify controllers of any router status entries that changed between
@@ -1634,7 +1646,7 @@ notify_control_networkstatus_changed(const networkstatus_t *old_c,
                      tor_memcmp(rs_old->identity_digest,
                             rs_new->identity_digest, DIGEST_LEN),
                      smartlist_add(changed, (void*) rs_new)) {
-    if (routerstatus_has_changed(rs_old, rs_new))
+    if (routerstatus_has_visibly_changed(rs_old, rs_new))
       smartlist_add(changed, (void*)rs_new);
   } SMARTLIST_FOREACH_JOIN_END(rs_old, rs_new);
 
diff --git a/src/feature/nodelist/networkstatus.h b/src/feature/nodelist/networkstatus.h
index 38929fa6b6..5e8c8a9e57 100644
--- a/src/feature/nodelist/networkstatus.h
+++ b/src/feature/nodelist/networkstatus.h
@@ -163,6 +163,8 @@ STATIC void warn_early_consensus(const networkstatus_t *c, const char *flavor,
 extern networkstatus_t *current_ns_consensus;
 extern networkstatus_t *current_md_consensus;
 #endif /* defined(TOR_UNIT_TESTS) */
+STATIC int routerstatus_has_visibly_changed(const routerstatus_t *a,
+                                    const routerstatus_t *b);
 #endif /* defined(NETWORKSTATUS_PRIVATE) */
 
 #endif /* !defined(TOR_NETWORKSTATUS_H) */
diff --git a/src/feature/nodelist/routerstatus_st.h b/src/feature/nodelist/routerstatus_st.h
index 270cb871a2..735c754b31 100644
--- a/src/feature/nodelist/routerstatus_st.h
+++ b/src/feature/nodelist/routerstatus_st.h
@@ -17,6 +17,10 @@
 /** Contents of a single router entry in a network status object.
  */
 struct routerstatus_t {
+ /* This should be kept in sync with the function
+ * routerstatus_has_visibly_changed and the printing function
+ * routerstatus_format_entry in NS_CONTROL_PORT mode.
+ */
   time_t published_on; /**< When was this router published? */
   char nickname[MAX_NICKNAME_LEN+1]; /**< The nickname this router says it
                                       * has. */
diff --git a/src/test/test_nodelist.c b/src/test/test_nodelist.c
index dc7faee5be..388cd74f3b 100644
--- a/src/test/test_nodelist.c
+++ b/src/test/test_nodelist.c
@@ -7,6 +7,7 @@
  **/
 
 #define NODELIST_PRIVATE
+#define NETWORKSTATUS_PRIVATE
 
 #include "core/or/or.h"
 #include "lib/crypt_ops/crypto_rand.h"
@@ -17,6 +18,8 @@
 #include "feature/nodelist/torcert.h"
 
 #include "core/or/extend_info_st.h"
+#include "feature/dirauth/dirvote.h"
+#include "feature/nodelist/fmt_routerstatus.h"
 #include "feature/nodelist/microdesc_st.h"
 #include "feature/nodelist/networkstatus_st.h"
 #include "feature/nodelist/node_st.h"
@@ -1246,6 +1249,169 @@ test_nodelist_router_get_verbose_nickname(void *arg)
   return;
 }
 
+static void
+test_nodelist_routerstatus_has_visibly_changed(void *arg)
+{
+  (void)arg;
+  routerstatus_t rs_orig, rs;
+  char *fmt_orig = NULL, *fmt = NULL;
+  memset(&rs_orig, 0, sizeof(rs_orig));
+  strlcpy(rs_orig.nickname, "friendly", sizeof(rs_orig.nickname));
+  memcpy(rs_orig.identity_digest, "abcdefghijklmnopqrst", 20);
+  memcpy(rs_orig.descriptor_digest, "abcdefghijklmnopqrst", 20);
+  rs_orig.addr = 0x7f000001;
+  rs_orig.or_port = 3;
+  rs_orig.published_on = time(NULL);
+  rs_orig.has_bandwidth = 1;
+  rs_orig.bandwidth_kb = 20;
+
+#define COPY() memcpy(&rs, &rs_orig, sizeof(rs))
+#define FORMAT() \
+  STMT_BEGIN \
+    tor_free(fmt_orig);                                                   \
+    tor_free(fmt);                                                        \
+    fmt_orig = routerstatus_format_entry(&rs_orig, NULL, NULL,            \
+                          NS_CONTROL_PORT,                                \
+                          ROUTERSTATUS_FORMAT_NO_CONSENSUS_METHOD,        \
+                          NULL);                                          \
+    fmt = routerstatus_format_entry(&rs, NULL, NULL, NS_CONTROL_PORT,     \
+                          ROUTERSTATUS_FORMAT_NO_CONSENSUS_METHOD,        \
+                          NULL);                                          \
+    tt_assert(fmt_orig);                                                  \
+    tt_assert(fmt);                                                       \
+  STMT_END
+#define ASSERT_SAME() \
+  STMT_BEGIN                                                    \
+    tt_assert(! routerstatus_has_visibly_changed(&rs_orig, &rs));       \
+    FORMAT();                                                   \
+    tt_str_op(fmt_orig, OP_EQ, fmt);                            \
+    COPY();                                                     \
+  STMT_END
+#define ASSERT_CHANGED() \
+  STMT_BEGIN                                                    \
+    tt_assert(routerstatus_has_visibly_changed(&rs_orig, &rs));         \
+    FORMAT();                                                   \
+    tt_str_op(fmt_orig, OP_NE, fmt);                            \
+    COPY();                                                     \
+  STMT_END
+#define ASSERT_CHANGED_NO_FORMAT() \
+  STMT_BEGIN                                                    \
+    tt_assert(routerstatus_has_visibly_changed(&rs_orig, &rs));         \
+    COPY();                                                     \
+  STMT_END
+
+  COPY();
+  ASSERT_SAME();
+
+  rs.addr = 0x7f000002;
+  ASSERT_CHANGED();
+
+  strlcpy(rs.descriptor_digest, "hello world", sizeof(rs.descriptor_digest));
+  ASSERT_CHANGED();
+
+  strlcpy(rs.nickname, "fr1end1y", sizeof(rs.nickname));
+  ASSERT_CHANGED();
+
+  rs.published_on += 3600;
+  ASSERT_CHANGED();
+
+  rs.or_port = 55;
+  ASSERT_CHANGED();
+
+  rs.dir_port = 9999;
+  ASSERT_CHANGED();
+
+  tor_addr_parse(&rs.ipv6_addr, "1234::56");
+  ASSERT_CHANGED();
+
+  tor_addr_parse(&rs_orig.ipv6_addr, "1234::56");
+  rs_orig.ipv6_orport = 99;
+  COPY();
+  rs.ipv6_orport = 22;
+  ASSERT_CHANGED();
+
+  rs.is_authority = 1;
+  ASSERT_CHANGED();
+
+  rs.is_exit = 1;
+  ASSERT_CHANGED();
+
+  rs.is_stable = 1;
+  ASSERT_CHANGED();
+
+  rs.is_fast = 1;
+  ASSERT_CHANGED();
+
+  rs.is_flagged_running = 1;
+  ASSERT_CHANGED();
+
+  // This option is obsolete and not actually formatted.
+  rs.is_named = 1;
+  ASSERT_CHANGED_NO_FORMAT();
+
+  // This option is obsolete and not actually formatted.
+  rs.is_unnamed = 1;
+  ASSERT_CHANGED_NO_FORMAT();
+
+  rs.is_valid = 1;
+  ASSERT_CHANGED();
+
+  rs.is_possible_guard = 1;
+  ASSERT_CHANGED();
+
+  rs.is_bad_exit = 1;
+  ASSERT_CHANGED();
+
+  rs.is_hs_dir = 1;
+  ASSERT_CHANGED();
+
+  rs.is_v2_dir = 1;
+  ASSERT_CHANGED();
+
+  rs.is_staledesc = 1;
+  ASSERT_CHANGED();
+
+  // Setting this to zero crashes us with an assertion failure in
+  // routerstatus_format_entry() if we don't have a descriptor.
+  rs.has_bandwidth = 0;
+  ASSERT_CHANGED_NO_FORMAT();
+
+  // Does not actually matter; not visible to controller.
+  rs.has_exitsummary = 1;
+  ASSERT_SAME();
+
+  // Does not actually matter; not visible to the controller.
+  rs.bw_is_unmeasured = 1;
+  ASSERT_SAME();
+
+  rs.bandwidth_kb = 2000;
+  ASSERT_CHANGED();
+
+  // not visible to the controller.
+  rs.has_guardfraction = 1;
+  rs.guardfraction_percentage = 22;
+  ASSERT_SAME();
+
+  // not visible to the controller.
+  rs_orig.has_guardfraction = 1;
+  rs_orig.guardfraction_percentage = 20;
+  COPY();
+  rs.guardfraction_percentage = 25;
+  ASSERT_SAME();
+
+  // not visible to the controller.
+  rs.exitsummary = (char*)"accept 1-2";
+  ASSERT_SAME();
+
+ done:
+#undef COPY
+#undef ASSERT_SAME
+#undef ASSERT_CHANGED
+  tor_free(fmt_orig);
+  tor_free(fmt);
+  return;
+}
+
 #define NODE(name, flags) \
   { #name, test_nodelist_##name, (flags), NULL, NULL }
 
@@ -1266,5 +1432,6 @@ struct testcase_t nodelist_tests[] = {
   NODE(routerstatus_describe, 0),
   NODE(extend_info_describe, 0),
   NODE(router_get_verbose_nickname, 0),
+  NODE(routerstatus_has_visibly_changed, 0),
   END_OF_TESTCASES
 };