Opened 18 months ago

Closed 2 months ago

#18982 closed defect (implemented)

Tor should log 1-based hop numbers

Reported by: teor Owned by: teor
Priority: Low Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.5-alpha
Severity: Minor Keywords: easy, logging
Cc: Actual Points: .1
Parent ID: Points: .1
Reviewer: arma Sponsor:

Description (last modified by teor)

The control-spec says hops are 1-based, and we often log "first hop":

  If HOP=HopNum is specified, Tor will choose the HopNumth hop in the
  circuit as the exit node, rather than the last node in the circuit.
  Hops are 1-indexed; generally, it is not permitted to attach to hop 1.

But the following functions log 0-based hops:

  • choose_good_middle_server
  • onion_extend_cpath (which also logs a 1-based hop message as well)

We need to add 1 to the 0-based hop counts in these functions.

Credit to Xiaofan Li for discovering this issue.

Child Tickets

Change History (26)

comment:1 Changed 18 months ago by teor

Description: modified (diff)

comment:2 Changed 18 months ago by teor

Actual Points: 1 hour
Keywords: 029-proposed added
Status: newneeds_review
Version: Tor: 0.2.4.5-alpha

See my branch bug18982 on https://github.com/teor2345/tor.git

It's a 2-line, 4-character fix.

comment:3 Changed 17 months ago by nickm

Keywords: 029-nickm-says-yes added

comment:4 Changed 17 months ago by arma

Reviewer: arma

comment:5 Changed 17 months ago by nickm

Keywords: review-group-2 added

Create a review-group-2 from (most of the) tickets in 0.2.8 or 0.2.9 or 029-nickm-says-yes listed as needs_review,

comment:6 Changed 17 months ago by nickm

Owner: set to teor
Status: needs_reviewassigned

setting owner

comment:7 Changed 17 months ago by nickm

Status: assignedneeds_review

comment:8 Changed 17 months ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Calling these "yes" because they are bugfixes.

comment:9 Changed 17 months ago by arma

Keywords: CoreTorTeam201606 added

comment:10 Changed 17 months ago by arma

Ok, I found a moment to look at the patch. It has the problem that I expected it to have: there's no way to tell now whether somebody giving us one of these lines is using the old code or the new code -- so now seeing a log line talking about "hop 1" is ambiguous about whether it's the first relay or the second relay.

In the distant future, this issue won't matter anymore, because nobody will be using the old code. But the distant future is years away.

I think an easy fix might be to change something else in each of these log lines too, so it's possible, given just the log line, to disambiguate?

comment:11 Changed 17 months ago by dgoulet

Status: needs_reviewneeds_information

comment:12 Changed 17 months ago by nickm

Trivial suggested disambiguation: Add a # before the number.

comment:13 Changed 17 months ago by arma

Sounds great!

comment:14 Changed 17 months ago by nickm

Actual Points: 1 hour.1
Points: small.1
Status: needs_informationneeds_revision

comment:15 Changed 17 months ago by nickm

Keywords: review-group-3 added; review-group-2 removed

These have all had at least one review during review-group-2.

comment:16 Changed 15 months ago by teor

I could revise this, or someone else could pick it up.

comment:17 Changed 14 months ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.

Please feel free to move these back to 0.2.9 if you finish the revisions soon.

comment:18 Changed 11 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:19 Changed 10 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:20 Changed 5 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:21 Changed 5 months ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:22 Changed 5 months ago by nickm

Keywords: CoreTorTeam201606 removed

comment:23 Changed 5 months ago by nickm

Keywords: review-group-3 removed

comment:24 Changed 5 months ago by nickm

Keywords: 029-nickm-says-yes removed

comment:25 Changed 4 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final

comment:26 Changed 2 months ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Fixed up and merged.

Note: See TracTickets for help on using tickets.