Opened 11 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:
Severity: Keywords:
Cc: arma, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


(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 11 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;
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

Version 0, edited 11 years ago by arma (next)

comment:2 Changed 11 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

comment:3 Changed 11 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 11 years ago by nickm

comment:5 Changed 11 years ago by nickm

Fix for #2 applied and backported.

comment:6 Changed 11 years ago by nickm

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

comment:7 Changed 11 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.