Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1739 closed defect (fixed)

Control port no longer sends CIRC TIMEOUT events

Reported by: mikeperry Owned by: mikeperry
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.x-final
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The shift to a right-censored pareto distribution for circuit build times in 0.2.2.14-alpha caused us to fail to emit the CIRC FAILED REASON=TIMEOUT event to the control port, due to the purpose change not immediately generating a circ close.

Child Tickets

Change History (17)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final
Owner: set to mikeperry
Status: newassigned

comment:2 Changed 9 years ago by mikeperry

Status: assignedneeds_review

See mikeperry/cbt-hs-and-controlport-fixes.

comment:3 Changed 9 years ago by nickm

What's the status on this branch, Mike? On #1740 it sounds like you're saying not to merge it, and one of the bugs it says it fixes ( #1741 ) is supposedly already fixed. Can you straighten me out here?

comment:4 Changed 9 years ago by mikeperry

If you can pull out the commit (0e10656747b7) from the branch without conflict, the fix for this should be independent from the one for #1740, which should not be merged. Otherwise I'll take care of it in a couple of days as soon as I'm done with Chrome stuff.

comment:5 Changed 9 years ago by nickm

That commit looks fine to me... but what keeps a random remote host from sending us END_CIRC_REASON_MEASUREMENT_EXPIRED and having us believe it? Maybe it should be one of the negative codes that are only generated internally. Am I right ? Would that work? Or am I looking at code too late at night?

comment:6 Changed 9 years ago by arma

Priority: normalmajor

Triage: I'm going to call this one blocking 0.2.2.x, since it's close to working, and is a bug introduced in 0.2.2.x

comment:7 Changed 9 years ago by mikeperry

Hrmm, if that's the case, shouldn't we also be worried about them sending us END_CIRC_REASON=TIMEOUT? That seems more dangerous.. Though I don't think that either case uses the actual reason codes to decide anything.

comment:8 Changed 9 years ago by nickm

I believe we should. We should distinguish end reasons that can be sent over the wire, and reasons that we ended stuff internally.

comment:9 Changed 9 years ago by nickm

Status: needs_reviewassigned

Moving this out of needs_review; please move it back when it's fixed?

comment:10 Changed 9 years ago by Sebastian

Looks to me like END_CIRC_REASON_MEASUREMENT_EXPIRED is the only reason that should never be sent over the wire. All the other reasons can happen legitimately on other relays, including END_CIRC_REASON_TIMEOUT. Therefore measurement expired doesn't need to go into tor-spec, and we just need to make sure we don't accept this from relays.

comment:11 Changed 9 years ago by Sebastian

One question would be whether cbt accepts remotely generated reasons though. If so, I think that's a separate bug

comment:12 Changed 9 years ago by mikeperry

Status: assignedneeds_review

Ok, I made this reason negative. mikeperry/bug1739 should be ready for merge. It is branched off of mikeperry/bug1740, so that will need to be merged first.

comment:13 Changed 9 years ago by Sebastian

We should wait on this until #1740 is resolved. It doesn't seem clear that mikeperrybug1740 will get merged the way it exists currently.

comment:14 Changed 9 years ago by mikeperry

Rebased to the new mikeperry/bug1740.

comment:15 Changed 9 years ago by arma

<nickm> 1739: looks good

comment:16 Changed 9 years ago by arma

Resolution: fixed
Status: needs_reviewclosed

Merged.

comment:17 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.