Merge branch 'mr/236'

changes/bug19011

@@ -0,0 +1,7 @@
+  o Minor bugfixes (directory authorities, voting):
+    - Add a new consensus method (31) to support any future changes that
+      authorities decide to make to the value of bwweightscale or
+      maxunmeasuredbw. Previously, there was a bug that prevented the
+      authorities from parsing these consensus parameters correctly under
+      most circumstances.
+      Fixes bug 19011; bugfix on 0.2.2.10-alpha.

src/feature/dirauth/dirvote.c

@@ -1757,26 +1757,14 @@ networkstatus_compute_consensus(smartlist_t *votes,
   }
 
   {
-    char *max_unmeasured_param = NULL;
-    /* XXXX Extract this code into a common function.  Or don't!  see #19011 */
-    if (params) {
-      if (strcmpstart(params, "maxunmeasuredbw=") == 0)
-        max_unmeasured_param = params;
-      else
-        max_unmeasured_param = strstr(params, " maxunmeasuredbw=");
-    }
-    if (max_unmeasured_param) {
-      int ok = 0;
-      char *eq = strchr(max_unmeasured_param, '=');
-      if (eq) {
-        max_unmeasured_bw_kb = (uint32_t)
-          tor_parse_ulong(eq+1, 10, 1, UINT32_MAX, &ok, NULL);
-        if (!ok) {
-          log_warn(LD_DIR, "Bad element '%s' in max unmeasured bw param",
-                   escaped(max_unmeasured_param));
-          max_unmeasured_bw_kb = DEFAULT_MAX_UNMEASURED_BW_KB;
-        }
-      }
+    if (consensus_method < MIN_METHOD_FOR_CORRECT_BWWEIGHTSCALE) {
+      max_unmeasured_bw_kb = (int32_t) extract_param_buggy(
+                  params, "maxunmeasuredbw", DEFAULT_MAX_UNMEASURED_BW_KB);
+    } else {
+      max_unmeasured_bw_kb = dirvote_get_intermediate_param_value(
+                  param_list, "maxunmeasurdbw", DEFAULT_MAX_UNMEASURED_BW_KB);
+      if (max_unmeasured_bw_kb < 1)
+        max_unmeasured_bw_kb = 1;
     }
   }
 
@@ -2326,38 +2314,16 @@ networkstatus_compute_consensus(smartlist_t *votes,
   smartlist_add_strdup(chunks, "directory-footer\n");
 
   {
-    int64_t weight_scale = BW_WEIGHT_SCALE;
-    char *bw_weight_param = NULL;
-
-    // Parse params, extract BW_WEIGHT_SCALE if present
-    // DO NOT use consensus_param_bw_weight_scale() in this code!
-    // The consensus is not formed yet!
-    /* XXXX Extract this code into a common function. Or not: #19011. */
-    if (params) {
-      if (strcmpstart(params, "bwweightscale=") == 0)
-        bw_weight_param = params;
-      else
-        bw_weight_param = strstr(params, " bwweightscale=");
+    int64_t weight_scale;
+    if (consensus_method < MIN_METHOD_FOR_CORRECT_BWWEIGHTSCALE) {
+      weight_scale = extract_param_buggy(params, "bwweightscale",
+                                         BW_WEIGHT_SCALE);
+    } else {
+      weight_scale = dirvote_get_intermediate_param_value(
+                       param_list, "bwweightscale", BW_WEIGHT_SCALE);
+      if (weight_scale < 1)
+        weight_scale = 1;
     }
-
-    if (bw_weight_param) {
-      int ok=0;
-      char *eq = strchr(bw_weight_param, '=');
-      if (eq) {
-        weight_scale = tor_parse_long(eq+1, 10, 1, INT32_MAX, &ok,
-                                         NULL);
-        if (!ok) {
-          log_warn(LD_DIR, "Bad element '%s' in bw weight param",
-              escaped(bw_weight_param));
-          weight_scale = BW_WEIGHT_SCALE;
-        }
-      } else {
-        log_warn(LD_DIR, "Bad element '%s' in bw weight param",
-            escaped(bw_weight_param));
-        weight_scale = BW_WEIGHT_SCALE;
-      }
-    }
-
     added_weights = networkstatus_compute_bw_weights_v10(chunks, G, M, E, D,
                                                          T, weight_scale);
   }
@@ -2459,6 +2425,53 @@ networkstatus_compute_consensus(smartlist_t *votes,
   return result;
 }
 
+/** Extract the value of a parameter from a string encoding a list of
+ * parameters, badly.
+ *
+ * This is a deliberately buggy implementation, for backward compatibility
+ * with versions of Tor affected by #19011.  Once all authorities have
+ * upgraded to consensus method 31 or later, then we can throw away this
+ * function.  */
+STATIC int64_t
+extract_param_buggy(const char *params,
+                    const char *param_name,
+                    int64_t default_value)
+{
+  int64_t value = default_value;
+  const char *param_str = NULL;
+
+  if (params) {
+    char *prefix1 = NULL, *prefix2=NULL;
+    tor_asprintf(&prefix1, "%s=", param_name);
+    tor_asprintf(&prefix2, " %s=", param_name);
+    if (strcmpstart(params, prefix1) == 0)
+      param_str = params;
+    else
+      param_str = strstr(params, prefix2);
+    tor_free(prefix1);
+    tor_free(prefix2);
+  }
+
+  if (param_str) {
+    int ok=0;
+    char *eq = strchr(param_str, '=');
+    if (eq) {
+      value = tor_parse_long(eq+1, 10, 1, INT32_MAX, &ok, NULL);
+      if (!ok) {
+        log_warn(LD_DIR, "Bad element '%s' in %s",
+                 escaped(param_str), param_name);
+        value = default_value;
+      }
+    } else {
+      log_warn(LD_DIR, "Bad element '%s' in %s",
+               escaped(param_str), param_name);
+      value = default_value;
+    }
+  }
+
+  return value;
+}
+
 /** Given a list of networkstatus_t for each vote, return a newly allocated
  * string containing the "package" lines for the vote. */
 STATIC char *

src/feature/dirauth/dirvote.h

@@ -53,7 +53,7 @@
 #define MIN_SUPPORTED_CONSENSUS_METHOD 28
 
 /** The highest consensus method that we currently support. */
-#define MAX_SUPPORTED_CONSENSUS_METHOD 30
+#define MAX_SUPPORTED_CONSENSUS_METHOD 31
 
 /**
  * Lowest consensus method where microdescriptor lines are put in canonical
@@ -65,6 +65,11 @@
  * See #7869 */
 #define MIN_METHOD_FOR_UNPADDED_NTOR_KEY 30
 
+/** Lowest consensus method for which we use the correct algorithm for
+ * extracting the bwweightscale= and maxunmeasuredbw= parameters. See #19011.
+ */
+#define MIN_METHOD_FOR_CORRECT_BWWEIGHTSCALE 31
+
 /** Default bandwidth to clip unmeasured bandwidths to using method >=
  * MIN_METHOD_TO_CLIP_UNMEASURED_BW.  (This is not a consensus method; do not
  * get confused with the above macros.) */
@@ -256,6 +261,9 @@ STATIC
 char *networkstatus_get_detached_signatures(smartlist_t *consensuses);
 STATIC microdesc_t *dirvote_create_microdescriptor(const routerinfo_t *ri,
                                                    int consensus_method);
+STATIC int64_t extract_param_buggy(const char *params,
+                                   const char *param_name,
+                                   int64_t default_value);
 
 /** The recommended relay protocols for this authority's votes.
  * Recommending a new protocol causes old tor versions to log a warning.

src/test/test_dirvote.c

@@ -656,6 +656,30 @@ done:
   ROUTER_FREE(pppp);
 }
 
+static void
+test_dirvote_parse_param_buggy(void *arg)
+{
+  (void)arg;
+
+  /* Tests for behavior with bug emulation to migrate away from bug 19011. */
+  tt_i64_op(extract_param_buggy("blah blah", "bwweightscale", 10000),
+            OP_EQ, 10000);
+  tt_i64_op(extract_param_buggy("bwweightscale=7", "bwweightscale", 10000),
+            OP_EQ, 7);
+  tt_i64_op(extract_param_buggy("bwweightscale=7 foo=9",
+                                "bwweightscale", 10000),
+            OP_EQ, 10000);
+  tt_i64_op(extract_param_buggy("foo=7 bwweightscale=777 bar=9",
+                                "bwweightscale", 10000),
+            OP_EQ, 10000);
+  tt_i64_op(extract_param_buggy("foo=7 bwweightscale=1234",
+                                "bwweightscale", 10000),
+            OP_EQ, 1234);
+
+ done:
+  ;
+}
+
 #define NODE(name, flags)                           \
   {                                                 \
     #name, test_dirvote_##name, (flags), NULL, NULL \
@@ -668,4 +692,5 @@ struct testcase_t dirvote_tests[] = {
     NODE(get_sybil_by_ip_version_ipv4, TT_FORK),
     NODE(get_sybil_by_ip_version_ipv6, TT_FORK),
     NODE(get_all_possible_sybil, TT_FORK),
+    NODE(parse_param_buggy, 0),
     END_OF_TESTCASES};