Retry on a new circuit for more reasons.
In git commit 388ac4126 I added some code that will never be reached:
if (rh->length > 0 && edge_reason_is_retriable(reason) &&
[...]
case END_STREAM_REASON_CONNECTREFUSED:
if (!conn->chosen_exit_optional)
break; /* break means it'll close, below */
/* Else fall through: expire this circuit, clear the
* chosen_exit_name field, and try again. */
[...]
case END_STREAM_REASON_TIMEOUT:since those two reasons aren't included in edge_reason_is_retriable().
One fix would be just to remove those dead code lines.
But another fix would be to include both of those reasons in the is_retriable() list, to better tolerate broken exits -- for example, an exit that sets -j REJECT in an iptable rule, rather than putting everything in its exit policy; or an exit for which the routing to the destination has a loop, causing the ttl to expire and some internet router to send back an icmp unreachable.
The downsides are a) if will take many seconds longer before your connection fails, if it fails, and b) we throw away circuits even more often, since we'll now chew through several circuits every time you attempt a destination that is down or unreachable.
The upside is that users will see fewer false failures, in proportion to how many broken or misconfigured exits there are.
I'm not entirely sure which side of the fence I'm on here. I think I lean slightly towards retrying for these two cases.
Activity
changed milestone to %Tor: unspecified
If we keep the code, we should apply this patch:
diff --git a/src/or/relay.c b/src/or/relay.c index f58c5c9..a80cebb 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -811,7 +811,7 @@ connection_ap_process_end_not_open( /* else, conn will get closed below */ break; case END_STREAM_REASON_CONNECTREFUSED: - if (!conn->chosen_exit_optional) + if (conn->chosen_exit_name && !conn->chosen_exit_optional) break; /* break means it'll close, below */ /* Else fall through: expire this circuit, clear the * chosen_exit_name field, and try again. */It's bug. You never knows what operator tuned or what filters installed. Like all setups by Torservers tuned tcp/ip to try only 2 syn before giveup, depends kernel it means 11s or 3s of timeout.
Trac:
Milestone: Tor: unspecified to Tor: 0.2.4.x-final
Type: enhancement to defect
Priority: normal to majorconnection_ap_process_end_not_open() is totally broken right now.
if (reason == END_STREAM_REASON_TORPROTOCOL || reason == END_STREAM_REASON_INTERNAL || reason == END_STREAM_REASON_DESTROY) { /* All three of these reasons could mean a failed tag * hit the exit and it complained. Do not probe. * Fail the circuit. */ circ->path_state = PATH_STATE_USE_FAILED; return -END_CIRC_REASON_TORPROTOCOL;Client should to retry for any end reason, internal, external, any! Client have no abilities to know what and why exit relay sent that reason and distinguish case of valid (for example) refusing tcp connection by target host or filter or attack or anything else. Keeping of throw away circuits is related but another task, that can't to stop fixing of this task. Right now exit relay fully controlling client, it can to stuck with it, to drop stream/circuit, and to force move to another circuit too anyway.
Trac:
Priority: major to critical
Summary: Retry on a new circuit if reason timeout or reason connectrefused to Retry on a new circuit if reason timeout or reason connectrefused. ALARM!
The right solution here is in branch my branch bug7870_024, but arma's original comment is right: we shouldn't mark the whole circuit as unsuitable for new circuits just because of this failure. Fortunately, there's a not-too-hard way to fix that; see #10478 (moved).
Trac:
Cc: N/A to nickm
Priority: critical to major
Summary: Retry on a new circuit if any reason. ALARM! to Retry on a new circuit for more reasons.
Keywords: tor-client deleted, tor-client 024-backport added-
Doing this right is contingent on #10478 (moved). The END_STREAM_REASON_INTERNAL business was part of #10777 (moved).
Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.6.x-final 
I believe that this change is unsafe for Tor Browser - it enables a website to include content elements that continually sends TCP resets which will cause the client to retry on new circuits until the point where a middle node is selected that enables guard discovery. See also #13669 (moved).
I would suggest a limit on the number of retries, but really, any number of automatic retries on a new circuit is bad for the web. A web page gets to include as many content elements as it likes, so even if each one is limited to one retry, you can still perform guard discovery with lots of them :/.
Trac:
Status: needs_review to needs_revision-
Perhaps a possible solution for this and related problems (#13669 (moved), others?) is to make SOCKS username+password isolation optionally much stronger. If I set a torrc option like "StrongSocksIsolation", and I send a username+password that is already in use for a request, perhaps Tor should fail that request if it cannot re-use a circuit that was already used for that SOCKS username+password?
-
It is impossible that we will fix all 188 currently open 028 tickets before 028 releases. Time to move some out. This is my second pass through the "needs_revision" tickets, looking for things to move to 0.2.9.
Note that if the requested revisions happen in advance of the 0.2.8 freeze, they can get considered for 0.2.8.
Trac:
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.9.x-final mentioned in issue #10478 (moved)
mentioned in issue #15458 (moved)
moved to tpo/core/tor#7870
mentioned in issue tpo/core/tor#10478 (closed)
