Merge remote-tracking branch 'origin/maint-0.2.3'

changes/bug7192

@@ -0,0 +1,10 @@
+  o Major bugfixes:
+    - When parsing exit policy summaries from microdescriptors, we had
+      previously been ignoring the last character in each one, so that
+      "accept 80,443,8080" would be treated by clients as indicating a
+      node that allows access to ports 80, 443, and 808. That would lead
+      to clients attempting connections that could never work, and
+      ignoring exit nodes that would support their connections. Now clients
+      parse these exit policy summaries correctly. Fixes bug 7192;
+      bugfix on 0.2.3.1-alpha.
+

src/or/policies.c

@@ -1347,8 +1347,10 @@ parse_short_policy(const char *summary)
     unsigned low, high;
     char dummy;
     char ent_buf[32];
+    size_t len;
 
     next = comma ? comma+1 : strchr(summary, '\0');
+    len = comma ? (size_t)(comma - summary) : strlen(summary);
 
     if (n_entries == MAX_EXITPOLICY_SUMMARY_LEN) {
       log_fn(LOG_PROTOCOL_WARN, LD_DIR, "Impossibly long policy summary %s",
@@ -1356,20 +1358,22 @@ parse_short_policy(const char *summary)
       return NULL;
     }
 
-    if (! TOR_ISDIGIT(*summary) || next-summary > (int)(sizeof(ent_buf)-1)) {
+    if (! TOR_ISDIGIT(*summary) || len > (sizeof(ent_buf)-1)) {
       /* unrecognized entry format. skip it. */
       continue;
     }
-    if (next-summary < 2) {
+    if (len < 1) {
       /* empty; skip it. */
+      /* XXX This happens to be unreachable, since if len==0, then *summary is
+       * ',' or '\0', and the TOR_ISDIGIT test above would have failed. */
       continue;
     }
 
-    memcpy(ent_buf, summary, next-summary-1);
-    ent_buf[next-summary-1] = '\0';
+    memcpy(ent_buf, summary, len);
+    ent_buf[len] = '\0';
 
     if (tor_sscanf(ent_buf, "%u-%u%c", &low, &high, &dummy) == 2) {
-      if (low<1 || low>65535 || high<1 || high>65535) {
+      if (low<1 || low>65535 || high<1 || high>65535 || low>high) {
         log_fn(LOG_PROTOCOL_WARN, LD_DIR,
                "Found bad entry in policy summary %s", escaped(orig_summary));
         return NULL;
@@ -1412,6 +1416,33 @@ parse_short_policy(const char *summary)
   return result;
 }
 
+/** Write <b>policy</b> back out into a string. Used only for unit tests
+ * currently. */
+char *
+write_short_policy(const short_policy_t *policy)
+{
+  int i;
+  char *answer;
+  smartlist_t *sl = smartlist_new();
+
+  smartlist_add_asprintf(sl, "%s", policy->is_accept ? "accept " : "reject ");
+
+  for (i=0; i < policy->n_entries; i++) {
+    const short_policy_entry_t *e = &policy->entries[i];
+    if (e->min_port == e->max_port) {
+      smartlist_add_asprintf(sl, "%d", e->min_port);
+    } else {
+      smartlist_add_asprintf(sl, "%d-%d", e->min_port, e->max_port);
+    }
+    if (i < policy->n_entries-1)
+      smartlist_add(sl, tor_strdup(","));
+  }
+  answer = smartlist_join_strings(sl, "", 0, NULL);
+  SMARTLIST_FOREACH(sl, char *, a, tor_free(a));
+  smartlist_free(sl);
+  return answer;
+}
+
 /** Release all storage held in <b>policy</b>. */
 void
 short_policy_free(short_policy_t *policy)

src/or/policies.h

@@ -61,6 +61,7 @@ void policies_free_all(void);
 char *policy_summarize(smartlist_t *policy);
 
 short_policy_t *parse_short_policy(const char *summary);
+char *write_short_policy(const short_policy_t *policy);
 void short_policy_free(short_policy_t *policy);
 int short_policy_is_reject_star(const short_policy_t *policy);
 addr_policy_result_t compare_tor_addr_to_short_policy(

src/test/test.c

@@ -1004,6 +1004,28 @@ test_circuit_timeout(void)
   return;
 }
 
+/* Helper: assert that short_policy parses and writes back out as itself,
+   or as <b>expected</b> if that's provided. */
+static void
+test_short_policy_parse(const char *input,
+                        const char *expected)
+{
+  short_policy_t *short_policy = NULL;
+  char *out = NULL;
+
+  if (expected == NULL)
+    expected = input;
+
+  short_policy = parse_short_policy(input);
+  tt_assert(short_policy);
+  out = write_short_policy(short_policy);
+  tt_str_op(out, ==, expected);
+
+ done:
+  tor_free(out);
+  short_policy_free(short_policy);
+}
+
 /** Helper: Parse the exit policy string in <b>policy_str</b>, and make sure
  * that policies_summarize() produces the string <b>expected_summary</b> from
  * it. */
@@ -1014,6 +1036,7 @@ test_policy_summary_helper(const char *policy_str,
   config_line_t line;
   smartlist_t *policy = smartlist_new();
   char *summary = NULL;
+  char *summary_after = NULL;
   int r;
   short_policy_t *short_policy = NULL;
 
@@ -1030,8 +1053,11 @@ test_policy_summary_helper(const char *policy_str,
 
   short_policy = parse_short_policy(summary);
   tt_assert(short_policy);
+  summary_after = write_short_policy(short_policy);
+  test_streq(summary, summary_after);
 
  done:
+  tor_free(summary_after);
   tor_free(summary);
   if (policy)
     addr_policy_list_free(policy);
@@ -1227,6 +1253,46 @@ test_policies(void)
                              "accept *:*",
                              "reject 1,3,5,7");
 
+  /* Short policies with unrecognized formats should get accepted. */
+  test_short_policy_parse("accept fred,2,3-5", "accept 2,3-5");
+  test_short_policy_parse("accept 2,fred,3", "accept 2,3");
+  test_short_policy_parse("accept 2,fred,3,bob", "accept 2,3");
+  test_short_policy_parse("accept 2,-3,500-600", "accept 2,500-600");
+  /* Short policies with nil entries are accepted too. */
+  test_short_policy_parse("accept 1,,3", "accept 1,3");
+  test_short_policy_parse("accept 100-200,,", "accept 100-200");
+  test_short_policy_parse("reject ,1-10,,,,30-40", "reject 1-10,30-40");
+
+  /* Try parsing various broken short policies */
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 200-199"));
+  tt_ptr_op(NULL, ==, parse_short_policy(""));
+  tt_ptr_op(NULL, ==, parse_short_policy("rejekt 1,2,3"));
+  tt_ptr_op(NULL, ==, parse_short_policy("reject "));
+  tt_ptr_op(NULL, ==, parse_short_policy("reject"));
+  tt_ptr_op(NULL, ==, parse_short_policy("rej"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3,100000"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3x,4"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3x,4"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 2-"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 2-x"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 1-,3"));
+  tt_ptr_op(NULL, ==, parse_short_policy("accept 1-,3"));
+  /* Test a too-long policy. */
+  {
+    int i;
+    char *policy = NULL;
+    smartlist_t *chunks = smartlist_new();
+    smartlist_add(chunks, tor_strdup("accept "));
+    for (i=1; i<10000; ++i)
+      smartlist_add_asprintf(chunks, "%d,", i);
+    smartlist_add(chunks, tor_strdup("20000"));
+    policy = smartlist_join_strings(chunks, "", 0, NULL);
+    SMARTLIST_FOREACH(chunks, char *, ch, tor_free(ch));
+    smartlist_free(chunks);
+    tt_ptr_op(NULL, ==, parse_short_policy(policy));/* shouldn't be accepted */
+    tor_free(policy); /* could leak. */
+  }
+
   /* truncation ports */
   sm = smartlist_new();
   for (i=1; i<2000; i+=2) {