Opened 3 months ago

Closed 5 weeks ago

#25960 closed enhancement (fixed)

Allow additional header lines in bandwidth measurements documents

Reported by: juga Owned by: juga
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-proposed, bwauth, 034-backport, 029-backport, 031-backport, 032-backport, 033-backport, tor-bwauth
Cc: teor Actual Points:
Parent ID: #25925 Points:
Reviewer: asn Sponsor:

Description

As commented by teor in: https://github.com/pastly/simple-bw-scanner/pull/112#issuecomment-385108911

get it backported to 0.2.9 and later.
I think Tor should only warn if:

an incomplete line contains a "bw=" key
an incomplete line contains a "node_id=" key
an incomplete line occurs after the first complete line

Child Tickets

Change History (19)

comment:2 Changed 3 months ago by teor

Keywords: 029-backport 031-backport 032-backport 033-backport added
Milestone: Tor: 0.3.4.x-final
Status: assignedneeds_revision
Type: defectenhancement

Thanks for this patch!

Here is my review:

  • please base this commit on maint-0.2.9, so we can backport it
  • Travis CI seems to have failed, is it set up on your account? Please check #tor-ci on IRC
  • please implement and add unit tests for the two header_ended=0 cases:
    • an incomplete line contains a "bw=" key
    • an incomplete line contains a "node_id=" key
  • please add a unit test for the header_ended=1 case:
    • an incomplete line occurs after the first complete line, please test:
      • a line missing node_id
      • a line missing bw
      • a line missing node_id and bw
  • this log message is confusing, because it contains a double negative, "no" and "nor":
    +    log_debug(LD_DIRSERV, "No bw nor node_id found in bandwidth file line: %s",
    +             escaped(orig_line));
    
  • Please use something like:
    +    log_debug(LD_DIRSERV, "Missing bw or node_id in bandwidth file line: %s",
    +             escaped(orig_line));
    

Edit: indenting
Edit: we sorted out CI

Last edited 3 months ago by teor (previous) (diff)

comment:3 Changed 3 months ago by juga

I think i implemented all the points in branch bug25960_maint-0.2.9_additional_headers (github, my fork). Ready for review again.

comment:4 Changed 3 months ago by juga

Status: needs_revisionneeds_review

comment:5 Changed 3 months ago by teor

Thanks, I left some comments on https://github.com/juga0/tor/tree/bug25960_maint-0.2.9_additional_headers

  1. A note about strict versus lax parsing. We don't need to change any code.
  2. Please add unit tests where header lines fail to parse.

It would also be great if we had tests for entire version 1.0.0 and version 1.1.0 files, so we know we haven't broken parsing.

Here's one way to code these tests, if the existing function takes a filename:

  1. Mock read_file_to_string to return the test data

Here's another way:

  1. Refactor the existing tor code so it reads the file to a string in one function A, then passes the string to another function B
  2. Call function B with the test data

Let me know if you need help with this code.

Please add extra commits for each change, and don't squash until we're ready to merge,
(When we squash, we keep commits that do different things separate.)

comment:6 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

comment:7 Changed 3 months ago by juga

Thanks for the review.

Mock read_file_to_string to return the test data

it does not use read_file_to_str, so i can't mock that one, it uses tor_fopen_cloexec and fgets.

Refactor the existing tor code so it reads the file to a string in one function A

And A could actually use read_file_to_str

Now i should also subtitute fgets, and instead change the loop read 512 bytes from the previous str.

comment:8 in reply to:  7 Changed 3 months ago by teor

Replying to juga:

Thanks for the review.

Mock read_file_to_string to return the test data

it does not use read_file_to_str, so i can't mock that one, it uses tor_fopen_cloexec and fgets.

Refactor the existing tor code so it reads the file to a string in one function A

And A could actually use read_file_to_str

Now i should also subtitute fgets, and instead change the loop read 512 bytes from the previous str.

Hmm, using read_file_to_str() will change the error behaviour, so we'll need to be careful.

comment:9 Changed 3 months ago by teor

Keywords: 035-proposed added
Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Next week is the last week of 0.3.4 features, and this feature is still being discussed on tor-dev@.
https://lists.torproject.org/pipermail/tor-dev/2018-May/013154.html

We should decide whether to put the small or the large version of this ticket on the roadmap.
The small change (format 1.1.0) adds backwards-compatible headers and silences warnings.
The large change (format 2.0.0) turns the bandwidth file into a directory document-style format, and rewrites the code using the directory parsing functions.

Here are the things we need to do for the large change:

  • The new format should have a new torrc option.
  • Tor should be modified to support the new format, and we should put time on the roadmap for people to work on implementing, testing, or reviewing it.
  • Either we should backport the new format to the latest stable release, or sbws should produce both formats.

comment:10 Changed 3 months ago by teor

Keywords: 034-backport added

comment:11 Changed 3 months ago by juga

If we decide 1.1.0, this ticket would be solved by https://trac.torproject.org/projects/tor/ticket/25947#comment:11

comment:12 Changed 2 months ago by juga

ticket25947_032_03_refactor should fix this ticket.

comment:13 Changed 2 months ago by juga

Status: needs_revisionneeds_review

comment:14 Changed 2 months ago by asn

Reviewer: asn

This ticket is tracked as part of #25947.
Adding myself as reviewer since I'm also reviewer of #25947.

Last edited 2 months ago by asn (previous) (diff)

comment:15 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

Putting to needs_revision because of comments in #25947.

comment:16 Changed 2 months ago by juga

Status: needs_revisionmerge_ready

Change to merge_ready because of #25947.

Probably there is no need to backport it to versions earlier than 032.

comment:17 Changed 7 weeks ago by juga

Keywords: tor-bwauth added

comment:18 Changed 5 weeks ago by nickm

Since the fix here is included in #25947, we can close this ticket.

First, please either close or un-parent #8494 as appropriate?

comment:19 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.