Merge branch 'bug21278_extra_029' into maint-0.2.9

6 files changed, 60 insertions, 43 deletions

diff --git a/changes/bug21278_extras b/changes/bug21278_extras
new file mode 100644
index 0000000000..ffdf4a047b
--- /dev/null
+++ b/changes/bug21278_extras
@@ -0,0 +1,3 @@
+  o Minor bugfixes (code correctness):
+    - Repair a couple of (unreachable or harmless) cases of the risky
+      comparison-by-subtraction pattern that caused bug 21278.
diff --git a/src/or/dirvote.c b/src/or/dirvote.c
index 2c10e784b4..738ab35bc1 100644
--- a/src/or/dirvote.c
+++ b/src/or/dirvote.c
@@ -421,16 +421,30 @@ compare_vote_rs(const vote_routerstatus_t *a, const vote_routerstatus_t *b)
                        b->status.descriptor_digest,
                        DIGEST_LEN)))
     return r;
-  if ((r = (int)(b->status.published_on - a->status.published_on)))
-    return r;
+  /* If we actually reached this point, then the identities and
+   * the descriptor digests matched, so somebody is making SHA1 collisions.
+   */
+#define CMP_FIELD(utype, itype, field) do {                             \
+    utype aval = (utype) (itype) a->status.field;                       \
+    utype bval = (utype) (itype) b->status.field;                       \
+    utype u = bval - aval;                                              \
+    itype r2 = (itype) u;                                               \
+    if (r2 < 0) {                                                       \
+      return -1;                                                        \
+    } else if (r2 > 0) {                                                \
+      return 1;                                                         \
+    }                                                                   \
+  } while (0)
+
+  CMP_FIELD(uint64_t, int64_t, published_on);
+
   if ((r = strcmp(b->status.nickname, a->status.nickname)))
     return r;
-  if ((r = (((int)b->status.addr) - ((int)a->status.addr))))
-    return r;
-  if ((r = (((int)b->status.or_port) - ((int)a->status.or_port))))
-    return r;
-  if ((r = (((int)b->status.dir_port) - ((int)a->status.dir_port))))
-    return r;
+
+  CMP_FIELD(unsigned, int, addr);
+  CMP_FIELD(unsigned, int, or_port);
+  CMP_FIELD(unsigned, int, dir_port);
+
   return 0;
 }
 
diff --git a/src/or/policies.c b/src/or/policies.c
index 6037ee311e..28770bb38d 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -1198,48 +1198,48 @@ policies_parse_from_options(const or_options_t *options)
   return ret;
 }
 
-/** Compare two provided address policy items, and return -1, 0, or 1
+/** Compare two provided address policy items, and renturn -1, 0, or 1
  * if the first is less than, equal to, or greater than the second. */
 static int
-cmp_single_addr_policy(addr_policy_t *a, addr_policy_t *b)
+single_addr_policy_eq(const addr_policy_t *a, const addr_policy_t *b)
 {
   int r;
-  if ((r=((int)a->policy_type - (int)b->policy_type)))
-    return r;
-  if ((r=((int)a->is_private - (int)b->is_private)))
-    return r;
+#define CMP_FIELD(field) do {                   \
+    if (a->field != b->field) {                 \
+      return 0;                                 \
+    }                                           \
+  } while (0)
+  CMP_FIELD(policy_type);
+  CMP_FIELD(is_private);
   /* refcnt and is_canonical are irrelevant to equality,
    * they are hash table implementation details */
   if ((r=tor_addr_compare(&a->addr, &b->addr, CMP_EXACT)))
-    return r;
-  if ((r=((int)a->maskbits - (int)b->maskbits)))
-    return r;
-  if ((r=((int)a->prt_min - (int)b->prt_min)))
-    return r;
-  if ((r=((int)a->prt_max - (int)b->prt_max)))
-    return r;
-  return 0;
+    return 0;
+  CMP_FIELD(maskbits);
+  CMP_FIELD(prt_min);
+  CMP_FIELD(prt_max);
+#undef CMP_FIELD
+  return 1;
 }
 
-/** Like cmp_single_addr_policy() above, but looks at the
- * whole set of policies in each case. */
+/** As single_addr_policy_eq, but compare every element of two policies.
+ */
 int
-cmp_addr_policies(smartlist_t *a, smartlist_t *b)
+addr_policies_eq(const smartlist_t *a, const smartlist_t *b)
 {
-  int r, i;
+  int i;
   int len_a = a ? smartlist_len(a) : 0;
   int len_b = b ? smartlist_len(b) : 0;
 
-  for (i = 0; i < len_a && i < len_b; ++i) {
-    if ((r = cmp_single_addr_policy(smartlist_get(a, i), smartlist_get(b, i))))
-      return r;
-  }
-  if (i == len_a && i == len_b)
+  if (len_a != len_b)
     return 0;
-  if (i < len_a)
-    return -1;
-  else
-    return 1;
+
+  for (i = 0; i < len_a; ++i) {
+    if (! single_addr_policy_eq(smartlist_get(a, i), smartlist_get(b, i)))
+      return 0;
+  }
+
+  return 1;
 }
 
 /** Node in hashtable used to store address policy entries. */
@@ -1255,7 +1255,7 @@ static HT_HEAD(policy_map, policy_map_ent_t) policy_root = HT_INITIALIZER();
 static inline int
 policy_eq(policy_map_ent_t *a, policy_map_ent_t *b)
 {
-  return cmp_single_addr_policy(a->policy, b->policy) == 0;
+  return single_addr_policy_eq(a->policy, b->policy);
 }
 
 /** Return a hashcode for <b>ent</b> */
@@ -1306,7 +1306,7 @@ addr_policy_get_canonical_entry(addr_policy_t *e)
     HT_INSERT(policy_map, &policy_root, found);
   }
 
-  tor_assert(!cmp_single_addr_policy(found->policy, e));
+  tor_assert(single_addr_policy_eq(found->policy, e));
   ++found->policy->refcnt;
   return found->policy;
 }
diff --git a/src/or/policies.h b/src/or/policies.h
index 850fa863d6..f73f850c21 100644
--- a/src/or/policies.h
+++ b/src/or/policies.h
@@ -76,7 +76,7 @@ void policy_expand_unspec(smartlist_t **policy);
 int policies_parse_from_options(const or_options_t *options);
 
 addr_policy_t *addr_policy_get_canonical_entry(addr_policy_t *ent);
-int cmp_addr_policies(smartlist_t *a, smartlist_t *b);
+int addr_policies_eq(const smartlist_t *a, const smartlist_t *b);
 MOCK_DECL(addr_policy_result_t, compare_tor_addr_to_addr_policy,
     (const tor_addr_t *addr, uint16_t port, const smartlist_t *policy));
 addr_policy_result_t compare_tor_addr_to_node_policy(const tor_addr_t *addr,
diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index b876795445..2365f28fd2 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -5445,7 +5445,7 @@ router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2)
       (r1->contact_info && r2->contact_info &&
        strcasecmp(r1->contact_info, r2->contact_info)) ||
       r1->is_hibernating != r2->is_hibernating ||
-      cmp_addr_policies(r1->exit_policy, r2->exit_policy) ||
+      ! addr_policies_eq(r1->exit_policy, r2->exit_policy) ||
       (r1->supports_tunnelled_dir_requests !=
        r2->supports_tunnelled_dir_requests))
     return 0;
diff --git a/src/test/test_policy.c b/src/test/test_policy.c
index f2d42b9561..1ffdc2cd51 100644
--- a/src/test/test_policy.c
+++ b/src/test/test_policy.c
@@ -304,10 +304,10 @@ test_policies_general(void *arg)
   tt_assert(!exit_policy_is_general_exit(policy10));
   tt_assert(!exit_policy_is_general_exit(policy11));
 
-  tt_assert(cmp_addr_policies(policy, policy2));
-  tt_assert(cmp_addr_policies(policy, NULL));
-  tt_assert(!cmp_addr_policies(policy2, policy2));
-  tt_assert(!cmp_addr_policies(NULL, NULL));
+  tt_assert(!addr_policies_eq(policy, policy2));
+  tt_assert(!addr_policies_eq(policy, NULL));
+  tt_assert(addr_policies_eq(policy2, policy2));
+  tt_assert(addr_policies_eq(NULL, NULL));
 
   tt_assert(!policy_is_reject_star(policy2, AF_INET, 1));
   tt_assert(policy_is_reject_star(policy, AF_INET, 1));