Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#26004 closed defect (fixed)

Allow Tor to accept node_id at the end of a bandwidth file line

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: fast-fix, 034-backport-maybe, 033-backport-maybe, 032-backport-maybe, 031-backport-maybe, 029-backport-maybe, tor-bwauth
Cc: Actual Points:
Parent ID: #25925 Points:
Reviewer: nickm Sponsor:


Tor doesn't accept node_id at the end of a line, because the trailing newline is part of the string at the end of the line. To fix this issue, we should make newline one of the separators in the string token list.

Child Tickets

Change History (14)

comment:1 Changed 2 years ago by teor

Keywords: 034-backport-maybe 033-backport-maybe 032-backport-maybe 031-backport-maybe 029-backport-maybe added

comment:3 Changed 2 years ago by teor

Status: newneeds_revision

That looks like it will work, and it seems like it passes the tests.
The checks are in a weird order, but we can fix that in a rewrite.

In our code style, we use character constants instead of integers. So 10 is '\n' and 0 is '\0'.

When we name branches, we call them after the tor branch they are based on.
This branch is based on master, so it should not have "maint-0.2.9" in the name.
Did you mean to base it on maint-0.2.9?

You can use "029" in branch names rather than "maint-0.2.9" if you want.

When you want a review on a ticket, you can put it in "needs_review".

comment:4 Changed 2 years ago by juga

Keywords: tor-dirauth added
Status: needs_revisionneeds_review

comment:5 Changed 2 years ago by juga

Changed what you suggested.
Yes, i meant to based in maint-0.2.9, isn't based on?
I haven't renamed the branch, since probably should create another squashing commits.
Opened PR following the naming proposed in the wiki (org/process/TorOnTrac).

comment:6 Changed 2 years ago by dgoulet

Reviewer: nickm

comment:7 Changed 2 years ago by nickm

Two changes needed here:

  • If the input to this function is the empty string, strlen(line) - 1 will underflow.
  • This branch needs a changes file.

I've made both of these changes in my own branch (with the same name as yours) -- please let me know if you agree with them, and I'll merge.

comment:8 Changed 2 years ago by juga

Thanks for the review. Maybe log_warn should be lower, since in the spec we have so far we would allow files with empty bw lines?

comment:9 Changed 2 years ago by juga

Feel free to squash, if needed for the log to looks better

comment:10 Changed 2 years ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.3.x-final

I've squashed this as bug26004_029_squashed, and merged it to master. Marking for possible backport!

comment:11 Changed 2 years ago by nickm

Status: needs_reviewmerge_ready

comment:12 Changed 2 years ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

comment:13 Changed 2 years ago by teor

Resolution: fixed
Status: merge_readyclosed

All directory authorities that vote on bandwidths are running 0.3.3 or later. torflow and sbws do not produce lines with node_id at the end. (And all other generators should be able to obey this constraint.)

So I don't think we need to backport this fix.

comment:14 Changed 2 years ago by juga

Keywords: tor-bwauth added; tor-dirauth removed
Note: See TracTickets for help on using tickets.