Opened 4 years ago

Closed 4 years ago

#8701 closed defect (fixed)

Stem doesn't recognize error 555 on attach_stream

Reported by: cypherpunks Owned by: atagar
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-spec tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When trying to attach a stream that is not controlled by the controller using Controller.attach_stream, Tor will reply with error 555. Stem does not recognize this and will throw a generic exception:

stem.ProtocolError: ATTACHSTREAM returned unexpected response code: 555

I think an InvalidArguments exception would be best suited for this error code.

@@ -1981,6 +1981,8 @@ class Controller(BaseController):
         raise stem.InvalidRequest(response.code, response.message)
       elif response.code == '551':
         raise stem.OperationFailed(response.code, response.message)
+      elif response.code == '555':
+        raise stem.InvalidArguments(response.code, response.message)
       else:
         raise stem.ProtocolError("ATTACHSTREAM returned unexpected response code: %s" % response.code)

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by atagar

  • Component changed from Stem to Tor

Thanks! You uncovered not just a Stem issue but one with Tor. According to the spec ATTACHSTREAM should only respond with 250, 552, or 551.

I think an InvalidArguments exception would be best suited for this error code.

I'm not entirely clear what a 555 response means. I'd rather not have attach_stream() raise a new kind of exception before the spec is updated, so raising a OperationFailed for the time being (stem change).

Would you mind printing raw_content() for this ControlMessage? I'd love to include it in our unit tests. :)

Reassigning this to the tor component for the spec fix.

comment:2 Changed 4 years ago by arma

  if (ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_CONTROLLER_WAIT &&
      ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_CONNECT_WAIT &&
      ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_RESOLVE_WAIT) {
    connection_write_str_to_buf(
                    "555 Connection is not managed by controller.\r\n",
                    conn);
    return 0;
  }

This happens when you try to attachstream a stream that isn't in a state where we think you should be able to do so. For example, if you try to attachstream an open stream, it should fail.

That said, there may be a bug here: the 'if' I just quoted doesn't consider AP_CONN_STATE_CIRCUIT_WAIT. That is, you can't ask to attachstream a stream that is waiting for Tor to produce a suitable circuit. That is probably because we assumed an implicit invariant that there *is* no acceptable circuit for a stream in this state (if there were one, the stream wouldn't still be in this state). But a) we might be wrong, and b) why constrain controllers?

comment:3 Changed 4 years ago by arma

diff --git a/control-spec.txt b/control-spec.txt
index 4498fbe..adf4fd1 100644
--- a/control-spec.txt
+++ b/control-spec.txt
@@ -837,8 +837,9 @@
   Hops are 1-indexed; generally, it is not permitted to attach to hop 1.
 
   Tor responds with "250 OK" if it can attach the stream, 552 if the circuit
-  or stream didn't exist, or 551 if the stream couldn't be attached for
-  another reason.
+  or stream didn't exist, 555 if the stream isn't in an appropriate
+  state to be attached (e.g. it's already open), or 551 if the stream
+  couldn't be attached for another reason.
 
   {Implementation note: Tor will close unattached streams by itself,
   roughly two minutes after they are born. Let the developers know if

is probably the spec change you had in mind

comment:4 Changed 4 years ago by atagar

Thanks Roger! Revised stem's handling to raise a UnsatisfiableRequest instead and added this to our tests (change). Spec change looks good to me.

comment:5 Changed 4 years ago by nickm

  • Keywords tor-spec added
  • Milestone set to Tor: 0.2.4.x-final
  • Status changed from new to needs_review

comment:6 Changed 4 years ago by nickm

Roger, that kind of inline patch is annoying for me to apply, since I have to copy-and-paste it and write a commit message and attribute it to you. "git format-patch" or a mergeable branch is much easier on my end.

comment:7 Changed 4 years ago by nickm

Merged it.

comment:8 Changed 4 years ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

comment:9 follow-up: Changed 4 years ago by arma

  • Keywords tor-spec removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I think there remains a bug here. See my "That said, there may be a bug here" paragraph. Why do we prevent the controller from attachstreaming a circuit that's currently in state AP_CONN_STATE_CIRCUIT_WAIT ?

comment:10 in reply to: ↑ 9 Changed 4 years ago by nickm

  • Keywords tor-spec tor-client added

Replying to arma:

I think there remains a bug here. See my "That said, there may be a bug here" paragraph. Why do we prevent the controller from attachstreaming a circuit that's currently in state AP_CONN_STATE_CIRCUIT_WAIT ?

Because there's probably a race condition in your program if you expect that to work.

This can happen if you are not setting LeaveStreamsUnattached, and you are trying to call attachstream on user streams. But that case indicates a questionable controller program, since there's no way for you to make sure that your choice of circuit will be honored. Instead, the circuit that the stream is waiting for might complete, and the stream might attach to that one instead of yours.

It can also happen if you are setting LeaveStreamsUnattached, and you are trying to call attachstream on Tor-internal streams, like directory requests. But that's even more unsupported.

comment:11 Changed 4 years ago by arma

  • Resolution set to fixed
  • Status changed from reopened to closed

Ok. In the interest of not adding more work for me, I agree. :) Closing.

Note: See TracTickets for help on using tickets.