eol@eof in test-dir.c

changes/ticket25947

@@ -0,0 +1,4 @@
+  o Minor feature (unit tests):
+    - Test complete bandwidth measurements files and test that incomplete lines
+      only give warnings when the end of the header has not been
+      detected. Fixes bug 25947; bugfix on 0.2.2.1-alpha

changes/ticket25960

@@ -0,0 +1,5 @@
+  o Minor feature (directory authorities):
+    - Stop warning about incomplete bw lines before the first complete bw line
+      has been found, so that additional header lines can be ignored.
+      Fixes bug 25960; bugfix on 0.2.2.1-alpha
+

src/or/dirserv.c

@@ -2448,11 +2448,20 @@ dirserv_read_guardfraction_file(const char *fname,
 
 /**
  * Helper function to parse out a line in the measured bandwidth file
- * into a measured_bw_line_t output structure. Returns -1 on failure
- * or 0 on success.
+ * into a measured_bw_line_t output structure.
+ *
+ * If <b>line_is_after_headers</b> is true, then if we encounter an incomplete
+ * bw line, return -1 and warn, since we are after the headers and we should
+ * only parse bw lines. Return 0 otherwise.
+ *
+ * If <b>line_is_after_headers</b> is false then it means that we are not past
+ * the header block yet. If we encounter an incomplete bw line, return -1 but
+ * don't warn since there could be additional header lines coming. If we
+ * encounter a proper bw line, return 0 (and we got past the headers).
  */
 STATIC int
-measured_bw_line_parse(measured_bw_line_t *out, const char *orig_line)
+measured_bw_line_parse(measured_bw_line_t *out, const char *orig_line,
+                       int line_is_after_headers)
 {
   char *line = tor_strdup(orig_line);
   char *cp = line;
@@ -2532,6 +2541,13 @@ measured_bw_line_parse(measured_bw_line_t *out, const char *orig_line)
   if (got_bw && got_node_id) {
     tor_free(line);
     return 0;
+  } else if (line_is_after_headers == 0) {
+    /* There could be additional header lines, therefore do not give warnings
+     * but returns -1 since it's not a complete bw line. */
+    log_debug(LD_DIRSERV, "Missing bw or node_id in bandwidth file line: %s",
+             escaped(orig_line));
+    tor_free(line);
+    return -1;
   } else {
     log_warn(LD_DIRSERV, "Incomplete line in bandwidth file: %s",
              escaped(orig_line));
@@ -2580,6 +2596,11 @@ dirserv_read_measured_bandwidths(const char *from_file,
   int applied_lines = 0;
   time_t file_time, now;
   int ok;
+   /* This flag will be 1 only when the first successful bw measurement line
+   * has been encountered, so that measured_bw_line_parse don't give warnings
+   * if there are additional header lines, as introduced in Bandwidth List spec
+   * version 1.1.0 */
+  int line_is_after_headers = 0;
 
   /* Initialise line, so that we can't possibly run off the end. */
   memset(line, 0, sizeof(line));
@@ -2627,7 +2648,11 @@ dirserv_read_measured_bandwidths(const char *from_file,
   while (!feof(fp)) {
     measured_bw_line_t parsed_line;
     if (fgets(line, sizeof(line), fp) && strlen(line)) {
-      if (measured_bw_line_parse(&parsed_line, line) != -1) {
+      if (measured_bw_line_parse(&parsed_line, line,
+                                 line_is_after_headers) != -1) {
+        /* This condition will be true when the first complete valid bw line
+         * has been encountered, which means the end of the header lines. */
+        line_is_after_headers = 1;
         /* Also cache the line for dirserv_get_bandwidth_for_router() */
         dirserv_cache_measured_bw(&parsed_line, file_time);
         if (measured_bw_line_apply(&parsed_line, routerstatuses) > 0)

src/or/dirserv.h

@@ -174,7 +174,8 @@ STATIC void dirserv_set_routerstatus_testing(routerstatus_t *rs);
 /* Put the MAX_MEASUREMENT_AGE #define here so unit tests can see it */
 #define MAX_MEASUREMENT_AGE (3*24*60*60) /* 3 days */
 
-STATIC int measured_bw_line_parse(measured_bw_line_t *out, const char *line);
+STATIC int measured_bw_line_parse(measured_bw_line_t *out, const char *line,
+                                  int line_is_after_headers);
 
 STATIC int measured_bw_line_apply(measured_bw_line_t *parsed_line,
                            smartlist_t *routerstatuses);

src/test/test_dir.c

@@ -1547,12 +1547,18 @@ test_dir_measured_bw_kb(void *arg)
   (void)arg;
   for (i = 0; strcmp(lines_fail[i], "end"); i++) {
     //fprintf(stderr, "Testing: %s\n", lines_fail[i]);
-    tt_int_op(measured_bw_line_parse(&mbwl, lines_fail[i]), OP_EQ, -1);
+    /* Testing only with line_is_after_headers = 1. Tests with
+     * line_is_after_headers = 0 in
+     * test_dir_measured_bw_kb_line_is_after_headers */
+    tt_assert(measured_bw_line_parse(&mbwl, lines_fail[i], 1) == -1);
   }
 
   for (i = 0; strcmp(lines_pass[i], "end"); i++) {
     //fprintf(stderr, "Testing: %s %d\n", lines_pass[i], TOR_ISSPACE('\n'));
-    tt_int_op(measured_bw_line_parse(&mbwl, lines_pass[i]), OP_EQ, 0);
+    /* Testing only with line_is_after_headers = 1. Tests with
+     * line_is_after_headers = 0 in
+     * test_dir_measured_bw_kb_line_is_after_headers */
+    tt_assert(measured_bw_line_parse(&mbwl, lines_pass[i], 1) == 0);
     tt_assert(mbwl.bw_kb == 1024);
     tt_assert(strcmp(mbwl.node_hex,
                 "557365204145532d32353620696e73746561642e") == 0);
@@ -1564,7 +1570,7 @@ test_dir_measured_bw_kb(void *arg)
 
 /* Test dirserv_read_measured_bandwidths */
 static void
-test_dir_dirserv_read_measured_bandwidths(void *arg)
+test_dir_dirserv_read_measured_bandwidths_empty(void *arg)
 {
   char *fname=NULL;
   (void)arg;
@@ -1581,6 +1587,129 @@ test_dir_dirserv_read_measured_bandwidths(void *arg)
   teardown_capture_of_logs();
 }
 
+/* Unit tests for measured_bw_line_parse using line_is_after_headers flag.
+ * When the end of the header is detected (a first complete bw line is parsed),
+ * incomplete lines fail and give warnings, but do not give warnings if
+ * the header is not ended, allowing to ignore additional header lines. */
+static void
+test_dir_measured_bw_kb_line_is_after_headers(void *arg)
+{
+  (void)arg;
+  measured_bw_line_t mbwl;
+  const char *line_pass = \
+    "node_id=$557365204145532d32353620696e73746561642e bw=1024\n";
+  int i;
+  const char *lines_fail[] = {
+    "node_id=$557365204145532d32353620696e73746561642e \n",
+    "bw=1024\n",
+    "rtt=300\n",
+    "end"
+  };
+
+  setup_capture_of_logs(LOG_DEBUG);
+
+  /* Test bw lines when header has ended */
+  for (i = 0; strcmp(lines_fail[i], "end"); i++) {
+    tt_assert(measured_bw_line_parse(&mbwl, lines_fail[i], 1) == -1);
+    expect_log_msg_containing("Incomplete line in bandwidth file:");
+    mock_clean_saved_logs();
+  }
+
+  tt_assert(measured_bw_line_parse(&mbwl, line_pass, 1) == 0);
+
+  /* Test bw lines when header has not ended */
+  for (i = 0; strcmp(lines_fail[i], "end"); i++) {
+    tt_assert(measured_bw_line_parse(&mbwl, lines_fail[i], 0) == -1);
+    expect_log_msg_containing("Missing bw or node_id in bandwidth file line:");
+    mock_clean_saved_logs();
+  }
+
+  tt_assert(measured_bw_line_parse(&mbwl, line_pass, 0) == 0);
+
+ done:
+  teardown_capture_of_logs();
+}
+
+/* Test dirserv_read_measured_bandwidths with whole files. */
+static void
+test_dir_dirserv_read_measured_bandwidths(void *arg)
+{
+  (void)arg;
+  char *content = NULL;
+  time_t timestamp = time(NULL);
+  char *fname = tor_strdup(get_fname("V3BandwidthsFile"));
+
+  /* Test Torflow file only with timestamp*/
+  tor_asprintf(&content, "%ld", timestamp);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(-1, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+  /* Test Torflow file with timestamp followed by '\n' */
+  tor_asprintf(&content, "%ld\n", timestamp);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+  /* Test Torflow complete file*/
+  const char *torflow_relay_lines=
+    "node_id=$557365204145532d32353620696e73746561642e bw=1024 "
+    "nick=Test measured_at=1523911725 updated_at=1523911725 "
+    "pid_error=4.11374090719 pid_error_sum=4.11374090719 "
+    "pid_bw=57136645 pid_delta=2.12168374577 circ_fail=0.2 "
+    "scanner=/filepath\n";
+
+  tor_asprintf(&content, "%ld\n%s", timestamp, torflow_relay_lines);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+  /* Test Torflow complete file including v1.1.0 headers */
+  const char *v110_header_lines=
+    "version=1.1.0\n"
+    "software=sbws\n"
+    "software_version=0.1.0\n"
+    "generator_started=2018-05-08T16:13:25\n"
+    "earliest_bandwidth=2018-05-08T16:13:26\n"
+    "====\n";
+
+  tor_asprintf(&content, "%ld\n%s%s", timestamp, v110_header_lines,
+               torflow_relay_lines);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+  /* Test Torflow with additional headers afer a correct bw line */
+  tor_asprintf(&content, "%ld\n%s%s", timestamp, torflow_relay_lines,
+               v110_header_lines);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+  /* Test Torflow with additional headers afer a correct bw line and more
+   * bw lines after the headers. */
+  tor_asprintf(&content, "%ld\n%s%s%s", timestamp, torflow_relay_lines,
+               v110_header_lines, torflow_relay_lines);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+  /* Test sbws file */
+  const char *sbws_relay_lines=
+    "node_id=$68A483E05A2ABDCA6DA5A3EF8DB5177638A27F80 "
+    "master_key_ed25519=YaqV4vbvPYKucElk297eVdNArDz9HtIwUoIeo0+cVIpQ "
+    "bw=760 nick=Test rtt=380 time=2018-05-08T16:13:26\n";
+
+  tor_asprintf(&content, "%ld\n%s%s", timestamp, v110_header_lines,
+               sbws_relay_lines);
+  write_str_to_file(fname, content, 0);
+  tor_free(content);
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+ done:
+  tor_free(fname);
+}
+
 #define MBWC_INIT_TIME 1000
 
 /** Do the measured bandwidth cache unit test */
@@ -5849,9 +5978,11 @@ struct testcase_t dir_tests[] = {
   DIR_LEGACY(versions),
   DIR_LEGACY(fp_pairs),
   DIR(split_fps, 0),
-  DIR_LEGACY(dirserv_read_measured_bandwidths),
+  DIR_LEGACY(dirserv_read_measured_bandwidths_empty),
   DIR_LEGACY(measured_bw_kb),
+  DIR_LEGACY(measured_bw_kb_line_is_after_headers),
   DIR_LEGACY(measured_bw_kb_cache),
+  DIR_LEGACY(dirserv_read_measured_bandwidths),
   DIR_LEGACY(param_voting),
   DIR(param_voting_lookup, 0),
   DIR_LEGACY(v3_networkstatus),