New consensus method to find bwweightscale & maxunmeasuredbw correctly.

Our original code for parsing these parameters out of our list of
parameters pre-dated us having the
dirvote_get_intermediate_param_value() function... and it was buggy.
Specifically, it would reject any " ... K=V ..." value
where there were additional unconverted characters after the V, and
use the default value instead,

We haven't run into this yet because we've never voted for
bwweightscale to be anything besides the default 10000, or
maxunmeasuredbw to be anything besides the default 20.

This requires a new consensus method because it is a change in how
consensuses are computed.

Fixes bug 19011; bugfix on 0.2.2.10-alpha.

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};