Opened 2 years ago

Closed 19 months ago

#20070 closed enhancement (fixed)

Make address choice failure log message more informative

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: CoreTorTeam201609, ipv6, nickm-deferred-20161017
Cc: Actual Points: 0.2
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor:

Description

Log a more informative message when we fail to find an address for a fallback or authority, because it has a hard-coded IPv6 address, but its descriptor does not have an IPv6 address.

Child Tickets

Change History (16)

comment:1 Changed 2 years ago by teor

Keywords: ipv6 added
Status: newneeds_review

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

It changes a log message and adds some unit tests.

comment:2 Changed 2 years ago by nickm

Keywords: review-group-9 added
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

comment:3 Changed 2 years ago by nickm

Keywords: 029-proposed removed

comment:4 Changed 2 years ago by nickm

Owner: set to teor
Status: needs_reviewassigned

Setting owner for these needs_review tickets.

comment:5 Changed 2 years ago by nickm

Status: assignedneeds_review

comment:6 Changed 2 years ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

Quick questions. Only ipv6, not v4?

+    /* If an authority or fallback directory has a hard-coded IPv6 address,
+     * but we receive a descriptor without that address, Tor will believe the
+     * descriptor, and end up here. */

Also, why don't we backtrace in the case of the hard-coded rs?

Else lgtm; Feel free to merge_ready this if the you believe my questions don't need code modification.

actual-review-points: 0.1

comment:7 in reply to:  6 Changed 2 years ago by teor

Replying to dgoulet:

Quick questions. Only ipv6, not v4?

+    /* If an authority or fallback directory has a hard-coded IPv6 address,
+     * but we receive a descriptor without that address, Tor will believe the
+     * descriptor, and end up here. */

IPv4 addresses are mandatory: they can be different between hard-coded details and descriptor, but they can't be missing in the descriptor or hard-coded details, so this case does not apply. (We might get a reachability mismatch if the IPv4 addresses aren't the same, I guess that would send us here also. I'll revise the comment.)

+    /* If an authority or fallback directory has:
+     * - a hard-coded IPv4 address, but we receive a descriptor with a different address, or
+     * - a hard-coded IPv6 address, but we receive a descriptor without that address,
+     * Tor can find no reachable relays in the consensus, try a fallback (or authority)
+     * on its hard-coded address, but ultimately believe the descriptor, and end up here. */

Also, why don't we backtrace in the case of the hard-coded rs?

Because it's not an unexpected situation (it's entirely normal when a fallback drops its IPv6 address) and backtraces alarm users.

Else lgtm; Feel free to merge_ready this if the you believe my questions don't need code modification.

actual-review-points: 0.1

comment:8 Changed 2 years ago by dgoulet

Ok cool thx for clarification! Feel free to put this in merger_ready when you've made your changes.

comment:9 Changed 23 months ago by nickm

Status: needs_revisionassigned

setting owner

comment:10 Changed 23 months ago by nickm

Status: assignedneeds_revision

comment:11 Changed 23 months ago by nickm

Keywords: review-group-10 added; review-group-9 removed

Moving not-reviewed-by-me tickets in review-group-9, and for-0.2.9/0.2.8 tickets, to review-group-10.

comment:12 Changed 22 months ago by nickm

Keywords: nickm-deferred-20161017 added
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

I am fairly sure that these are neither regressions nor major problems. So, deferring from 0.2.9. Please let me know if I'm wrong.

comment:13 Changed 22 months ago by nickm

Keywords: review-group-10 removed

comment:14 Changed 19 months ago by nickm

Type: defectenhancement

batch modify: I think these are "enhancement", though I could be wrong about some.

comment:15 Changed 19 months ago by dgoulet

Status: needs_revisionmerge_ready

No action items needed after review. Going in merge_ready.

comment:16 Changed 19 months ago by nickm

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