Ticket3723 02 #194

juga0 · 2018-06-30T19:37:52Z

Follows #126 after rebase to master.
@teor2345 I think i addressed all your comments.
In the end i thought it was better to add all the tests to the same function to don't duplicate all the variables.
Instead of truncating the string with the bandwidth file headers, i thought it was better to truncate the smartlist. I created a function for that but something is wrong since there's memory leaks when i use it in the test. See the last 3 commits and the FIXMEs in the code. Not ready to merge because of that.

* 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

bandwidth-file

and add bw file terminator constant

If bandwidth file terminator is found, set end of headers flag and do not store the line. If it is not, parse a relay line and check whether it is a header line.

also add tests for bw_file_headers. Headers are all that is found before a correct relay line or the terminator. Tests include: * a empty bandwidth file * a bandwidth file with only timestamp * a bandwidth file with v1.0.0 headers * a bandwidth file with v1.0.0 headers and relay lines * a bandwidth file with v1.1.0 headers and v1.0.0 relay lines * a bandwidth file with v1.0.0 headers, malformed relay lines and relay lines * a bandwidth file with v1.0.0 headers, malformed relay lines, relay lines and malformed relay lines * a bandwidth file with v1.1.0 headers without terminator * a bandwidth file with v1.1.0 headers with terminator * a bandwidth file with v1.1.0 headers without terminator and relay lines * a bandwidth file with v1.1.0 headers with terminator and relay lines * a bandwidth file with v1.1.0 headers without terminator, bad relay lines and relay lines * a bandwidth file with v1.1.0 headers with terminator, bad relay lines and relay lines

and complete v1.0.0 bandwidth file

FIXME: this seems to leak memory

FIXME: depends on smartlist_truncate

FIXME: leaks memory

juga0 · 2018-06-30T20:27:15Z

Right now there is also this problem with last fixup:

src/or/dirserv.c:2690:15: error: implicit declaration of function ‘string_is_key_value’ [-Werror=implicit-function-declaration]
               string_is_key_value(LOG_DEBUG, line) &&
               ^
src/or/dirserv.c:2690:15: error: nested extern declaration of ‘string_is_key_value’ [-Werror=nested-externs]

teor2345

Please see my comments on the new code.

teor2345 · 2018-06-30T23:15:17Z

src/or/dirserv.h

+
+/** Terminatore that separates bandwidth file headers from bandwidth file
+ * relay lines */
+#define BW_FILE_TERMINATOR "=====\n"


Please rename to BW_FILE_HEADERS_TERMINATOR to avoid confusion.

teor2345 · 2018-06-30T23:20:05Z

src/or/dirserv.c

+       * then assume this line is a header and add it to bw_file_headers */
+      else if (bw_file_headers &&
+              (line_is_after_headers == 0) &&
+              string_is_key_value(LOG_DEBUG, line) &&


string_is_key_value returns true for relay bandwidth lines, because it allows spaces:

tor/src/lib/encoding/keyval.c

Line 19 in be40ad5

string_is_key_value(int severity, const char *string)

Please also check that there are no spaces in the line.

teor2345 · 2018-06-30T23:23:45Z

src/lib/container/smartlist.c

+  for (i=0; i < diff; i++) {
+    log_debug(LD_DIRSERV, "sl->list[size] %s\n", (char *)sl->list[size]);
+    log_debug(LD_DIRSERV, "sl len %d\n", smartlist_len(sl));
+    smartlist_del(sl, size);


Use smartlist_del_keeporder() to avoid reordering the bandwidth headers.
Use smartlist_del_keeporder(sl, i) to delete the correct item.
Use tor_free(smartlist_get(sl, i)) to free the item before deleting it.

Also, you should probably delete from the end of the smartlist, rather than the start.

Actually, I think we can delete this entire commit.
See my comments on the next commit.

teor2345 · 2018-06-30T23:32:26Z

src/or/dirauth/dirvote.c

+      if (smartlist_len(v3_ns->bw_file_headers) > MAX_BW_FILE_HEADERS_LEN)
+        /* FIXME */
+        smartlist_truncate(v3_ns->bw_file_headers, MAX_BW_FILE_HEADERS_LEN);
+    } else
      bw_file_headers = tor_strdup("");


If any block in a group of if/else blocks has braces, please put braces around every block.

teor2345 · 2018-06-30T23:36:20Z

src/or/dirauth/dirvote.c

+                                               NULL);
+      if (smartlist_len(v3_ns->bw_file_headers) > MAX_BW_FILE_HEADERS_LEN)
+        /* FIXME */
+        smartlist_truncate(v3_ns->bw_file_headers, MAX_BW_FILE_HEADERS_LEN);


We already limit the number of headers in earlier code.

So you can just log a BUG() warning here, and vote an empty string:

if (BUG(smartlist_len(v3_ns->bw_file_headers) > MAX_BW_FILE_HEADERS_LEN)) { bw_file_headers = tor_strdup(""); }

You should do this check before joining the string together, to avoid a memory leak, and to avoid allocating large amounts of memory.

so then i would do 2 checks?, 1 with the smartlist and 1 with the string?.
The code then would be:

if (BUG(smartlist_len(v3_ns->bw_file_headers) > MAX_BW_FILE_HEADERS_LEN)) { bw_file_headers = tor_strdup(""); } bw_file_headers = smartlist_join_strings(v3_ns->bw_file_headers, " ", 0, NULL); if (BUG(strlen(bw_file_headers) > MAX_VOTE_LINE_LEN)) { tor_free(bw_file_headers); bw_file_headers = tor_strdup(""); }

Almost, but you can't overwrite bw_file_headers with valid data after an error:

if (BUG(smartlist_len(v3_ns->bw_file_headers) > MAX_BW_FILE_HEADERS_LEN)) { bw_file_headers = tor_strdup(""); } else { bw_file_headers = smartlist_join_strings(v3_ns->bw_file_headers, " ", 0, NULL); if (BUG(strlen(bw_file_headers) > MAX_VOTE_LINE_LEN)) { tor_free(bw_file_headers); bw_file_headers = tor_strdup(""); } }

Here's another alternative that might make the code simpler:

char *bw_file_headers = NULL; … if (! BUG(smartlist_len(v3_ns->bw_file_headers) > MAX_BW_FILE_HEADERS_LEN)) { bw_file_headers = smartlist_join_strings(v3_ns->bw_file_headers, " ", 0, NULL); if (BUG(strlen(bw_file_headers) > MAX_VOTE_LINE_LEN)) { /* Free and set to NULL, so the vote header line is empty */ tor_free(bw_file_headers); } } … bw_file_headers ? bw_file_headers : "");

Then the final tor_free(bw_file_headers) works because free(NULL) does nothing.

teor2345 · 2018-06-30T23:39:55Z

src/or/dirauth/dirvote.c

@@ -267,10 +267,13 @@ format_networkstatus_vote(crypto_pk_t *private_signing_key,
    else
      params = tor_strdup("");
    tor_assert(cert);
-    if (v3_ns->bw_file_headers)
+    if (v3_ns->bw_file_headers) {
      bw_file_headers = smartlist_join_strings(v3_ns->bw_file_headers, " ", 0,


Please limit the length of the bw_file_headers string, not just the number of headers:
#126 (comment)

(Limiting the number of headers to 50 header is not enough to limit the vote line to 1024 bytes, because each header can be 512 bytes long.)

You could log a BUG() warning and use the empty string if the string is too long.
(It's very unlikely that the string will be too long.)
Remember to free the string before replacing it with "".

(Limiting the number of headers to 50 header is not enough to limit the vote line to 1024 bytes, because each header can be 512 bytes long.)

Is there already a constant limiting the size of the vote line?, i don't find something like that, maybe i should add it?.

It would be:

/** Maximum size of a line in a vote. */ MAX_VOTE_LINE_LEN 1024

In dirvote.h

There are similar line length constants in diserv.h:
https://github.com/torproject/tor/blob/master/src/or/dirserv.h#L29
So I think this new constant should go in dirserv.h, even though it's only used in votes.

You said:

/** Maximum size of a line in a vote. */
MAX_VOTE_LINE_LEN 1024
But this constant does not limit all lines in the vote, just the bandwidth-file-headers line.

Please name the line limit constant after the kind of line, something like:
MAX_BANDWIDTH_HEADERS_LINE_LEN

Or MAX_BW_FILE_HEADERS_LINE_LEN to match MAX_BW_FILE_HEADERS_LEN.

Now that we have two similar constants, we should change MAX_BW_FILE_HEADERS_LEN to MAX_BW_FILE_HEADER_COUNT_IN_VOTE. Otherwise people might get confused.

teor2345 · 2018-06-30T23:53:30Z

Right now there is also this problem with last fixup:

key_value.h isn't included in util.h:
https://github.com/torproject/tor/blob/master/src/common/util.h#L19

Please make a separate commit with this fix.

juga0 · 2018-07-01T19:09:30Z

Continuing in #195

juga0 added 13 commits June 28, 2018 19:53

Do not add bw file line to the vote

549643e

when there are not bw file headers lines.

Check that the header is key_value

0c5909b

to avoid interpreting as headers extra lines that are not key_values

Add bw_file_headers to networkstatus_t

82c5678

Free bw_list_headers in networstatus_t

e9c8abf

Rename bwlist to bw_file and banwidth to

e8aa63b

bandwidth-file

Replace bwlist by bw_file

201af3f

and add bw file terminator constant

Replace bwlist by bw_file and terminator condition

7a609d1

If bandwidth file terminator is found, set end of headers flag and do not store the line. If it is not, parse a relay line and check whether it is a header line.

Add test with NULL bw_file_header

73f516e

and complete v1.0.0 bandwidth file

Add smartlist_truncate and test

5055739

FIXME: this seems to leak memory

Ensure that bw_file_headers is not bigger than max

1591e48

FIXME: depends on smartlist_truncate

Add tests for max length bw file headers

a3f0b66

FIXME: leaks memory

juga0 mentioned this pull request Jun 30, 2018

Ticket3723 #126

Closed

fixup! Check that the header is key_value

acbe52e

teor2345 suggested changes Jun 30, 2018

View reviewed changes

juga0 mentioned this pull request Jul 1, 2018

Ticket3723 03 #195

Closed

juga0 closed this Jul 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket3723 02 #194

Ticket3723 02 #194

juga0 commented Jun 30, 2018 •

edited

Loading

juga0 commented Jun 30, 2018

teor2345 left a comment

teor2345 Jun 30, 2018

teor2345 Jun 30, 2018

teor2345 Jun 30, 2018

teor2345 Jun 30, 2018

teor2345 Jun 30, 2018

teor2345 Jun 30, 2018

juga0 Jul 1, 2018

teor2345 Jul 1, 2018

teor2345 Jun 30, 2018

teor2345 Jun 30, 2018

juga0 Jul 1, 2018

juga0 Jul 1, 2018

teor2345 Jul 1, 2018

teor2345 Jul 1, 2018

teor2345 Jul 1, 2018

teor2345 commented Jun 30, 2018

juga0 commented Jul 1, 2018

Ticket3723 02 #194

Ticket3723 02 #194

Conversation

juga0 commented Jun 30, 2018 • edited Loading

juga0 commented Jun 30, 2018

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 commented Jun 30, 2018

juga0 commented Jul 1, 2018

juga0 commented Jun 30, 2018 •

edited

Loading