Opened 12 years ago

Last modified 3 years ago

#681 closed defect (Implemented)

encrypted dir conns confuse LeaveStreamsUnattached

Reported by: arma Owned by: nickm
Priority: Low Milestone: 0.2.0.x-final
Component: Core Tor/Tor Version: 0.2.0.25-rc
Severity: Keywords:
Cc: arma, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

(This is Mikeperry's bug)

When we launch an encrypted dir conn in directory.c via
connection_ap_make_link(), it'll put the stream in state
AP_CONN_STATE_CIRCUIT_WAIT. This means that even controllers
that set LeaveStreamsUnattached won't need to handle it,
since it's already being handled.

Bug #1: controllers don't know this, and can't distinguish
streams that are already being handled. Mike had a patch to
put an extra flag on the stream status line.

Bug #2: then if the stream causes a circuit to get created, and gets
attached to that circuit, but the circuit times out or otherwise
fails to fulfill the request, the stream will be handled by
connection_ap_detach_retriable() which will set it to state
AP_CONN_STATE_CONTROLLER_WAIT. And the controller doesn't realize
that this time it's supposed to handle it (and it probably shouldn't
need to handle it)

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (8)

comment:1 Changed 12 years ago by arma

--- connection_edge.c   (revision 14577)
+++ connection_edge.c   (working copy)
@@ -549,7 +549,7 @@
 {
   control_event_stream_status(conn, STREAM_EVENT_FAILED_RETRIABLE, reason);
   conn->_base.timestamp_lastread = time(NULL);
-  if (! get_options()->LeaveStreamsUnattached) {
+  if (!get_options()->LeaveStreamsUnattached || conn->use_begindir) {
     conn->_base.state = AP_CONN_STATE_CIRCUIT_WAIT;
     circuit_detach_stream(TO_CIRCUIT(circ),conn);
     return connection_ap_handshake_attach_circuit(conn);

Is this adequate for solving #2? I think so. If we want to be more clever,
we could make that second check into its own function, named something like
connection_edge_chooses_circuits_automatically().

Last edited 3 years ago by arma (previous) (diff)

comment:2 Changed 12 years ago by nickm

Sent to mikeperry wrt his patch for #1:

See also Robert Hogan's suggestion on or-talk as of Wed, 26 Mar 2008
19:22:30 +0000. I think it might be a little more general than this.
Haven't thought too hard about it though.

As for the code: it looks mostly fine, but I don't like the way that
"reason" is overloaded to sometimes hold a remap reason and sometimes
overloaded to hold this new stream-source thing. Just because they're
currently orthogonal doesn't mean it's a good idea to use the same
field for both.

Also, as a nitpick, we usually abbreviate connection as "conn", not
"con".

comment:3 Changed 12 years ago by mikeperry

Just for the record, I think Robert Hogan's proposal is the way to go rather than my
initial patch. But I haven't had the time to grep through the source and find all the
locations for different purpose types...

comment:4 Changed 12 years ago by nickm

comment:5 Changed 12 years ago by nickm

Fix for #2 applied and backported.

comment:6 Changed 12 years ago by nickm

Part #1 implemented in svn and on 0.2.0.x branch.

comment:7 Changed 12 years ago by nickm

flyspray2trac: bug closed.

comment:8 Changed 7 years ago by nickm

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