Ticket3723 #126
Ticket3723 #126
Conversation
* add bwlist_headers argument to dirserv_read_measured_bandwidth in order to store all the headers found when parsing the file * add bwlist_headers to networkstatus_t in order to store the the headers found by the previous function * include the bandwidth headers as string in vote documents * add test to check that dirserv_read_measured_bandwidth generates the bwlist_headers
when there are not bw file headers lines.
to avoid interpreting as headers extra lines that are not key_values
Looks like a good patch, but there are a few memory handling issues, and spec issues. Please fix the issues as commented.
Please also rebase onto the latest tor master, as or/or.h has been split up.
| @@ -2621,6 +2623,8 @@ dirserv_read_measured_bandwidths(const char *from_file, | |||
| return -1; | |||
| } | |||
|
|
|||
| smartlist_add_asprintf(bwlist_headers, "timestamp=%ld", file_time); | |||
bwlist_headers is sometimes NULL, but it can't be NULL in this function call. This might cause other unit tests to crash.
| } else { | ||
| /* If line does not contain the header separator and it is key_value, | ||
| * it is probably a KeyValue header.*/ | ||
| if (strcmp(line, "====\n") != 0 && |
The terminator is 5 =, but this line has 4 =
https://gitweb.torproject.org/torspec.git/tree/bandwidth-file-spec.txt#n88
The header ends with the terminator, or the first valid relay bandwidth entry:
https://gitweb.torproject.org/torspec.git/tree/bandwidth-file-spec.txt#n235
But this code will also add any malformed relay entries to the bandwidth-file line, which is not what we want.
Please stop adding header lines when the terminator is read, or when the first valid relay bandwidth line is read.
Please also add a limit to the length of bwlist_headers. I don't expect we will ever have more than 50 headers. Stop adding headers when the limit is reached.
| tor_assert(cert); | ||
| if (v3_ns->bwlist_headers) | ||
| bwlist_headers = smartlist_join_strings(v3_ns->bwlist_headers, " ", 0, | ||
| NULL); |
Please add a limit to the length of the bwlist_headers string. I don't expect we will ever have more than 1024 (~50*20) bytes of headers. Truncate the string when the limit is reached.
| @@ -4518,6 +4527,7 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key, | |||
| options->ConsensusParams, NULL, 0, 0); | |||
| smartlist_sort_strings(v3_out->net_params); | |||
| } | |||
| v3_out->bwlist_headers = bwlist_headers; | |||
v3_out->bwlist_headers is set, but never freed. When the vote is freed, you must free both the smartlist and the strings in it.
Where is the vote freed?, this sounds like the place https://gitweb.torproject.org/tor.git/tree/src/or/dirauth/dirvote.c#n3616, but not to free bwlist_headers
The vote is a networkstatus_t, which is freed by networkstatus_vote_free_ at https://gitweb.torproject.org/tor.git/tree/src/or/networkstatus.c#n321
| bwlist_headers)); | ||
|
|
||
| /* The bwlist_headers str that should get generated by the previous | ||
| * v110v110_header_lines */ |
There is an extra "v110" on this line.
| "software_version=0.1.0\n" | ||
| "generator_started=2018-05-08T16:13:25\n" | ||
| "earliest_bandwidth=2018-05-08T16:13:26\n" | ||
| "====\n"; |
This terminator is wrong, see above.
| expect_log_msg("Empty bandwidth file\n"); | ||
|
|
||
| /* Test v1.1.0 headers */ |
Please add another test for v1.0.0 headers. (That is, just a timestamp.)
Please add a test for v1.1.0 headers with no terminator (it is a SHOULD in the spec).
Please add a test for all 3 kinds of headers (v100, v110-terminator, v110-no-terminator) with relay lines.
You can also add tests with malformed relay lines if you want: any malformed lines before the terminator (if present) or the first valid relay line will end up being treated as headers.
One of the tests should have all the header lines listed in the latest bandwidth file spec.
I think i should move the 1st comparison of this test (bandwidth file empty) to https://gitweb.torproject.org/tor.git/tree/src/test/test_dir.c#n1649, that has been recently merged, and the name of this test to test_dir_dirserv_read_measured_bandwidths_bwlist_headers, since what we want to check here is the generation of the bwlist_headers.
Sounds like a good idea to me!
| @@ -2632,6 +2636,14 @@ dirserv_read_measured_bandwidths(const char *from_file, | |||
| dirserv_cache_measured_bw(&parsed_line, file_time); | |||
| if (measured_bw_line_apply(&parsed_line, routerstatuses) > 0) | |||
| applied_lines++; | |||
| } else { | |||
Also check if bwlist_headers is NULL here.
| /* Test an empty file */ | ||
| write_str_to_file(fname, "", 0); | ||
| setup_capture_of_logs(LOG_WARN); | ||
| tt_int_op(-1, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL)); | ||
| tt_int_op(-1, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL, NULL)); |
Please also add tests for a non-empty file with a NULL bwlist_headers, because the code calls:
dirserv_read_measured_bandwidths(options->V3BandwidthsFile, NULL, NULL);
|
Reminder to myself: s/bandwidth-file/bandwidth-file-headers |
|
Closing because rebased to master and created #194 |
No description provided.
The text was updated successfully, but these errors were encountered: