commit 0a6f4627a4292e4bed94cc1b44b6f53fc8f9ce37 Author: Nick Mathewson nickm@torproject.org Date: Wed Jun 20 08:29:38 2018 -0400
eol@eof in test-dir.c --- changes/ticket25947 | 4 ++ changes/ticket25960 | 5 ++ src/or/dirserv.c | 33 +++++++++++-- src/or/dirserv.h | 3 +- src/test/test_dir.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 175 insertions(+), 9 deletions(-)
diff --git a/changes/ticket25947 b/changes/ticket25947 new file mode 100644 index 000000000..68559a73f --- /dev/null +++ b/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 diff --git a/changes/ticket25960 b/changes/ticket25960 new file mode 100644 index 000000000..0d1be2119 --- /dev/null +++ b/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 + diff --git a/src/or/dirserv.c b/src/or/dirserv.c index c01234e0b..2362089d3 100644 --- a/src/or/dirserv.c +++ b/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) diff --git a/src/or/dirserv.h b/src/or/dirserv.h index f0b8913c5..9026f332b 100644 --- a/src/or/dirserv.h +++ b/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); diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 0106e40d9..96adb5ed5 100644 --- a/src/test/test_dir.c +++ b/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),